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

Make type error fatal to prevent assert failure at later point #8313

Merged
merged 1 commit into from
Feb 13, 2020

Conversation

Marenz
Copy link
Contributor

@Marenz Marenz commented Feb 13, 2020

fixes #8283, #8272

@Marenz Marenz force-pushed the checkFunctionsExistInIsoltest branch 2 times, most recently from 7a70fa6 to 5d53d3c Compare February 13, 2020 14:08
@@ -2,9 +2,8 @@ contract C {
uint256 Test;

function f() public {
emit Test();
emit f();
Copy link
Member

Choose a reason for hiding this comment

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

This is not what the test is supposed to test...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I thought the idea was to test the code that produces the error message "must emit event" and that one is only output if you use f(), if you use Test() you only get "Test is not callable"

Copy link
Member

Choose a reason for hiding this comment

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

Ah well, ok maybe it is, but then you can remove the uint256 Test; above.

Copy link
Member

Choose a reason for hiding this comment

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

Or maybe best: change Test to be something callable.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated

@Marenz Marenz force-pushed the checkFunctionsExistInIsoltest branch from 5d53d3c to 8911b58 Compare February 13, 2020 14:16
Copy link
Member

@ekpyron ekpyron left a comment

Choose a reason for hiding this comment

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

Yeah, I was wondering whether in this particular case, this should really be fatal, but the number of test cases we have about this with comments like "this used to crash" kind of confirms that it's a good idea to just make this fatal :-).

@ekpyron
Copy link
Member

ekpyron commented Feb 13, 2020

But maybe (independently of this PR) we should have a general discussion about whether to generally always make type errors fatal, whenever you cannot reasonably assign types to all expressions and document that somewhere...

Especially, since in the long term and considering stuff like error recovery we might want to deal with all these cases more gracefully...

@Marenz
Copy link
Contributor Author

Marenz commented Feb 13, 2020

Alternatively we could adopt something like an "ErrorType" used in places where the code was supposed to assign a type but errored (and similar dummy constructs)

@ekpyron
Copy link
Member

ekpyron commented Feb 13, 2020

Yeah... the problem with that is that we'd probably need special cases for the error type all over... at least that could happen and wouldn't be nice...

@Marenz Marenz merged commit c635377 into develop Feb 13, 2020
@Marenz Marenz deleted the checkFunctionsExistInIsoltest branch February 13, 2020 15:08
@chriseth
Copy link
Contributor

This is missing a changelog entry.

@chriseth
Copy link
Contributor

Created #8316 so that we do not forget it.

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.

ICE in solidity::frontend::TypeChecker::visit, Cannot declare variable with void (empty tuple) type.
3 participants