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

Check labels in function calls on self and external contracts #518

Merged
merged 1 commit into from
Aug 20, 2021

Conversation

cburgdorf
Copy link
Collaborator

What was wrong?

As explained in #517, function calls on self or external contracts do not check the labels, allowing developers to apply any kind of label and in the worst case leave the user with the impression that they could use labels to call functions with the arguments ordered as they wish (which isn't the case).

How was it fixed?

Added checks in the analyzer and added tests.

@cburgdorf cburgdorf requested a review from sbillig August 19, 2021 09:38
@codecov-commenter
Copy link

codecov-commenter commented Aug 19, 2021

Codecov Report

Merging #518 (7fcb53e) into master (b4d819f) will decrease coverage by 0.42%.
The diff coverage is 89.91%.

❗ Current head 7fcb53e differs from pull request most recent head 12e03be. Consider uploading reports for the commit 12e03be to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##           master     #518      +/-   ##
==========================================
- Coverage   87.93%   87.51%   -0.43%     
==========================================
  Files          80       86       +6     
  Lines        5306     5550     +244     
==========================================
+ Hits         4666     4857     +191     
- Misses        640      693      +53     
Impacted Files Coverage Δ
crates/abi/src/elements.rs 77.77% <ø> (-9.84%) ⬇️
crates/common/src/diagnostics.rs 77.41% <ø> (+9.23%) ⬆️
crates/driver/src/lib.rs 76.31% <ø> (+0.64%) ⬆️
crates/lowering/src/context.rs 91.30% <ø> (-8.70%) ⬇️
crates/lowering/src/lib.rs 100.00% <ø> (ø)
crates/lowering/src/mappers/contracts.rs 100.00% <ø> (ø)
crates/lowering/src/mappers/expressions.rs 100.00% <ø> (ø)
crates/lowering/src/mappers/functions.rs 98.49% <ø> (-0.68%) ⬇️
crates/lowering/src/mappers/module.rs 84.44% <ø> (+1.11%) ⬆️
crates/lowering/src/mappers/types.rs 100.00% <ø> (ø)
... and 88 more

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 bfeffe8...12e03be. Read the comment docs.

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 great! validate_arg_types and validate_arg_labels were only separate because I failed to think of a scheme like this. If you were so inclined (no pressure), you could combine the two so that we could not report a type error in the case that the label is incorrect (or maybe just when the labels are in the incorrect order).

@cburgdorf
Copy link
Collaborator Author

so that we could not report a type error in the case that the label is incorrect (or maybe just when the labels are in the incorrect order).

I think I don't follow.

This will only report label errors because the types are correct but mislabeled:

contract Foo:

    pub fn bar():
        self.baz(some=1, input=true)

    pub fn baz(input: u256, some: bool):
        pass

This will report label and type error because they are mislabled and in the wrong order:

contract Foo:

    pub fn bar():
        self.baz(some=true, input=1)

    pub fn baz(input: u256, some: bool):
        pass

Isn't that already what we want?

I'm going to merge this already but happy to follow up if I understand the intention of the proposed change. Or feel free to send a PR if it's less effort to explain it with the code directly :D

@cburgdorf cburgdorf merged commit a0821aa into ethereum:master Aug 20, 2021
@cburgdorf cburgdorf mentioned this pull request Aug 31, 2021
4 tasks
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.

3 participants