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.

Sign up for GitHub

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

Jump to bottom

Syntax for defining custom errors. #10987

Merged
merged 1 commit into from
Mar 30, 2021
Merged

Syntax for defining custom errors. #10987

merged 1 commit into from
Mar 30, 2021

Conversation

chriseth
Copy link
Contributor

Split out of #10922

Added the check for reserved signatures, but I'm still looking for an example for the all-ones-signature.

@chriseth chriseth requested review from hrkrshnn and axic February 22, 2021 17:25
hrkrshnn
hrkrshnn reviewed Feb 22, 2021
View reviewed changes
Copy link
Member

@hrkrshnn hrkrshnn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tests are failing.

libsolidity/analysis/ContractLevelChecker.cpp Outdated Show resolved Hide resolved
libsolidity/ast/AST.h Outdated Show resolved Hide resolved
libsolidity/ast/ASTJsonConverter.cpp Outdated Show resolved Hide resolved
libsolidity/ast/ASTUtils.cpp Outdated Show resolved Hide resolved
@chriseth chriseth force-pushed the customErrorDeclaration branch 2 times, most recently from c964b90 to 04b0326 Compare February 23, 2021 09:24
@hrkrshnn
Copy link
Member

test266151307() has the selector 0xffffffff. Only took two minutes on a single thread to compute it!

@chriseth chriseth force-pushed the customErrorDeclaration branch 2 times, most recently from 19ad0b3 to 37b4972 Compare February 23, 2021 10:41
@chriseth
Copy link
Contributor Author

Ah great, thanks!
Added example and also changed the formatting of the selector to hex.

hrkrshnn
hrkrshnn reviewed Feb 23, 2021
View reviewed changes
test/libsolidity/syntaxTests/errors/weird2.sol Outdated Show resolved Hide resolved
test/libsolidity/syntaxTests/errors/error_location_memory.sol
@@ -0,0 +1,3 @@
error E(uint[] memory);
// ----
// ParserError 2314: (15-21): Expected ',' but got 'memory'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This error message is rather confusing. Not sure if improving this is easy.

libsolidity/parsing/Parser.cpp Show resolved Hide resolved
libsolidity/codegen/ExpressionCompiler.cpp Show resolved Hide resolved
libsolidity/ast/ASTUtils.cpp Outdated Show resolved Hide resolved
hrkrshnn
hrkrshnn previously approved these changes Feb 23, 2021
View reviewed changes
Copy link
Member

@hrkrshnn hrkrshnn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can be merged after squashing the last commit.

@chriseth
Copy link
Contributor Author

We should discuss on Wednesday if we want to merge this already.

@chriseth chriseth force-pushed the customErrorDeclaration branch from c019d7b to d31bd77 Compare February 23, 2021 17:03
@chriseth chriseth mentioned this pull request Feb 24, 2021
Merged
@chriseth chriseth changed the title Syntax for custom errors. Syntax for defining custom errors. Mar 3, 2021
@chriseth chriseth force-pushed the customErrorDeclaration branch 2 times, most recently from 4b8159f to b4ba6d1 Compare March 9, 2021 15:47
@chriseth chriseth force-pushed the customErrorDeclaration branch from b4ba6d1 to ae5fa34 Compare March 16, 2021 18:18
cameel
cameel reviewed Mar 18, 2021
View reviewed changes
Copy link
Member

@cameel cameel left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I haven't found anything serious. Pretty much only minor/stylistic tweaks and test case suggestions.

The only other thing is that I noticed that we allow @return for events and have assertions about their return arguments. I thought events do not have those?

There are also some unresolved TODOs but given that there are more PRs on top of this, they might be fine.

docs/grammar/Solidity.g4 Outdated Show resolved Hide resolved
libsolidity/analysis/DocStringTagParser.cpp Show resolved Hide resolved
libsolidity/analysis/SyntaxChecker.cpp Outdated Show resolved Hide resolved
libsolidity/analysis/ContractLevelChecker.cpp Outdated Show resolved Hide resolved
libsolidity/analysis/ContractLevelChecker.cpp Outdated Show resolved Hide resolved
libsolidity/ast/ASTUtils.cpp Outdated Show resolved Hide resolved
libsolidity/ast/Types.cpp Outdated Show resolved Hide resolved
libsolidity/ast/Types.cpp Show resolved Hide resolved
test/libsolidity/syntaxTests/errors/clash_function_error_inheritance.sol Show resolved Hide resolved
test/libsolidity/syntaxTests/errors/weird4.sol Show resolved Hide resolved
@cameel cameel mentioned this pull request Mar 19, 2021
Merged
@chriseth chriseth force-pushed the customErrorDeclaration branch 8 times, most recently from 4438a59 to d0cec87 Compare March 25, 2021 11:00
cameel
cameel previously approved these changes Mar 25, 2021
View reviewed changes
Copy link
Member

@cameel cameel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is ready now.

There's still one pending question where I'd like to get an answer but it's about events, not errors so there's no reason for it to block this PR: #10987 (comment)

@leonardoalt
Copy link
Member

DO NOT MERGE!
@cameel said that @chriseth wants to merge all custom errors PR together.

@chriseth chriseth force-pushed the customErrorDeclaration branch from d0cec87 to b04b189 Compare March 30, 2021 19:15
@chriseth chriseth merged commit 317eaf6 into develop Mar 30, 2021
@chriseth chriseth deleted the customErrorDeclaration branch March 30, 2021 21:03
@cameel cameel mentioned this pull request Apr 24, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hrkrshnn hrkrshnn hrkrshnn left review comments

@cameel cameel cameel left review comments

@axic axic Awaiting requested review from axic

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@chriseth @hrkrshnn @leonardoalt @cameel