Adding binding rules to typeof(<enum>).min and max, and to <enum>.<member> so they work with extensions
#1177
Conversation
|
There was a problem hiding this comment.
I think this needs rebasing on latest main.
cc @ggiraldez if you don't mind a quick review?
|
I suppose it's best to wait for #1149 to be in sync with main, and then rebase this one on top of it |
| @@ -1,7 +1,17 @@ | |||
| enum Answer { | |||
There was a problem hiding this comment.
It would be preferable to have a separate test for this, and make it as small as possible. Another suggestion is to use a library for extending the type, as that approach is compatible with all versions of Solidity.
| edge type -> typeof | ||
| edge typeof -> built_in_member | ||
| edge built_in_member -> min_built_in | ||
| edge built_in_member -> max_built_in | ||
| edge min_built_in -> min_max_accessor | ||
| edge max_built_in -> min_max_accessor | ||
| edge min_max_accessor -> @enum.lexical_scope |
There was a problem hiding this comment.
This is not correct. The trailing . in the path (the min_max_accessor node) allows binding min. with anything in the lexical scope, which is why the test case resolves correctly. But it would also bind to any symbol at that scope.
Keep in mind you should "replace" (as in the symbol stack, pop and then push) min and max with a @typeof -> X, where X is the argument to type() so that you can continue the binding resolution at X.
beta-ziliani
force-pushed
the
beta/1158
branch
from
9bfd79f to
0a578b3
Compare
There was a problem hiding this comment.
This file is a left over from #1196, but it doesn't harm to have it here
beta-ziliani
changed the title
typeof(<enum>).min and max, and to <enum>.<member> so they work with extensions
| node minmax_type_of | ||
| attr (minmax_type_of) push_symbol = "@typeof" | ||
| node minmax_replace | ||
| attr (minmax_replace) push_symbol = (source-text @name) |
There was a problem hiding this comment.
I suggest extracting this path pushing @typeof -> ENUM_NAME and re-using it in each member rules below as well. Something like @enum.value_ref_typeof. This would be a bit simpler and avoid creating extra nodes for each of the enum elements.
Also not too keen on the _replace suffix. Maybe value_ref_typeof and value_ref_type since both nodes form a reference path to the type of a value of the enum.
| node built_in_member | ||
| attr (built_in_member) pop_symbol = "." | ||
| node min_built_in | ||
| attr (min_built_in) pop_symbol = "min" | ||
| node max_built_in | ||
| attr (max_built_in) pop_symbol = "max" |
There was a problem hiding this comment.
Both min and max only exist in Solidity >= 0.6.8. It's not critical, but to be consistent with the built-in definitions these should be only added for those versions.
| node type_of | ||
| attr (type_of) push_symbol = "@typeof" | ||
| node type_def | ||
| attr (type_def) push_symbol = (source-text @name) | ||
| edge def -> type_of | ||
| edge type_of -> type_def | ||
| edge type_def -> @enum.lexical_scope |
There was a problem hiding this comment.
Here instead of repeating the path segment you can use the aforementioned path and do only add:
edge def -> @enum.value_ref_typeof`
You'll not need to capture the enum's name either.
Solves #1158.
Solves #1196.
Considerations:
It's built on top of Solidity binding fixes driven by Sanctuary #1149It's rebased on master.It's currently missing a test formaxI adapted an existing test, though maybe it's preferred to add a different one?using for, so it can very well be moved to theusingfolder.EDITED