Change mutation of non-mutable locals from an error to a lint

[em-tg]

• implement proof of concept
• ensure ui tests pass
• submit pull request to get others' thoughts before continuing
• ensure remaining tests pass
• add more automated tests
• adjust the wording in the lint error messages
• update documentation
• decide on a name for the lint(s) (currently "mut_non_mut")
• make the lint deny-by-default (currently warn-by-default for testing purposes)
• hide the change behind a feature flag

Overview

Hello, all. This idea has been discussed previously:

• 2014 Blog post
• 2021 IRLO thread
• Zulip thread

As a quick summary of the above: the rust mut keyword has two meanings currently:

• When applied to a reference, it means the reference is not aliased
• When applied to a local binding, it means that the binding is allowed be re-assigned or borrowed using &mut

The first of these is fundamental to Rust's safety guarantees and enables some optimizations. The second, however, is more of a lint: the error message may help catch mistakes, but ignoring it doesn't violate type safety or subvert any of Rust's global correctness guarantees.

Reasons for making the lint configurable instead of a hard error:

• Reduced friction while making changes
• Symmetry/consistency with the "unused_mut" lint
• Potentially improved teachability

Reasons for maintaining the status quo:

• Consistency with most other languages that have the concept of immutability
• Making the lint configurable opens the door for public projects written in rust to refuse to use the mut annotation for locals
• Prevent some style debates in projects which use rust

I don't necessarily expect this PR to get merged, but:

• I think it's valuable to have a concrete implementation available for people to play with while considering an idea.
• I implemented this change mostly for fun in my free time, and I think submitting it as a pull request and being told "no" is better than never doing anything with it.

Implementation Details

• error paths in borrow checking related to mutation of locals have been replaced with success paths that return some extra information
• in add_used_mut, the extra information is used to create and buffer non-fatal diagnostics
• When emitting diagnostics, non-fatal diagnostics are converted into "mut_non_mut" lints
• Relevant parts of the diagnostic-generating code in rustc_borrowck are now generic over the EmissionGuarantee of the returned Diagnostic

Implementation Concerns

• I know very little about rustc or rustc_borrowck. I think my changes don't introduce any holes, but I'm not very confident, especially around the code related to closures/upvars.
• Converting a borrowck-constructed Diag to a LintDiagnostic was the least invasive way I could think to make this change, but it's a bit hacky.
• Lint diagnostics use the same deduplication code as non-lint diagnostics. This could result in a legitimate error being suppressed by the presence of a lint, although things seem to be ok just based on the ui tests.

[rustbot]

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @SparrowLii (or someone else) some time within the next two weeks.

Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (S-waiting-on-review and S-waiting-on-author) stays updated, invoking these commands when appropriate:

• @rustbot author: the review is finished, PR author should check the comments and take action accordingly
• @rustbot review: the author is ready for a review, this PR will be queued again in the reviewer's queue

[SparrowLii]

Thanks for the work!

I think this change should at least be submitted to a formal MCP or a language team fcp decision?

[SparrowLii]

@rust-lang/compiler

[compiler-errors]

Yeah this definitely needs an RFC.

[compiler-errors]

I'd frankly opt to close this PR at this point. While it's definitely cool that you've demonstrated that this is within the realm of possibility of implementation, it's probably not actionable to keep this PR open indefinitely (and accumulating merge conflicts) while the underlying desire for this feature is still up in the air.

Both the zulip thread and the i-rl-o thread you linked failed to converged on any consensus towards this being experimented with, and the latter was actually closed by a moderator because of how much back and forth it led to. I expect an RFC to take some serious time to settle on a decision as well.

I think it's most appropriate to reopen this PR when there's actually team consensus that this should be pushed forward.

[dev-ardi]

However it's not bad to have this as a future reference for an implementation if the team decides to go forward.

[em-tg]

RFC created