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

Conversation

Projects
None yet
4 participants
@axic
Member

axic commented Aug 2, 2017

Implements #2346.

@axic axic added the in progress label Aug 2, 2017

@axic

This comment has been minimized.

Show comment
Hide comment
@axic

axic Aug 2, 2017

Member

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).

Member

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 Aug 2, 2017

@axic

This comment has been minimized.

Show comment
Hide comment
@axic

axic Aug 2, 2017

Member

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

Member

axic commented Aug 2, 2017

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

@roadriverrail

This comment has been minimized.

Show comment
Hide comment
@roadriverrail

roadriverrail Aug 2, 2017

Collaborator

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)?

Collaborator

roadriverrail commented Aug 2, 2017

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

This comment has been minimized.

Show comment
Hide comment
@axic

axic Aug 2, 2017

Member

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

Member

axic commented Aug 2, 2017

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

@axic

This comment has been minimized.

Show comment
Hide comment
@axic

axic Aug 2, 2017

Member

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

Member

axic commented Aug 2, 2017

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

@roadriverrail

This comment has been minimized.

Show comment
Hide comment
@roadriverrail

roadriverrail Aug 2, 2017

Collaborator

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

Collaborator

roadriverrail commented Aug 2, 2017

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

@axic

This comment has been minimized.

Show comment
Hide comment
@axic

axic Aug 2, 2017

Member

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;

Member

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;

else
if (_pragma.tokens()[0] != Token::Identifier)
m_errorReporter.syntaxError(_pragma.location(), "Invalid pragma \"" + _pragma.literals()[0] + "\"");
else if (_pragma.literals()[0] == "experimental")

This comment has been minimized.

@chriseth

chriseth Aug 3, 2017

Contributor

Depending on the feature, this has to be done much earlier, probably best in the parser.

@chriseth

chriseth Aug 3, 2017

Contributor

Depending on the feature, this has to be done much earlier, probably best in the parser.

This comment has been minimized.

@chriseth

chriseth Aug 4, 2017

Contributor

Sorry, confused that with SemanticAnalyzer.

@chriseth

chriseth Aug 4, 2017

Contributor

Sorry, confused that with SemanticAnalyzer.

This comment has been minimized.

@axic

axic Aug 4, 2017

Member

Agreed, but that requires a few more helpers as the parser doesn't currently maintain what sourceunit it is in.

@axic

axic Aug 4, 2017

Member

Agreed, but that requires a few more helpers as the parser doesn't currently maintain what sourceunit it is in.

@axic

This comment has been minimized.

Show comment
Hide comment
@axic

axic Aug 4, 2017

Member

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}



Member

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

This comment has been minimized.

Show comment
Hide comment
@axic

axic Aug 4, 2017

Member

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
Member

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 added the nextrelease label Aug 4, 2017

@chriseth chriseth referenced this pull request Aug 7, 2017

Merged

New ABI encoder #2704

@axic

This comment has been minimized.

Show comment
Hide comment
@axic

axic Aug 8, 2017

Member

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.

Member

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

This comment has been minimized.

Show comment
Hide comment
@axic

axic Aug 8, 2017

Member

@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.

Member

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.

@axic

This comment has been minimized.

Show comment
Hide comment
@axic

axic Aug 9, 2017

Member

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

Member

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

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@axic axic deleted the experimental-pragma branch Aug 10, 2017

@duaraghav8

This comment has been minimized.

Show comment
Hide comment
@duaraghav8

duaraghav8 Oct 10, 2017

Contributor

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

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

This comment has been minimized.

Show comment
Hide comment
@axic

axic Oct 10, 2017

Member

No, it was changed to only a single one.

Member

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