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

fix: removed equality method from Literal class #57

Merged
merged 1 commit into from
Aug 17, 2023
Merged

Conversation

kevinclancy
Copy link

@kevinclancy kevinclancy commented Aug 15, 2023

Notes

Slither's Literal class implements an __eq__ method but not a __hash__ method, making it unhashable. The eq implementation was introduced in this PR so that two distinct occurrences of int[n] are treated as equal types, where n is some literal integer; that was a flawed solution, because it doesn't take into account cases where constants and arithmetic expressions occur in the type expression, e.g. int[CONST_1 + CONST_2]

Another problem with giving Literal a custom equality operator is that all other Expression subclasses use the built-in equality implementation, which is reference equality. It is confusing to have different Expression subclasses using different notions of equality.

This PR makes Literal hashable by moving the literal equality hack from the Literal class into the ArrayType class.

Testing

  • Copy this under utilities/slither in a local copy of the slither-task repo.
  • Run make test and verify that all tests succeed
  • Run ./evaluate.sh run 100 and verify that all projects succeed
  • Print the IR for the code from this issue and verify that a LIBRARY_CALL operation is generated.

Related Issue

https://github.com/CertiKProject/slither-task/issues/674

Additional Comments

I created an issue about how giving Literal an equality method didn't fix the issue it intended to solve:
https://github.com/CertiKProject/slither-task/issues/689
This issue remains unresolved.

@kevinclancy kevinclancy requested a review from duckki August 15, 2023 22:13
Copy link

@duckki duckki left a comment

Choose a reason for hiding this comment

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

Have a question. Otherwise, it makes sense.

slither/core/expressions/literal.py Show resolved Hide resolved
Copy link

@duckki duckki 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.

@duckki
Copy link

duckki commented Aug 17, 2023

I confirmed https://github.com/CertiKProject/slither-task/pull/663 is no longer crashing.

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.

2 participants