-
Notifications
You must be signed in to change notification settings - Fork 6k
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
Support experimental feature pragma #2690
Conversation
This has no tests yet, let us first agree it does everything the way it should:
Now when checking for a specific feature, I'd say to check if |
Possibly also it should issue a warning whenever the pragma is encountered. |
I think this is a good start. Right now, it looks like if I typed in a non-existant experimental feature (e.g. "pragma experimental foo"), this would end up being basically a no-op, so we don't really get the benefit of the compiler spotting typos. What if experimental features could register their pragma names, and then if they weren't on the list, the |
I think to keep it simple just add the supported const list in |
Also the wildcard will not be able to do much type checking :) |
I guess since we currently only have two features, a simple const list in the SyntaxChecker works. No reason to get overly complicated. |
Also once any of these features (z3, overflow check) graduate from experimental status, we could still keep them optionally turned on by |
b7e2a99
to
3dff8c0
Compare
else | ||
if (_pragma.tokens()[0] != Token::Identifier) | ||
m_errorReporter.syntaxError(_pragma.location(), "Invalid pragma \"" + _pragma.literals()[0] + "\""); | ||
else if (_pragma.literals()[0] == "experimental") |
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.
Depending on the feature, this has to be done much earlier, probably best in the parser.
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.
Sorry, confused that with SemanticAnalyzer.
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.
Agreed, but that requires a few more helpers as the parser doesn't currently maintain what sourceunit it is in.
Idea: how about adding an experimental tag into the metadata CBOR if any experimental mode is selected? Reason: expose that the contract should not be relied on in the bytecode and not only in the metadata JSON, which very linkely will not be uploaded anywhere. Effect: regular contract validators will just deny because the CBOR looks invalid to them, and those which understand the flag will have to stop because of the flag. Example:
|
We've also decided offline to:
|
3dff8c0
to
ae0390b
Compare
7ecfd9a
to
4c942c7
Compare
I'd like to split off the metadata stamp changes to a separate PR, because that might require more decision, while this above should be ready. |
@chriseth do you want to review this after the release? Perhaps we could reduce the number of error messages, it may be too much detail. |
|
||
enum class ExperimentalFeature {}; | ||
|
||
static const std::map<std::string, ExperimentalFeature> experimentalFeatureNames = {}; |
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.
Can you call this g_experimentalFeatureNames
or put it inside a class?
4c942c7
to
470950e
Compare
Updated |
@axic slightly confused here. Are multiple identifiers allowed after |
No, it was changed to only a single one. |
Implements #2346.