Rust: SSA additions

[paldepind]

This PR makes two small improvements to the SSA:

• Mutable variables that are captured are now supported. Immutable variables where already supported. I don't think allowing mutable variables as well pose much of a problem, as they don't introduce the same issues that mutable borrows do.

  The only change I made to support this was to insert pseudo reads upon exit from a closure where captured variables are written to, which I observed was done in the Ruby SSA implementation. There might be other necessary changes that I've missed, so please double check 🙏

• Mutable variables are now only excluded from SSA when they are mutably borrowed (&mut) whereas before merely immutably borrowing them (&) excluded them. I don't think immutable borrows pose any problems for the SSA construction.

[hvitved]

Great to have the SSA analysis apply to more cases!

We need a bit more work, though, to make the analysis sound for captured mutable variables.

[hvitved]

This line illustrates the problem with allowing for captured mutable variables: The test snippet is

    let mut y = 2; // y [line 418]
    // Captures mutable value by mutable reference
    let mut closure2 = || {
        y = 3; // $ write_access=y
    };
    closure2(); // $ read_access=closure2
    print_i64(y); // $ read_access=y [line 424]

It is not true that the value written at line 418 is the one read at line 424, because the value is updated in the call to closure2.

One way that we can fix it is to say that mutable captured variables may be read/written via any call, by adding uncertain pseudo reads and writes at all call nodes. In Ruby this is done in the predicates
capturedCallRead and capturedCallWrite, respectively.

[paldepind]

That makes sense and I did not think of that. I'll look at the Ruby approach and try an adapt it.

[hvitved]

Nit: getACapturedVariableAccess.

[hvitved]

Nit: I prefer va = any(RefExpr re | re.isMut()).getExpr()

[paldepind]

Thanks. That is definitely nicer.

[paldepind]

I've updated the PR to address comment and to handle captured variables in the same way as in Ruby. This fixes the error that was previously pointed out.

I think the way Ruby detects when a call can write/read might have a small soundness problem. Consider this example:

fn mutable_capture_and_write_in_a_sibling_closure() {
    let mut y = 2; // y
    let mut closure1 = || { y = 3; };
    let mut closure2 = || {
        y = 2; // (1): SSA definition for `y`
        closure1(); // no SSA definition for `y` here
        print_i64(y); // (2): reads from (1) which is not correct
    };
    closure2();
    print_i64(y);
}

Here the read at (2) reads what was written by closure1 on the line above, but the SSA analysis doesn't see that closure1 can write to y. This is because hasVariableReadWithCapturedWrite only holds for calls when there is a future read and a captured write in a nested CFG scope of the read's CFG scope. In the example the read is at (2) but the write is in closure1 which not nested inside (2)s CFG scope.

The above example doesn't actually compile due to Rust's borrow checker (which I didn't realize at first). When one closure (closure1 in this case) has mutably borrowed a variable it's not possible to use the variable in any other closure. I haven't been able to reproduce the issue in code that actually compiles. But I think it should be possible to port the example to Ruby and see the issue there.

To fix the above and improve precision, we could say that a call might write to a variable v if

1. there exists a closure cl that captures and writes to v
2. there is path from cl to the call through the CFG and through nested CFG scopes.

This should fix the above problem and also make the check more control flow sensitive in examples such as the following:

fn mutable_capture_further_down_in_the_cfg() {
    let mut y = 2;
    print_i64(y); // (1): This call can not write to `y` as the capturing closure is not yet in scope
    let mut closure = || { y = 3; // $ write_access=y };
    closure();
    print_i64(y);
}

In this example the current analysis will conclude that the call at (1) can write to y even though closure is not yet declared. I'm not sure how much value there is in doing that, but I don't think it would be very hard. I could try it out in a follow up PR.

[hvitved]

A few minor comments, otherwise LGTM. We should also do a DCA run before merging.

[hvitved]

Should be getEnclosingCallable+.

[hvitved]

Same

[hvitved]

There should be no need to cache this.

[paldepind]

Should I just remove the cached annotation or should I also move it out of the Cached module?

[paldepind]

The Ruby equivalent is also cached. Is there something different about that one that causes it to need caching?

[hvitved]

It needs to be moved out, otherwise the compiler will complain (all public declarations in a cached module must be marked as cached).

The reason why it is cached in Ruby is that it is referenced in a user-visible class.

[paldepind]

I see, thanks. I've addressed the comments :)

[hvitved]

LGTM. Let's wait for DCA (I have started a run) before merging.

[paldepind]

DCA looks fine to me.

[paldepind]

It's not very important right now, but I wonder what you think about #17786 (comment) @hvitved?

[hvitved]

It's not very important right now, but I wonder what you think about #17786 (comment) @hvitved?

I think you are right that it may be unsound, but it is probably not something that is likely to happen often in practice.