Skip to content

Comments

#2586 use enum to track variable location instead of string#2682

Open
nsiregar wants to merge 10 commits intocrytic:masterfrom
nsiregar:2586-use-enum-track-var-location
Open

#2586 use enum to track variable location instead of string#2682
nsiregar wants to merge 10 commits intocrytic:masterfrom
nsiregar:2586-use-enum-track-var-location

Conversation

@nsiregar
Copy link
Contributor

@nsiregar nsiregar commented Mar 4, 2025

No description provided.

Copy link
Collaborator

@smonicas smonicas left a comment

Choose a reason for hiding this comment

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

The way it's done at the moment the location value wouild still be the string that's for example why you have to use the Enum value in the comparisons. You should make the location the actual Enum when it's set which means going where set_location is called and passing the enum's variant as argument. For example myvar.set_location(VariableLocation("memory")) would set myvar's location to VariableLocation.MEMORY not "memory" the string. After that you would need to check all the places where location is used and if it's used in a comparison (like myvar.location == "memory") you need to change it to compare it against the enum variant (myvar.location == VariableLocation.MEMORY). This for both LocalVariable and StateVariable.

@nsiregar nsiregar force-pushed the 2586-use-enum-track-var-location branch from 9ecc744 to cc754af Compare May 16, 2025 15:01
@nsiregar nsiregar force-pushed the 2586-use-enum-track-var-location branch from cc754af to 87f2e28 Compare May 16, 2025 15:07
@nsiregar nsiregar requested a review from smonicas May 17, 2025 01:23
@dguido dguido changed the base branch from dev to master January 15, 2026 19:06
dguido and others added 2 commits January 15, 2026 15:54
# Conflicts:
#	slither/core/variables/variable.py
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
@dguido
Copy link
Member

dguido commented Jan 16, 2026

Code review

Found 2 issues:

  1. Missing enum conversion in local_variable_init_from_tuple.py. When "storageLocation" in attributes, the code still passes the raw string attributes["storageLocation"] to set_location() instead of wrapping it in VariableLocation(). The else branches were correctly updated, but the if branch was missed.

if "storageLocation" in attributes:
location = attributes["storageLocation"]
self.underlying_variable.set_location(location)

Fix: Change to location = VariableLocation(attributes["storageLocation"])

  1. Missing enum conversion in state_variable.py. Same issue - the if "storageLocation" in attributes: branch passes raw string instead of VariableLocation(attributes["storageLocation"]). The else branch was correctly updated to use VariableLocation.DEFAULT.

if "storageLocation" in attributes:
self.underlying_variable.set_location(attributes["storageLocation"])

Fix: Change to self.underlying_variable.set_location(VariableLocation(attributes["storageLocation"]))

Note: The correct pattern is shown in local_variable.py which properly wraps the string in VariableLocation(). These type mismatches would cause runtime issues when parsing contracts with explicit storage location keywords.

🤖 Generated with Claude Code

- If this code review was useful, please react with 👍. Otherwise, react with 👎.

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