-
Notifications
You must be signed in to change notification settings - Fork 208
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
Represent type(f) correctly in the ast #1609
Conversation
This makes the ast correct, and also the following code is now parsed correctly: function foo() { type(int); } Signed-off-by: Sean Young <sean@mess.org>
Signed-off-by: Sean Young <sean@mess.org>
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #1609 +/- ##
==========================================
+ Coverage 87.94% 88.14% +0.19%
==========================================
Files 137 137
Lines 65902 65992 +90
==========================================
+ Hits 57959 58166 +207
+ Misses 7943 7826 -117 ☔ View full report in Codecov by Sentry. |
Signed-off-by: Sean Young <sean@mess.org>
Signed-off-by: Sean Young <sean@mess.org>
Signed-off-by: Sean Young <sean@mess.org>
3f99f81
to
e37a8c8
Compare
ast::Builtin::TypeRuntimeCode | ast::Builtin::TypeCreatorCode => { | ||
let ast::Expression::TypeOperator { | ||
ty: Type::Contract(contract_no), | ||
.. | ||
} = &args[0] |
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.
Instead of a builtin, wouldn't it be nicer of item we access from a type operator was a member access? This way you could define the possible members for the operator.
I found it somewhat uncomfortable that you require the argument to be TypeOperator
in all cases with an unreachable.
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.
As there is a constraint for the argument, perhaps it could be enforced in another way that avoids the unreachable.
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.
What do you mean by member access? Expression::StructMember
? That expression is not suitable, it takes a field number and it has "Struct" in its name.
You may want the location of type(..)
in type(int) . min
and type(int)
can be free standing too, so a separate Expression::TypeOperator
enum makes sense to me.
Please make suggestions because I have no idea.
let mut creates = Vec::new(); | ||
let mut diagnostics = Diagnostics::default(); | ||
|
||
swap(&mut creates, &mut ns.contracts[contract_no].creates); |
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.
Why is the swap necessary?
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.
borrow checker
Co-authored-by: Lucas Steuernagel <38472950+LucasSte@users.noreply.github.com> Signed-off-by: Sean Young <sean@mess.org>
If there are no further comments, I'll merge |
This makes the ast correct, and also the following code is now parsed correctly:
type(enum-type).min
ortype(enum).min
is now supported.This fixes all the issues wrt
type(T)
in the solc tests.