Skip to content
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

Merged
merged 6 commits into from
Aug 10, 2017
Merged

Support experimental feature pragma #2690

merged 6 commits into from
Aug 10, 2017

Conversation

axic
Copy link
Member

@axic axic commented Aug 2, 2017

Implements #2346.

@axic
Copy link
Member Author

axic commented Aug 2, 2017

This has no tests yet, let us first agree it does everything the way it should:

  • pragma must be followed by an identifier (i.e. cannot be any other token), which can be followed by any token
  • experimental must be followed by at least one identifier, but multiple are accepted
  • duplicates are forbidden

Now when checking for a specific feature, I'd say to check if experimentalFeatures.count(feature) || experimentalFeatures.count('*'). We could add a helper for that on the annotation too: bool enablesExperimentalFeature(key).

@axic axic requested review from roadriverrail and chriseth and removed request for roadriverrail August 2, 2017 18:17
@axic
Copy link
Member Author

axic commented Aug 2, 2017

Possibly also it should issue a warning whenever the pragma is encountered.

@roadriverrail
Copy link
Contributor

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 SyntaxChecker could issue an error)?

@axic
Copy link
Member Author

axic commented Aug 2, 2017

I think to keep it simple just add the supported const list in SyntaxChecker.

@axic
Copy link
Member Author

axic commented Aug 2, 2017

Also the wildcard will not be able to do much type checking :)

@roadriverrail
Copy link
Contributor

I guess since we currently only have two features, a simple const list in the SyntaxChecker works. No reason to get overly complicated.

@axic
Copy link
Member Author

axic commented Aug 2, 2017

Also once any of these features (z3, overflow check) graduate from experimental status, we could still keep them optionally turned on by pragma feature z3;

@axic axic force-pushed the experimental-pragma branch from b7e2a99 to 3dff8c0 Compare August 2, 2017 19:22
else
if (_pragma.tokens()[0] != Token::Identifier)
m_errorReporter.syntaxError(_pragma.location(), "Invalid pragma \"" + _pragma.literals()[0] + "\"");
else if (_pragma.literals()[0] == "experimental")
Copy link
Contributor

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.

Copy link
Contributor

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.

Copy link
Member Author

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.

@axic
Copy link
Member Author

axic commented Aug 4, 2017

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:

{bzzr:"<swarmhash>",experimental:true}



@axic
Copy link
Member Author

axic commented Aug 4, 2017

We've also decided offline to:

  • have one pragma experimental per feature
  • do not support wildcard
  • have an enum for the features
  • keep the enum/string mapping in a new header AST/ExperimentalFeatures.h

@axic axic force-pushed the experimental-pragma branch from 3dff8c0 to ae0390b Compare August 4, 2017 22:28
@chriseth chriseth mentioned this pull request Aug 7, 2017
@axic axic force-pushed the experimental-pragma branch 4 times, most recently from 7ecfd9a to 4c942c7 Compare August 8, 2017 14:18
@axic
Copy link
Member Author

axic commented Aug 8, 2017

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.

@axic
Copy link
Member Author

axic commented Aug 8, 2017

@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 = {};
Copy link
Contributor

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?

@axic axic force-pushed the experimental-pragma branch from 4c942c7 to 470950e Compare August 9, 2017 23:15
@axic
Copy link
Member Author

axic commented Aug 9, 2017

Updated ExperimentalFeatureNames to start with an uppercase letter as it is a global (similar to Version.h)

@chriseth chriseth merged commit 41e3cbe into develop Aug 10, 2017
@axic axic deleted the experimental-pragma branch August 10, 2017 16:07
@duaraghav8
Copy link
Contributor

duaraghav8 commented Oct 10, 2017

@axic slightly confused here. Are multiple identifiers allowed after experimental? If yes, what's the syntax? (Tried comma-separated & space-separated with no luck)

@axic
Copy link
Member Author

axic commented Oct 10, 2017

No, it was changed to only a single one.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants