-
Notifications
You must be signed in to change notification settings - Fork 6.3k
Emit warnings for keywords selected to be reserved in the future #16206
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?
Changes from all commits
81dcbe9
b20a864
1445742
d0f575a
fb8ef20
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -46,6 +46,7 @@ Solidity language without a compiler change. | |||||
| pragma solidity >=0.4.16 <0.9.0; | ||||||
|
|
||||||
| library GetCode { | ||||||
| // This will report a warning - at you will be promoted to reserved keyword | ||||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
| function at(address addr) public view returns (bytes memory code) { | ||||||
| assembly { | ||||||
| // retrieve the size of the code, this needs assembly | ||||||
|
|
||||||
| Original file line number | Diff line number | Diff line change | ||||||||
|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -391,6 +391,9 @@ bool SyntaxChecker::visit(ContractDefinition const& _contract) | |||||||||
| "Functions are not allowed to have the same name as the contract. " | ||||||||||
| "If you intend this to be a constructor, use \"constructor(...) { ... }\" to define it." | ||||||||||
| ); | ||||||||||
|
|
||||||||||
| checkFutureKeyword(_contract); | ||||||||||
|
|
||||||||||
| return true; | ||||||||||
| } | ||||||||||
|
|
||||||||||
|
|
@@ -477,6 +480,8 @@ bool SyntaxChecker::visit(FunctionDefinition const& _function) | |||||||||
| else if (!_function.isImplemented() && !_function.modifiers().empty()) | ||||||||||
| m_errorReporter.syntaxError(2668_error, _function.location(), "Functions without implementation cannot have modifiers."); | ||||||||||
|
|
||||||||||
| checkFutureKeyword(_function); | ||||||||||
|
|
||||||||||
| return true; | ||||||||||
| } | ||||||||||
|
|
||||||||||
|
|
@@ -508,5 +513,31 @@ bool SyntaxChecker::visitNode(ASTNode const& _node) | |||||||||
| solAssert(m_sourceUnit); | ||||||||||
| solAssert(m_sourceUnit->experimentalSolidity()); | ||||||||||
| } | ||||||||||
| auto const* declaration = dynamic_cast<Declaration const*>(&_node); | ||||||||||
| if (declaration) | ||||||||||
| checkFutureKeyword(*declaration); | ||||||||||
| return ASTConstVisitor::visitNode(_node); | ||||||||||
| } | ||||||||||
|
|
||||||||||
|
|
||||||||||
| void SyntaxChecker::checkFutureKeyword(Declaration const& _declaration) | ||||||||||
| { | ||||||||||
| std::set<ASTString> const futureKeywords = { | ||||||||||
| "transient", | ||||||||||
| "layout", | ||||||||||
| "at", | ||||||||||
| "error", | ||||||||||
| "super", | ||||||||||
| "this" | ||||||||||
| }; | ||||||||||
|
Comment on lines
+525
to
+532
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We should put the list in a more general location. |
||||||||||
| if (futureKeywords.count(_declaration.name())) | ||||||||||
| m_errorReporter.warning( | ||||||||||
|
Comment on lines
+533
to
+534
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||||
| 6335_error, | ||||||||||
| _declaration.location(), | ||||||||||
| fmt::format( | ||||||||||
| "\"{}\" will be promoted to reserved keyword in the next breaking version" | ||||||||||
| " and will not be allowed as an identifier anymore.", | ||||||||||
|
Comment on lines
+538
to
+539
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not necessarily something to change here, but just wanted to note that our terminology is all over the place: "keyword" vs "reserved keyword" vs "reserved identifier". We should get that straight at some point, because these are not synonyms. |
||||||||||
| _declaration.name() | ||||||||||
| ) | ||||||||||
| ); | ||||||||||
| } | ||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -298,6 +298,7 @@ void AsmAnalyzer::operator()(VariableDeclaration const& _varDecl) | |
| for (auto const& variable: _varDecl.variables) | ||
| { | ||
| expectValidIdentifier(variable.name, nativeLocationOf(variable)); | ||
| checkFutureReservedKeyword(variable.name, nativeLocationOf(variable)); | ||
| } | ||
|
|
||
| if (_varDecl.value) | ||
|
|
@@ -326,6 +327,7 @@ void AsmAnalyzer::operator()(FunctionDefinition const& _funDef) | |
| { | ||
| yulAssert(!_funDef.name.empty()); | ||
| expectValidIdentifier(_funDef.name, nativeLocationOf(_funDef)); | ||
| checkFutureReservedKeyword(_funDef.name, nativeLocationOf(_funDef)); | ||
| Block const* virtualBlock = m_info.virtualBlocks.at(&_funDef).get(); | ||
| yulAssert(virtualBlock, ""); | ||
| Scope& varScope = scope(virtualBlock); | ||
|
|
@@ -953,3 +955,30 @@ void AsmAnalyzer::validateObjectStructure(langutil::SourceLocation const& _astRo | |
| } | ||
| } | ||
| } | ||
|
|
||
| void AsmAnalyzer::checkFutureReservedKeyword(YulName _identifier, langutil::SourceLocation const& _location) | ||
| { | ||
| std::set<std::string> futureReservedKeywords{"leave"}; | ||
| if (m_evmVersion < langutil::EVMVersion::london()) | ||
| futureReservedKeywords.insert("basefee"); | ||
| if (m_evmVersion < langutil::EVMVersion::paris()) | ||
| futureReservedKeywords.insert("prevrandao"); | ||
| if (m_evmVersion < langutil::EVMVersion::cancun()) | ||
| { | ||
|
Comment on lines
+962
to
+967
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This warning should not be EVM-version dependent. When we make these into keywords, they will be disallowed regardless of EVM version. |
||
| futureReservedKeywords.insert("blobbasefee"); | ||
| futureReservedKeywords.insert("blobhash"); | ||
| futureReservedKeywords.insert("mcopy"); | ||
| futureReservedKeywords.insert("tstore"); | ||
| futureReservedKeywords.insert("tload"); | ||
| } | ||
| if (futureReservedKeywords.count(_identifier.str())) | ||
| m_errorReporter.warning( | ||
| 5470_error, | ||
| _location, | ||
| fmt::format( | ||
| "\"{}\" will be promoted to reserved keyword in the next breaking version " | ||
| "and will not be allowed anymore as an identifier.", | ||
| _identifier.str() | ||
| ) | ||
| ); | ||
| } | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'd add a few more tests:
|
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1 +1,3 @@ | ||
| contract transient {} | ||
| // ---- | ||
| // Warning 6335: (0-21): "transient" will be promoted to reserved keyword in the next breaking version and will not be allowed as an identifier anymore. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,2 +1,3 @@ | ||
| contract at layout at 0x1234ABC { } | ||
| // ---- | ||
| // Warning 6335: (0-35): "at" will be promoted to reserved keyword in the next breaking version and will not be allowed as an identifier anymore. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,2 +1,3 @@ | ||
| contract layout layout at 0x1234ABC { } | ||
| // ---- | ||
| // Warning 6335: (0-39): "layout" will be promoted to reserved keyword in the next breaking version and will not be allowed as an identifier anymore. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,3 +1,4 @@ | ||
| contract at layout at uint40(bytes5(hex"0011223344")) { } | ||
| // ---- | ||
| // Warning 6335: (0-57): "at" will be promoted to reserved keyword in the next breaking version and will not be allowed as an identifier anymore. | ||
| // TypeError 6396: (22-53): The base slot of the storage layout must evaluate to a rational number. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,3 +1,4 @@ | ||
| contract at layout at uint(42) { } | ||
| // ---- | ||
| // Warning 6335: (0-34): "at" will be promoted to reserved keyword in the next breaking version and will not be allowed as an identifier anymore. | ||
| // TypeError 6396: (22-30): The base slot of the storage layout must evaluate to a rational number. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,3 +1,4 @@ | ||
| contract at layout at type(uint).max { } | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'd rename the contract here and in other similar tests to something other than |
||
| // ---- | ||
| // Warning 6335: (0-40): "at" will be promoted to reserved keyword in the next breaking version and will not be allowed as an identifier anymore. | ||
| // TypeError 6396: (22-36): The base slot of the storage layout must evaluate to a rational number. | ||
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.
The PR needs a changelog entry.
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.
Perhaps we should also add a note/warning about the new keywords under Reserved Keywords in the docs.