Add a warning when a variable is reassigned to itself

[WardBrian]

Closes #1357.

This is marked as a draft just because it should not be merged before the 2.33.1 patch release.

Submission Checklist

• [x] Run unit tests
• Documentation
  • [ ] If a user-facing facing change was made, the documentation PR is here:
  • [x] OR, no user-facing changes were made

Release notes

The Stan compiler will now warn you when you reassign a variable to itself.

Copyright and Licensing

By submitting this pull request, the copyright holder is agreeing to license the submitted work under the BSD 3-clause license (https://opensource.org/licenses/BSD-3-Clause)

[codecov[bot]]

Codecov Report

Merging #1358 (1d278cf) into master (934496d) will increase coverage by 0.03%. Report is 2 commits behind head on master. The diff coverage is 97.22%.

@@            Coverage Diff             @@
##           master    #1358      +/-   ##
==========================================
+ Coverage   89.41%   89.44%   +0.03%     
==========================================
  Files          65       65              
  Lines       10615    10649      +34     
==========================================
+ Hits         9491     9525      +34     
  Misses       1124     1124              

Files Changed	Coverage Δ
src/frontend/Ast.ml	56.41% <94.11%> (+3.41%)	:arrow_up:
src/frontend/Typechecker.ml	91.11% <100.00%> (+0.11%)	:arrow_up:
src/stan_math_backend/Lower_stmt.ml	95.91% <100.00%> (+0.08%)	:arrow_up:

[nhuurre]

Some additional test cases.

model {
  real x = 1.0;
  x = (x); // should warn
  x = -x;
  x = (x > 0) ? x : 0.0;
  tuple(real, real) tup = (1.0, 2.0);
  tup.1 = tup.1; // what about this?
}

[WardBrian]

@nhuurre I just made some changes that ended up adding a warning for this line in deprecated.stan:

  idxs[1][:] = idxs[1][:];

To be perfectly honest I've never quite wrapped my head around the semantic problem which lead to this being deprecated, but am I correct that this is a case where the lvalue and rvalue are actually not the same?

[WardBrian]

On the other hand, nested multi-indexing like that is disallowed as of the most recent release, so maybe it's not a big deal if that warning is slightly wrong?

[nhuurre]

The specific case

  idxs[1][:] = idxs[1][:];

is not deprecated, and is an example where the lhs and rhs are equal. The deprecated construct is

  idxs[:][1] = idxs[:,1];

and the broken variant where lhs and rhs are secretly different is

  idxs[:][1] = idxs[:][1];

[WardBrian]

@nhuurre mind giving this a review now that 2.33.1 was released?

[nhuurre]

I note that the original bug report in #1357 was a C++ warning. As far as I understand, this PR makes that warning redundant by adding a Stan warning but does not actually silence the C++ warning. I think we should also change the C++ backend to

go one step further and just remove such a statement, since it is semantically meaningless in Stan.

before closing the issue.

[WardBrian]

I think most of the value comes from letting the user know "hey, this is actually your fault". The original issue was opened because the user thought it was something being generated by stanc, not something in their own code.

Removing those lines entirely masks the C++ warning but also isn't something we can do in the typechecker due to our sanity check that (untype (typecheck prog)) == prog, so it seems more like something we should do in the dead code pass of optimization (which seems to already remove some of these lines)

[nhuurre]

The user's code in the original issue would have worked correctly but had a no-op statement that C++ devs have decided is probably an error. In my opinion the warning had negative value because all it did is tell user to fix what isn't broken. Regardless, whether no-op assigns are considered errors in Stan shouldn't be for C++ devs to decide. stanc should never emit C++ code that causes further warnings or errors.

[WardBrian]

I think the warning has independent value. The fact that this statement is a no-op means it is very likely a programmer error, e.g. you meant to do some_variable_a = some_variable_b but accidentally wrote some_variable_a = some_variable_a. This will always trivially typecheck, so spotting that you make this typo is not easy!

Even if we make it so the C++ compiler never produces this warning, I think it is still worth alerting to it on the Stan level.

And for decls, I think I would actually be in favor of having any use of the variable being declared on the right hand side be a hard error, but that's obviously a pretty major breaking change (even if the code we were breaking is almost certainly wrong)

[nhuurre]

The fact that this statement is a no-op means it is very likely a programmer error

You're probably right about that, even though it was a false positive in the reported case.

Even if we make it so the C++ compiler never produces this warning, I think it is still worth alerting to it on the Stan level.

And I think that even if we alert to it on the Stan level, it is still worth making it so the C++ compiler never produces this warning.

I believe adding

  | Assignment ((LVariable x, []), _, {pattern= Var y;_}) when String.equal x y -> [] (* self-assign is a no-op *)

to the big match in Stan_math_backend.Lower_stmt.lower_statement should be enough to silence the C++ compiler.