-
Notifications
You must be signed in to change notification settings - Fork 6.3k
Introduce experimental flag/option #16229
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
base: develop
Are you sure you want to change the base?
Conversation
1beb509 to
ba48a91
Compare
fd70f82 to
98cad7c
Compare
5c5a463 to
2518378
Compare
2518378 to
83b670d
Compare
|
This pull request is stale because it has been open for 14 days with no activity. |
83b670d to
17c3e0e
Compare
6c4e799 to
136bd18
Compare
136bd18 to
8f8a1dc
Compare
test/libsolidity/syntaxTests/experimental/builtin/builtin_type_definition.sol
Show resolved
Hide resolved
|
Note that there is an issue for this: #16123. Please include it in the description and also check if the PR covers everything. |
|
|
||
| Compiler Features: | ||
| * Commandline Interface: Introduce `--experimental` flag required for toggling the experimental mode. | ||
| * Standard JSON Interface: Introduce `settings.experimental` setting required for toggling the experimental mode. |
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.
I wonder if it wouldn't be a good idea to make a prerelease once we merge this PR.
I imagine that this kind of change could cause some disruption for those who were relying on experimental features but shouldn't have.
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.
Should we also mention (as a separate entry) that some features now require the experimental mode (and list those features)? If we do make a prerelease that will be the primary source of information about this.
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.
Yes, we should also notify the solc-tooling group in advance - perhaps even get Lea or whoever has access to our twitter to tweet this out so that tooling is aware.
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.
Let's do that.
5f3fe19 to
422183a
Compare
| /// @use-src 0:\"C\" | ||
| object \"C_2\" { | ||
| code { | ||
| /// @src 0:56:69 |
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.
I think we have a bug in debugInfo option. The @src annotation is supposed to be present only if "location" is on the list and here it's not (you only included "ethdebug").
| bool const experimentalMode = !onlySafeExperimentalFeaturesActivated( | ||
| _contract.contract->sourceUnit().annotation().experimentalFeatures | ||
| ); | ||
| ) || m_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.
You can reduce this to just m_experimental and assert the rest. The analysis is supposed to report an error if any experimental feature is used without m_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.
Note also that we have this weird "safe experimental feature" concept here. Apparently it applies to SMTChecker and ABI coder:
solidity/libsolidity/ast/ExperimentalFeatures.h
Lines 39 to 44 in d28d552
| static std::set<ExperimentalFeature> const ExperimentalFeatureWithoutWarning = | |
| { | |
| ExperimentalFeature::ABIEncoderV2, | |
| ExperimentalFeature::SMTChecker, | |
| ExperimentalFeature::TestOnlyAnalysis, | |
| }; |
Oddly, before #7759 which made ABI coder v2 non-experimental, this was instead a list of "analysis-only experimental features". Then it was renamed.
In any case, those happen to be exactly those problematic pragmas that were holding back this PR. Since we were actually going out of our way not to warn about these features, maybe we should just keep the mechanism and exempt from the --experimental flag? Otherwise the mechanism has no use and I'd remove it completely.
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.
You can reduce this to just
m_experimentaland assert the rest. The analysis is supposed to report an error if any experimental feature is used withoutm_experimental.
Assert in what way - both m_eofVersion.has_value() and !onlySafeExperimentalFeaturesActivated(...) are just subsets of m_experimental? Asserting every single experimental feature would be a bit of an overkill with the entire OR chain of features.
I'd rather just get rid of it completely.
61b71f3 to
6302544
Compare
9559fde to
91c51ea
Compare
81bd190 to
2ffef5a
Compare
Under development, don't review yet.
Partially (for now) implements #16123