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 duplicate function definitons a fatal error #455

Merged

Conversation

cburgdorf
Copy link
Collaborator

What was wrong?

#451 demonstrates an interesting issue where we crash when trying to present a certain error. The reason for that is that the function is defined multiple times and when we retrieve the type information for another error we don't get the type for that function bar that we assume but for that other function bar that has a different type.

How was it fixed?

An easy way to fix this is to say that duplicate definitions are treated as fatal errors with the reasoning that it becomes unsafe to retrieve type information for definitions of which we have multiple for the same name.

@codecov-commenter
Copy link

codecov-commenter commented Jun 15, 2021

Codecov Report

Merging #455 (d897503) into master (163d1b0) will increase coverage by 0.12%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #455      +/-   ##
==========================================
+ Coverage   86.77%   86.90%   +0.12%     
==========================================
  Files          70       70              
  Lines        4803     4804       +1     
==========================================
+ Hits         4168     4175       +7     
+ Misses        635      629       -6     
Impacted Files Coverage Δ
analyzer/src/traversal/functions.rs 91.47% <100.00%> (+0.04%) ⬆️
parser/src/parser.rs 76.39% <0.00%> (+2.57%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 163d1b0...d897503. Read the comment docs.

@cburgdorf cburgdorf requested a review from sbillig June 15, 2021 18:05
Copy link
Collaborator

@sbillig sbillig left a comment

Choose a reason for hiding this comment

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

Looks good. I have some ideas about further reducing error fatalities that I might implement when the salsa stuff is in. Basically in this case the 'bar' function would be marked as "tainted", and only the use of 'bar' would be fatal. When (or if??) the analyzer is query-based, error fatality won't stop all analysis, just that of the current definition.

Comment on lines 9 to 11
4 │ ╭ pub def bar:
5 │ │ s
│ ╰──────^ Conflicting definition of contract `bar`
Copy link
Collaborator

Choose a reason for hiding this comment

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

I guess "contract" is a copy-paste issue neither of us noticed last time. Probably worth fixing now.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Oops, right 👍

When we have a duplicate function defintion it becomes
unsafe to retrieve type information about such function
because it can not be guaranteed whether we retrieve
the type for function a or a'.

Fixes ethereum#451
@cburgdorf cburgdorf force-pushed the christoph/fix/error-message-duplicate-def branch from ec7e1a1 to d897503 Compare June 17, 2021 08:15
@cburgdorf cburgdorf merged commit f35aa01 into ethereum:master Jun 17, 2021
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.

None yet

3 participants