-
-
Notifications
You must be signed in to change notification settings - Fork 803
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix[ux]: fix error message for common typo #4363
base: master
Are you sure you want to change the base?
fix[ux]: fix error message for common typo #4363
Conversation
the `staticall` typo is very common, and was raising a hard-to-read error message from the python parser like "invalid syntax. Perhaps you forgot a comma?" this commit adds it to the pre-parser, so we can raise an exception after we have successfully gone through the python parser and have the ability to add source info and a hint.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think a more scalable approach is to rely on the existing levenshtein utility. It currently raises a circular import, but I we can move vyper/semantics/analysis/levenshtein_utils.py
to a top-level vyper/levenshtein_utils.py
.
...
NAMESPACE = VYPER_CLASS_TYPES | CUSTOM_STATEMENT_TYPES | CUSTOM_EXPRESSION_TYPES
...
def pre_parse(...):
...
try:
...
for token in token_list:
...
if typ == NAME:
...
else:
from vyper.semantics.analysis.levenshtein_utils import _get_levenshtein_error_suggestions
hint = _get_levenshtein_error_suggestions(string, NAMESPACE, 0.3)
if hint != "":
raise SyntaxException("Possible typo", code, start[0], start[1], hint=hint)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i don't think that's good, because
- the keyword might not be in the namespace (which could be addressed by hand-rolling the dict, i suppose)
- this is going to throw on all names (e.g.
my_var: ...
). we don't have enough info at this stage to determine whether we are parsing a keyword or any other name. we only have token info.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #4363 +/- ##
==========================================
- Coverage 91.23% 90.46% -0.78%
==========================================
Files 112 112
Lines 16010 16012 +2
Branches 2697 2698 +1
==========================================
- Hits 14607 14485 -122
- Misses 968 1072 +104
- Partials 435 455 +20 ☔ View full report in Codecov by Sentry. |
compile_code(bad_code) | ||
|
||
|
||
def test_bad_staticcall_keyword(): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The test contract itself is incomplete. Can we make sure we unit test only the introduced change here?
A correct (i.e. compileable) Vyper contract would look like this
from ethereum.ercs import IERC20
from ethereum.ercs import IERC20Detailed
@external
def foo():
extcall IERC20(msg.sender).transfer(
msg.sender,
convert(staticcall IERC20Detailed(msg.sender).decimals(), uint256),
)
so we could have the following here:
from ethereum.ercs import IERC20
from ethereum.ercs import IERC20Detailed
@external
def foo():
extcall IERC20(msg.sender).transfer(
msg.sender,
convert(staticall IERC20Detailed(msg.sender).decimals(), uint256),
)
Another example with nested staticcall
could be:
from ethereum.ercs import IERC20
from ethereum.ercs import IERC20Detailed
@view
def bar() -> uint256:
return staticcall IERC20(msg.sender).balanceOf(
convert(staticall IERC20Detailed(msg.sender).decimals(), address)
)
In the last example, if you replace view
with pure
, it will raise the staticcall
first instead of:
vyper.exceptions.StateAccessViolation: Cannot call a view function from a pure function
which is the expected behaviour IMO.
CUSTOM_EXPRESSION_TYPES = { | ||
"extcall": "ExtCall", | ||
"staticcall": "StaticCall", | ||
"staticall": "BadStaticCall", # common typo - we parse it to AST for better error reporting |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
some other (not so common, but still) typos are:
staticcal
(that one we should probably also cover)statiiccall
stacticall
staticcal
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
although I can imagine such mistakes being made, I don't like the idea that we would reserve 5 keywords just for error reporting (though I don't think such keywords would be used by users anyway)
very unlikely.. but for completness staticall: uint256
or def staticall():
pass
yieds same the same ex edit: added examples |
the
staticall
typo is very common, and was raising a hard-to-read error message from the python parser like "invalid syntax. Perhaps you forgot a comma?"this commit adds it to the pre-parser, so we can raise an exception after we have successfully gone through the python parser and have the ability to add source info and a hint.
What I did
How I did it
How to verify it
Commit message
Commit message for the final, squashed PR. (Optional, but reviewers will appreciate it! Please see our commit message style guide for what we would ideally like to see in a commit message.)
Description for the changelog
Cute Animal Picture