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

Miscompilation of assignment #7170

Closed
AndreasLoow opened this issue Nov 24, 2024 · 2 comments · Fixed by #7176
Closed

Miscompilation of assignment #7170

AndreasLoow opened this issue Nov 24, 2024 · 2 comments · Fixed by #7176
Labels

Comments

@AndreasLoow
Copy link

Hi, the following code

{
let j = ref(0)

if Js.String.get("%", j.contents) == "%" {
 j := j.contents + 1
 j := j.contents + 1
} else {
 j := j.contents + 1
}

Console.log(j.contents)
}

is compiled into

var j = 0;

j = j + 1 | 0;

if ("%"[j] === "%") {
  j = j + 1 | 0;
}

console.log(j);

Note how the assignment of j is moved outside the if-expression despite the move changing the evaluation of the condition of the if-expression. The code without the surrounding { and } is compiled correctly.

@cristianoc
Copy link
Collaborator

Thanks for reporting.
This seems to be a long standing issue, that can be reproduced already on old versions of the compiler.
Seems to be triggered by not exporting variable j, by either putting the code into { ... } block or shadowing the variable.

@cristianoc cristianoc added the bug label Nov 24, 2024
@cristianoc
Copy link
Collaborator

This appears to be due to an optimisation for records with only one field, such as contents.
The optimisation should only affect the generated code, but in this case prematurely turning x.contents into x confuses the checker for side effects, which thinks there are none.

cristianoc added a commit that referenced this issue Nov 25, 2024
Fixes #7170

Assignment of a ref, or other record with a single mutable field, is compiled to simple variable assignment.
This triggers an incorrect optimisation that moves identical assignments in the true and false branches of a conditional above if they are identical and the guard has no side effects.

Added a check that if the assigments to be moved are to variable, that variable must not occur free in the guard of the conditional.
cristianoc added a commit that referenced this issue Nov 25, 2024
Fixes #7170

Assignment of a ref, or other record with a single mutable field, is compiled to simple variable assignment.
This triggers an incorrect optimisation that moves identical assignments in the true and false branches of a conditional above if they are identical and the guard has no side effects.

Added a check that if the assigments to be moved are to variable, that variable must not occur free in the guard of the conditional.
cristianoc added a commit that referenced this issue Nov 27, 2024
Fixes #7170

Assignment of a ref, or other record with a single mutable field, is compiled to simple variable assignment.
This triggers an incorrect optimisation that moves identical assignments in the true and false branches of a conditional above if they are identical and the guard has no side effects.

Added a check that if the assigments to be moved are to variable, that variable must not occur free in the guard of the conditional.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants