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

Mutable variables - let 1 become 0 #6873

Closed
michalmuskala opened this issue Feb 16, 2023 · 4 comments · Fixed by #6877
Closed

Mutable variables - let 1 become 0 #6873

michalmuskala opened this issue Feb 16, 2023 · 4 comments · Fixed by #6877
Assignees
Labels
bug Issue is reported as a bug team:VM Assigned to OTP team VM

Comments

@michalmuskala
Copy link
Contributor

michalmuskala commented Feb 16, 2023

Describe the bug
In some specific cases pattern matching doesn't do what it's supposed to - instead it mutates variables.

To Reproduce
Given the following snippet:

-module(test).

-export([test/0]).

test() ->
    Zero = 0,
    One = 1,

    Result = One = Zero,

    io:format("~p ~p~n", [Zero, One]),
    Result.

Compiling the test module and running test:test() from the shell produces:

1> test:test().
0 0
0

Expected behavior
The code should crash with match error, and variables should remain immutable.

Affected versions
Tested with the latest maint-25 and maint-24 branches. This correctly fails on current master, even emitting a warning during compilation with erlc.

Additional context
Originally discovered by @jcpetruzza

@michalmuskala michalmuskala added the bug Issue is reported as a bug label Feb 16, 2023
@bjorng bjorng self-assigned this Feb 16, 2023
@bjorng bjorng added the team:VM Assigned to OTP team VM label Feb 16, 2023
@michalmuskala
Copy link
Contributor Author

michalmuskala commented Feb 16, 2023

This also incorrectly succeeds and rebinds a variable with other expressions in the match tail, not just plain variables, e.g.

test2() ->
    Zero = 0,

    Result = Zero = self(),
    io:format("~p~n", [Zero]),
    Result.
1> test:test2().
<0.81.0>
<0.81.0>

@bjorng
Copy link
Contributor

bjorng commented Feb 16, 2023

It is also incorrect in OTP 23 and probably in many earlier releases.

It seems that the bug was inadvertently corrected in #6415. My current plan is to backport part of that PR to OTP 23 through 25 in order to fix this bug.

@michalmuskala
Copy link
Contributor Author

michalmuskala commented Feb 16, 2023

Looks like the comment I was replying to is gone now

fun arguments allow shadowing, if you mean code similar to the example below, it's working as documented.

test3() ->
    Zero = 0,
    One = 1,

    F = fun(One) -> io:format("~p~n", [One]) end,
    F(Zero).

The compiler also issues a warning about this:

test.erl:23:13: Warning: variable 'One' shadowed in 'fun'
%   23|     F = fun(One) -> io:format("~p~n", [One]) end,
%     |             ^

bjorng added a commit to bjorng/otp that referenced this issue Feb 16, 2023
The compiler would generate incorrect code for the
following type of expression:

    Pattern = BoundVar1 = . . . = BoundVarN = Expression

An exception should be raised if any of the bound variables have
different values than `Expression`. The compiler would generate code
that would cause the bound variables to be bound to the value of
`Expression` whether the value matched or not.

For example:

    t() ->
        Zero = 0,
        One = 1,
        Result = One = Zero,
        {Result,One,Zero}.

There should be a `badmatch` exception, but instead, the `t/0`
function returned `{0,0,0}`.

Closes erlang#6873
@bjorng bjorng linked a pull request Feb 16, 2023 that will close this issue
bjorng added a commit to bjorng/otp that referenced this issue Feb 17, 2023
The compiler would generate incorrect code for the
following type of expression:

    Pattern = BoundVar1 = . . . = BoundVarN = Expression

An exception should be raised if any of the bound variables have
different values than `Expression`. The compiler would generate code
that would cause the bound variables to be bound to the value of
`Expression` whether the value matched or not.

For example:

    t() ->
        Zero = 0,
        One = 1,
        Result = One = Zero,
        {Result,One,Zero}.

There should be a `badmatch` exception, but instead, the `t/0`
function returned `{0,0,0}`.

Closes erlang#6873
bjorng added a commit that referenced this issue Feb 17, 2023
…70' into maint

* bjorn/compiler/fix-mutable-variables/24/GH-6873/OTP-18470:
  Eliminate "mutable variables"
@bjorng bjorng closed this as completed in bff8f7e Feb 17, 2023
@michalmuskala
Copy link
Contributor Author

Thanks for fixing this quickly!

bjorng added a commit that referenced this issue Feb 22, 2023
…70' into maint

* bjorn/compiler/fix-mutable-variables/23/GH-6873/OTP-18470:
  Eliminate "mutable variables"
IngelaAndin pushed a commit that referenced this issue Feb 23, 2023
…70' into maint-24

* bjorn/compiler/fix-mutable-variables/24/GH-6873/OTP-18470:
  Eliminate "mutable variables"
rickard-green pushed a commit that referenced this issue Jun 8, 2023
…70' into maint-23

* bjorn/compiler/fix-mutable-variables/23/GH-6873/OTP-18470:
  Eliminate "mutable variables"
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Issue is reported as a bug team:VM Assigned to OTP team VM
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants