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

Add rule for rand(Bool) instead of rand() < 0.5 #14

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

Conversation

iuliadmtru
Copy link
Collaborator

@iuliadmtru
Copy link
Collaborator Author

The fix does not remove the assignment in cases like:

x = rand()
if x < 0.5
    ...
end

This fixes to:

x = rand()
if rand(Bool)
    ...
end

@imciner2
Copy link

imciner2 commented Sep 2, 2024

The fix does not remove the assignment in cases like:

x = rand()
if x < 0.5
    ...
end

This fixes to:

x = rand()
if rand(Bool)
    ...
end

This is different code semantically. If x was being used anywhere inside the if statement, then this can cause different execution behavior, since now the entrance to the if statement is guarded by a different random draw. The only way this transformation is valid is if x is not used anywhere else.

Take this section of code for instance:

x = rand()
if x < 0.5
    y = sqrt(0.5 - x)
end

Under this transformation, it is now possible to end up with a domain error in the square root when it wasn't before - because if this is transformed to

x = rand()
if rand(Bool)
    y = sqrt(0.5 - x)
end

then there is no longer a guarantee that x is less than 0.5 allowing 0.5 - x to be always positive. It could be that rand(Bool) returns true when x=rand() has returned 0.75 for instance.

@iuliadmtru
Copy link
Collaborator Author

I hadn't thought of that! I would say then either just remove the autofix, or remove altogether the pattern that checks for assigning rand() to a variable and using that variable (and might as well keep the autofix in that case). Not sure which is better.

@iuliadmtru
Copy link
Collaborator Author

Other ideas:

  • keep this as a partial fix, needing checking by whoever runs the runs (not nice)
  • write two separate rules, one for rand() < 0.5 with autofix, one for x = rand(); x < 0.5 without autofix

@aviks
Copy link
Member

aviks commented Sep 3, 2024

write two separate rules, one for rand() < 0.5 with autofix, one for x = rand(); x < 0.5 without autofix

maybe this is the best way forward?

@iuliadmtru
Copy link
Collaborator Author

I split it in two rules. The test files are the same, only the rule names differ (and the tests they pass/fail for of course). The name for the second rule is not very inspired...

Copy link
Member

@aviks aviks 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. Can we add a description of some sort in the rule definition files so that I'm not confused when I see this in a few months?

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