fix[lang]: improve error message for exprs in type position#4898
fix[lang]: improve error message for exprs in type position#4898rafael-abuawad wants to merge 22 commits into
Conversation
Resolving a name to VarInfo (e.g. a constant passed to empty()) is a user/type error, not an internal compiler failure. Raise InvalidType with the existing message; preserve CompilerPanic for unexpected values. Fixes vyperlang#4865 Made-with: Cursor
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #4898 +/- ##
=======================================
Coverage 92.06% 92.06%
=======================================
Files 187 187
Lines 27132 27135 +3
Branches 4758 4759 +1
=======================================
+ Hits 24978 24981 +3
Misses 1437 1437
Partials 717 717 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
charles-cooper
left a comment
There was a problem hiding this comment.
thanks for the submission, looks fine overall, requested style changes
|
I'm not sure this is the right fix, it seems to me like a better error for MAX_MESSAGES: constant(uint256) = 64
@external
def parse_blob(blob: Bytes[4096]):
starts: DynArray[uint16, MAX_MESSAGES] = empty(MAX_MESSAGES)would be something like: Because There's also the question of whether we should just allow the above, since |
|
@Sporarum The error is not that they are using a constant for the length of the |
|
@rafael-abuawad oh, of course Then I think it would make sense to add a couple more tests (if they don't already exist) of similar cases, for example: (and every other spot where a type can be) |
|
@Sporarum im going to add a few more tests maybe 2 or 3 to see how it handles that. And going to remove the compile panic line |
Remove test for non-Vyper non-varinfo type raising CompilerPanic.
Removed unnecessary blank line in utils.py
Co-authored-by: Charles Cooper <cooper.charles.m@gmail.com>
|
@rafael-abuawad did you get around to add more tests ? |
Hey @Sporarum , no, not yet I'm going to do so this weekend hopefully |
|
@Sporarum I've added more tests, but I don't know if that's enough |
|
@rafael-abuawad For example: But it's also something I can do myself if you prefer |
@Sporarum understood, I’ve added more tests. Sorry, I became too short-sighted. I didn’t want to go overboard with the tests, but I think this list covers enough |
|
Very nice, thanks ! |
|
@Sporarum @charles-cooper do you need anything else from my side to have this merged? |
|
Looks good, will merge shortly |
|
@rafael-abuawad looks like there is something not resolving correctly on initialization, can you fix? probably if you get lint passing it should be good |
| UnknownType, | ||
| ) | ||
| from vyper.semantics.analysis.levenshtein_utils import get_levenshtein_error_suggestions | ||
| from vyper.semantics.analysis.base import VarInfo |
|
@charles-cooper I think that was it, I was able to run the lint properly, but I don't know how to re-run the checks |
What I did
Fixed a compiler panic when a name in a type position resolves to a
VarInfo. The compiler now raisesInvalidTypewith the existing'…' is not a type!message for that case, and keepsCompilerPanicfor other unexpected non-VyperType namespace values.How I did it
In
_type_from_annotationinvyper/semantics/types/utils.py, after resolving aNamethrough the namespace and unwrappingModuleInfo, the code now checksisinstance(typ_, VarInfo)before the fallthroughCompilerPanic. If it is aVarInfo, it raisesInvalidType(err_msg, node)using the existing error message; otherwise theCompilerPanicis preserved for truly unexpected values.How to verify it
Commit message
Description for the changelog
Semantics: treat VarInfo in type position as InvalidType, not CompilerPanic (GH 4865).
Cute Animal Picture