-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Convert "reassignment of immutable local" and "mutable borrow of immutable local" from a hard error to a deny-by-default lint #3742
base: master
Are you sure you want to change the base?
Conversation
Pedagogically, this strikes me as very useful change, as it can be used to highlight the difference between what's an essential aspect of Rust's ownership system, and what's (effectively) a lint. I expect this RFC to cause a lot of spirited discussion, but it probably shouldn't. Educators will find this useful, and it's probably the case that only a minority of Rustaceans will opt-in to |
Even just having the option to disable this lint means that whenever I would look at code written by someone else, I would have to take into account that a non-mut variable may actually get mutated. Also
Maybe we should rather have a cli flag to turn this error into a warning instead? That way arbitrary crates can't disable the error, only the end user, and it would be at least a warning and not be possible to disable entirely (unlike lints). This flag could have more teaching related relaxations behind it. Maybe even disabling the borrow checker? (probably doing should at least force the executable to print a big warning at startup time though and maybe even every time a function where borrowck doesn't pass gets executed.) That would probably be even more contentious than just disabling the mut error though. |
As the RFC observes: This is already the case. The absence of But, stepping back: I don't think accepting this RFC creates any great drama as a third party contributor. Consider, for instance, that |
I don't consider the "move loophole" to be a loophole. It still requires explicitly writing |
I can see why not allowing mutation of non-mutable locals feels more like a lint, as it enforces a particular code style. However, I'm not sure if the benefits of allowing this rule to be disabled outweigh the benefits in code readability. Especially in IDEs jumping to a variable definition is common and you usually get type hints for inferred types as well. With these, it's possible to see if the variable is mutable itself or has top-level interior mutability. However, if allowing mutation of non-mutable locals would become possible, reading code with it would require checking every use of the variable. This might lead to much of the ecosystem forbidding the lint e.g. in workspace lint configs and for some tools to flag crates which don't. Especially if this feature would be motivated for quick code refactoring and beginners, it might be a lot easier if the lint could only be allowed/warned on a per-function scope. This would provide a much more local warning sign that code will be more difficult to read |
I don't believe that it makes code more difficult to read to begin with. I leave unused mut all over all of my projects and no one has once ever said "i can't tell what you're mutating or not!". I just add I'd rather rust not ever bug me at all about this. I'd definitely put an |
How about teaching rust-analyzer to automatically add mut to variable definitions if it sees that you write code that mutates a variable? |
like an "on_file_saved: add mut if it's missing" trigger thing? i'd be cool with that as an alternative solution |
I certianly think |
let mut not_mutable = not_mutable; | ||
not_mutable.push(4); | ||
``` | ||
- But that loophole _doesn't_ work for `move` closures, which ostensibly do the same thing: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm personally not convinced by this argument: The snipped below doesn't include the mut
keyword anywhere, so why would it enable mutation? (And it compiles just fine if you add let mut not_mutable = not_mutable;
to the closure body).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I got a bit too fancy with the IIFE. If you assign to a temporary you can see the mut
:
let not_mutable = vec![1, 2, 3];
let mut f = move || { // `f` is `mut` and owns `not_mutable` after the move
not_mutable.push(4); // still rejected by the compiler
};
f();
to compile will continue to do so after the change. | ||
|
||
One interesting consequence of this change is that it allows mutation in places where there | ||
previously was no syntax to allow it, for example in `ref` bindings: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Syntax for this exists behind #![feature(mut_ref)]
: mut ref mut
and mut ref
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm, ok. I actually don't know of any other examples off the top of my head where mut
syntax is missing, so maybe the whole "syntax doesn't exist" point is actually moot. I've added a note.
} | ||
``` | ||
|
||
# Reference-level explanation |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would be good to more concretely mention here or elsewhere all the places where mut
can now be omitted.
All the examples in the RFC are for simple let
bindings, but there are a more possible locations:
- Irrefutable
let
bindings:let (a, b) = gen_tuple()
- Function parameters
match
andif let
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know if being exhaustive would be worthwhile, but I added the examples you listed.
- [Zulip thread](https://rust-lang.zulipchat.com/#narrow/stream/213817-t-lang/topic/Idea.3A.20Downgrade.20.22immutable.20variable.E2.80.9C.20errors.20to.20lints) | ||
|
||
One benefit of this change is reduced friction: adding and removing of the need for `mut` on a given local | ||
can happen frequently when refactoring, similar to how variables and functions may change from used to unused |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd be curios if there's any data backing this statement. It's been a while since I've written significant amounts of Rust, but I don't think marking bindings as mut
was ever an issue or particularly disruptive.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've not seen any hard data, but some people consider it a major inconvenience while others swear it's never an issue. I think it depends on:
- The way you think through problems (writing a bunch of code first then asking the compiler if it's correct vs "conversing" with the compiler while writing)
- Tools in use (LSP/IDE vs old-fashioned edit/compile/debug loop)
- Overall coding style (Static single-assignment and functional vs mutation-heavy and imperative)
- Possibly also project type (library, cli, gui, server, or game)
This RFC proposes that the re-assignment or creation of a `&mut` to a local variable not | ||
marked with `mut` be changed from a hard error to a deny-by-default lint diagnostic (called | ||
`mut_non_mut` for the sake of discussion). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's an error and what's a lint is a mixture of implementation detail and user configuration. Abstracting those details away, what this RFC proposes is relaxing a forbid-by-default diagnostic to deny-by-default.
This RFC proposes that the re-assignment or creation of a `&mut` to a local variable not | |
marked with `mut` be changed from a hard error to a deny-by-default lint diagnostic (called | |
`mut_non_mut` for the sake of discussion). | |
This RFC proposes that the lint against the reassignment creation of a `&mut` to a local variable not | |
marked with `mut` be changed from a forbid-by-default diagnostic to a deny-by-default diagnostic (called | |
`mut_non_mut` for the sake of discussion). | |
This will allow users, if they wish, to `allow(mut_non_mut)` or `warn(mut_non_mut)`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're right, but I think the existing language is more precise. Lints in rustc are defined as configurable diagnostics, so a change from an unconfigurable diagnostic to a configurable one is a change from a non-lint to a lint. I like that final sentence, though, so I added it in.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not more precise; it contains a category error. Whether or not a diagnostic is a warning or error is a mix of compiler and user configuration. It's a category error to say "we're turning an error into a lint", since lints can be (and often are) errors. A user deny
ing or forbid
'ing a lint does not make the lint cease to be a lint.
Moreover, since RFCs are for user-facing changes, they should use user-facing language. It doesn't matter to a user how the compiler technically implements a diagnostic.
(An MCP, on the other hand, should reference the implementation details of a diagnostic. It'd be totally appropriate to file an MCP proposing that the E0596
diagnostic be moved from rustc_borrowck
to rustc_errors
as a forbid-by-default lint, since that's not a user-facing change.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a category error to say "we're turning an error into a lint", since
lints can be (and often are) errors
Perhaps my use of the term "hard error" is too ambiguous? I was using "hard" to
mean "hard-coded"/"non-configurable," but I can see how it might be unclear.
Moreover, since RFCs are for user-facing changes, they should use user-facing
language. It doesn't matter to a user how the compiler technically implements
a diagnostic.
I believe the usage of the term "lint" in user-facing rust documentation is consistent with
the "configurable diagnostic" definition. Anecdotally, I've seen the term used that
way in various online rust spaces as well.
The actual compiler output admittedly doesn't make a strong distinction.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think it is accurate to call this a forbid-by-default diagnostic. Only lints have an "X-by-default" property, but this is clearly not a lint. Also, lints are subject to --cap-lints
, even when they are forbidden, so it is even directly observable that the mut
requirement is not a forbid-by-default anything.
The typical term we use for these is "hard error" (which is of the same category as "lint"), so it is completely in line with standard long-standing rustc terminology to say that something is converted from a hard error to an X-by-default lint.
What's an error and what's a lint is a mixture of implementation detail and user configuration.
What's a hard error and what's a lint is very directly observable by the user, and is not subject to user configuration.
EDIT: Sorry I saw too late that the thread is locked.
- But `mut` doesn't just mean "exclusive," either: local variables are _already_ exclusive | ||
by nature, so wouldn't need `mut` under that definition. | ||
- If you can't get a `&mut` to a local declared without `mut`, how does `drop` do it? | ||
- There is also the "move loophole," where moving out of a binding lets you mutate it: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another 'loophole': macro invocations: There is no way, without inspecting the definition of mystery_macro
, to tell what this program will print:
let foo = 42;
mystery_macro!(foo);
println!("{foo}");
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting... I'm not sure if macros count, though, since arguably the whole point of macros is to let you cheat the system a bit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, emphatically: macros count. The point you're trying to make here is that the presence of the mut
modifier on a variable does not mean that its data is immutable, nor does it mean that the name itself is free from apparent mutation. You have the whole Rust Programming Language at your disposal to make this point, without exceptions.
A strong RFC pulls out all the stops in motivating itself in its Motivation section, and offers a candid accounting of consequences in the Drawbacks section. Don't undercut the RFC in the Motivation section!
if condition { | ||
value // rejected due to missing semicolon: probably not worth changing. | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This isn't a candidate for "lintification". It's a type error. You can verify for yourself that Rust accepts this:
if true {
()
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, that example is a bit wonky. A better one would be this:
{
value
}
let whatever = 12;
Adding a semicolon after value
fixes the error, but so does adding a semicolon after the block, where we expect the semicolon to be implicit.
I've removed the example anyway, per your other recommendation.
```rs | ||
fn maybe_thing(i: i32) -> Option<i32> { | ||
i // rejected due to missing `Some`: Would require code generation to convert to a lint. | ||
} | ||
``` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is also a type error.
I think you should move away from using the term "lintification". A lint is a well-formedness check on a program that's non-essential for soundness. mut_non_mut
— even today — falls under this definition. This RFC only proposes that it be moved from forbid-by-default (which cannot be overridden), to deny-by-default (which can be overridden).
These "Future Possibilities" are an entirely different thing, proposing type-directed syntactic sugar. This RFC is controversial enough without these (im)possibilities. I suggest you remove them altogether.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're right, the examples were a distraction, so I removed them. That said, they were based on points raised in the IRLO thread, which might provide better context/explanation than I did.
- Does `mut` mean mutable? Surely not, since there exists interior mutability. | ||
- But `mut` doesn't just mean "exclusive," either: local variables are _already_ exclusive |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given your keen points that "mut" is overloaded, perhaps the lint name "mut_non_mut" is a poor choice. Consider something like unmarked_variable_mutation
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added your suggestion to the list of possible lint name options.
and back. In this context, a fatal error for `mut_non_mut` can be a flow interruption. Yes, adding | ||
`mut` in all the right places is ideally something you should do _eventually_, just as you should | ||
delete any unused variables/functions, but it's not something that needs to prevent you from running | ||
the program. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The "you should do this eventually" prescriptivism is unnecessary here. As @Lokathor points out, a subset of Rust users (including myself) find this diagnostic to be pure annoyance, both bloating run-edit cycles and adding line noise. Given that this RFC will allow such users to opt-out of this annoyance, there's no need to alienate this body of potential support by saying their preference is not ideal.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tweaked the phrasing a bit. I'm still not super happy with the sentence, but it's definitely better without the "you should"s in it.
Also: add another possible name for the lint
I like being reminded of my intention of making a variable a |
This comment was marked as duplicate.
This comment was marked as duplicate.
This comment was marked as duplicate.
This comment was marked as duplicate.
This comment was marked as duplicate.
This comment was marked as duplicate.
Reading the 2014 blog post linked, this proposal is distinct in an important way:
That proposal meaningfully simplifies the language, reducing complexity for everyone. This proposal makes the language less consistent, and leaves us with a largely vestigial, optional |
request that developers use a particular coding style, and it's not clear whether that's | ||
warranted in this case. | ||
|
||
# Rationale and alternatives |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This section should cover why this proposal is better than simply removing the use of mut
for local variables over an edition boundary.
This comment was marked as duplicate.
This comment was marked as duplicate.
If you want to move this to a lint to allow an incremental migration to "no mut for locals", then a corresponding allow-by-default lint for "mut on locals" should be added, allowing code bases to fully migrate to a post-mut world. Without that, code bases will be left in a messy half-state. |
I think there's value in being able to read any variable declaration and know whether its stack memory can change. You can technically modify memory that it points to, but the direct memory is stable. I imagine there could even be features that cleverly take advantage of its global enforcement; perhaps a macro that messes with variable declarations. On the other hand, the keyword can be obnoxious in certain cases. e.g. in Bevy where you have to redundantly mark queries and their items as fn system(mut items: Query<&mut Item>) {
for mut item in &mut items {
item.x += 1;
}
} This is partially cause I'm not sure if this RFC is the best answer to that problem or not, though. Edit: Actually looking at the source code, |
"Eventually" is a very weak deadline, and I don't trust the median programmer not to kick the can down the road indefinitely if they can disable the "noisy lint" with a single line of code. As such I oppose this RFC. |
@Yokinman This is a common misconception. The use std::cell::Cell;
fn main() {
let foo = Cell::new(1);
foo.set(2);
assert_eq!(foo.get(), 2);
}
When I cite the pedagogical benefits of this RFC, this is the sort of misconception I think it would help educators dispel. |
Why don't we go a step further and make the lint warn-by-default? To me, that seems like the only way to get the actual workflow improvement. Before:
After:
The workflow only improves if the lint is a warning that doesn't prevent compilation. So, by making the lint deny-by-default, we would be asking Rust devs to tweak the lint configuration on every project to get the best workflow. In my opinion, having good defaults is one of Rust's strenghts. Remember that the slow debug-run cycle is one of the major complaints people have with Rust! And the difference between warn-by-default and deny-by-default is pretty much irrelevant for code that's older than five minutes. Rust doesn't have the problem of people ignoring hundreds of compiler warnings because we have good warnings. I'm also not very concerned about having to read other people's code. For the tiny fraction of people who will actually allow this lint, they will probably do so on a project-wide basis. So it's not like every function changes style and can trip you up. And to be honest, many Rust libraries are probably hard for me to read for a number of much more impactful factors: FFI, unsafe, procedural macros... If I had to be able to read and understand all Rust libraries I use, a missing In summary, I see the trade off like this:
|
While this RFC has good spirit, I don't think this is the right way to go about it- making |
This syntax conveys the intent of the author, and removing it removes that metadata. |
You are now back full circle to the C++ default that everything is mutable by default, but even worse since you have no way of communicating that something really shouldn't be mutable. This default is seen by many as the wrong default. C++ has a lot of tools that will warn that something could/should be made This change only risks harming correctness, increase the risk of proliferation of lower quality, more bug-prone code. And this default is by many considered wrong in C++. Rust learned from that, please don't undo that lesson. 100% against! |
Interesting, I did think the value of It would certainly be nice if mutable dereferencing didn't require the |
As a student I'd like to add that I do not see any pedagogical benefit to this. Coming from other languages with |
The friction is intentional. Code is read much more often than it is written, minimizing the effort when reading is a good thing. While there is an additional effort when people are first learning the language that is a very useful lesson to learn early in on. Having people consistently be aware of the ownership model helps make that effort easier and second nature. |
This is simply incorrect. In C++, a lack of I think the correct comparison would be a proposal to make the lint against non-snake-case identifiers a hard error that's impossible to allow. Would anyone be in favor of that? No, because it slows down the run-debug cycle unnecessarily. Although it is only a warning, the Rust ecosystem does not suffer from rampant use of non-snake-case-identifiers.
The answer to that question is a very important insight into the ownership system and how it actually contributes to safety. So, I wouldn't dismiss this question as irrelevant at all. |
I don't have an opinion either way on the RFC: sometimes I really don't want to be bothered (function parameters needing mut is annoying), sometimes I really care. I do want to push back on the idea that most people won't turn it off. If you give a new Rust programmer a way to turn this off they're going to turn it off, they're not going to learn when they should and shouldn't. Most Rust code in the future will be written by new Rust programmers, a fact true of most programming languages, but especially ones still growing in popularity. There's no reason to turn this back on once you've turned it off and turning it off is one line, assuming you don't just copy a template that has it off in the first place or whatever. People aren't going to learn mut. They'll probably just come away going "man those Rust people are annoying about lints" and just not bother. if this is accepted it is somewhat equivalent to accepting removal of mut from the language in 5 years, sort of. That's not something I can make a technical argument for, but it's my intuition of it. I also have no real opinion on removal of mut from the language, but I do want to at least point out that it seems like this RFC fundamentally makes that decision, not the smaller decision it makes on the face of it. Eventually the new people who never learned mut are the people designing the Rust of tomorrow, and of the recent RFCs this is probably the one I'd say has the most momentum in changing their view of the language. Now that said on a sidenote, yes you can cheat mut. But in practice I see that rarely and do it rarely. I think that's kind of beside the point, because you have to make up "technically you can" examples that mostly don't happen. Even things like Mutex require the guard to be mutable so even in such cases it's not as "spooky action at a distance" as all that. |
This comment was marked as duplicate.
This comment was marked as duplicate.
Do you want |
This comment was marked as duplicate.
This comment was marked as duplicate.
From an internal-to-the-compiler perspective this might make sense (after all, Having lack of I disagree with the RFCs statement that this would make refactoring and experimenting easier, clearly seeing which parts are mutable makes my reasoning about refactoring and writing code easier. In addition my IDE is set up to underline mutable bindings. Adding/removing mut is trivial and not a burden. As to the "move loophole", I consider it a feature, that I can say that "in this section of this function, the binding is mutable, then it isn't any more" (I rarely see any use case for going the other way, but that could happen I suppose).
Again, I consider this a feature. It is easier to reason about code that is more limited in what it can do.
The problem is that you then have to first grep a code base for anything (Cargo.toml lint table, allow-directive) that changes that default. I would consider it extreme code smell if I came across anyone actually using this, and I would probably avoid using that crate as a dependency if I could. |
This is a very good observation. Why should new Rust programmers that are taught Rust with the lint allowed switch it to deny when it is "easier" to just keep allowing it? |
This is very likely a bad idea for implementation reasons, but may be an interesting one: we could make Rust-analyzer could display the missing |
This thread has gotten very hot very fast, and while I've only skimmed the comments, I don't think most are providing meaningful feedback on the RFC itself. I'm going to lock this thread for now, at least until the lang team can give a vibe check on whether or not this is insta-close. |
Co-authored-by: Wilfred Hughes <[email protected]>
Co-authored-by: Wilfred Hughes <[email protected]>
This comment was marked as duplicate.
This comment was marked as duplicate.
If you are permitted to comment here while this subject is locked, please refrain from striking up further discussion until this is unlocked. It is unfair to those who are not bound equally. Further, both now and when it is unlocked, you will have to try very hard to say something new: the majority of relevant points have already been made and further reiteration of them will not improve the quality nor tone of this discussion. @ChayimFriedman2 For instance, your remarks regarding it being about correctness were already stated by @AndWass and @c0repwn3r in a largely similar form. |
I have done a pass of marking-as-duplicate a number of comments. This is not a criticism of the content of those comments per se, only that we cannot possibly discuss this with a thousand "plus one" comments. Please, unless the statement you are making is procedural with respect to the RFC and not about its contents, all further commentary on the contents of this RFC should prefer to use an inline review. And by "discuss", I mean "even to reject it". To those who are concerned about this proposal, please understand that anyone, including you, random person of the public, can open an RFC. This RFC will not simply be thrown in without consideration of the drawbacks, many of which have already been voiced by members of the Rust team. If you have an issue with the contents of this RFC, you will not improve things by making it harder to moderate this. The reason this RFC has to happen, and should be taken seriously, is that we need to actually have a real discussion and decision in the public record, instead of muttering in various corners about it. Thus I thank the author for having the courage to start this. As they said on the original PR:
This may remain locked for a bit longer until we determine we are sufficiently ready to moderate this. |
As discussed previously:
Mutation of non-mutable locals arguably resembles a lint more than a hard error. Converting it to a configurable lint has the potential to reduce friction and more clearly distinguish it from Rust's more fundamental type system restrictions.
Implementing this is less effort than you might expect: the borrow checker already has separate code paths for handling local variables. I've written a proof-of-concept for the change as part of this pull request.
Rendered