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

Warn if the same operand is on either side of the equality operator #4213

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

Ramkarthik
Copy link
Contributor

Closes #4205

Based on the conversation in the comments thread of the mentioned issue, I have implemented this for variables, field access, and literals. I'm not sure about module selects. @giacomocavalieri Could you please share an example for module select? (I'm still fairly new to Gleam)

Currently, it only checks for the same operand. In the case of literals, it doesn't warn if the check is 1 == 2 (which is always false). Do we want to handle those cases? If yes, I will likely have to rename the Warning::EqualityOnSameOperands variant and change the warning title. Any equality check for literals on both sides of the operator is always going to be the same result, so maybe we want to warn?

This PR also doesn't handle gt, gte, lt, and lte for Int and Float literals (based on the discussion in the issue thread).

If there's a better way to phrase the warning (title, hint, etc), please let me know.

Currently, the warning message template is:

warning: Same operand on either side of the equality operator
   ┌─ /code/gleam/gleam/test/equality/src/equality.gleam:10:8
   │
10 │   case name == name {
   │        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ This will always be true

Not related: It looks like the person who added this particular changelog entry missed adding their name/link - 65d7bb5

@Ramkarthik
Copy link
Contributor Author

My bad, I forgot to run rust fmt which is causing the ci/rustfmt check to fail. Since there will likely be suggested changes, I will combine this with the changes in the next commit.

@lpil
Copy link
Member

lpil commented Feb 1, 2025

Thank you! Please see my comment here #4205 (comment)

@lpil lpil marked this pull request as draft February 1, 2025 12:44
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.

Warn on useless comparisons
2 participants