Skip to content
This repository has been archived by the owner on Feb 26, 2024. It is now read-only.

Fix modifier argument detection by factoring out no-op handling #6168

Merged
merged 1 commit into from
Aug 21, 2023

Conversation

haltman-at
Copy link
Contributor

PR description

This PR fixes an old bug regarding detection of arguments to modifiers, and factors some code in the process.

Both mapping key handling and modifier argument detection can require looking inside nodes that aren't sourcemapped because they're no-ops on the EVM level and so don't correspond to any instruction. (Certain type conversions; unary plus in Solidity prior to 0.5.0; and wrap/unwrap for UDVTs.) When dealing with these, we often need to peel away the node representing the no-op, to find the node inside representing an actual operation.

For mapping-key handling, this code has been kept up to date. But for modifier argument detection, which had the logic separate, it was not. I didn't notice this until going over the code with @cds-amal this past Friday.

This PR factors out the logic into a function in helpers, unifying the two places it's used. So now the up-to-date logic from mapping key handling is applied to modifier argument detection as well. And if there's ever any further need to update this, now the code will all be in one place so it won't desync like that again!

I used some fairly C-style loops in this one, hope you don't mind, it seemed the easiest way. :P

Testing instructions

I didn't bother adding any tests for this one, I just did some brief manual testing. I tested using the modifier-test-8 project in the solidity-test-cases repo.

Copy link
Member

@cds-amal cds-amal 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 to me!

@haltman-at haltman-at merged commit e10de93 into develop Aug 21, 2023
10 checks passed
@haltman-at haltman-at deleted the factor-nothing branch August 21, 2023 23:18
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants