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

Shadowing an immutable binding to a mutable one #1699

Open
KasMA1990 opened this Issue Apr 25, 2017 · 5 comments

Comments

Projects
None yet
3 participants
@KasMA1990
Copy link

KasMA1990 commented Apr 25, 2017

In Rust it is possible to shadow a binding to some data, and as part of that, the binding can also be freely changed from immutable to mutable and vice versa. Shadowing that goes from mutable bindings to immutable bindings are good to have, since they allow you to do some amount of mutation before locking the data down. However, shadowing that goes from immutable bindings to mutable bindings are less useful as they hurt readability (an immutable binding cannot be assumed to remain immutable at any point).

A lint against cases of shadowing used to change a binding from immutable to mutable would thus be nice :)

@KasMA1990

This comment has been minimized.

Copy link

KasMA1990 commented Apr 25, 2017

The discussion for this started as part of #1691 but has now moved here

@KasMA1990

This comment has been minimized.

Copy link

KasMA1990 commented Apr 25, 2017

And again, I'm still interested in implementing such a lint if it's something that we want in clippy, though I'm still a noob, and may need help!

@KasMA1990

This comment has been minimized.

Copy link

KasMA1990 commented Apr 26, 2017

I think this should be less about shadowing and more about being able to guarantee that once some data has been declared immutable, this does not change. These are the cases I've been able to think up that would be relevant:

  1. Shadowing (as per the OP)
  2. Passing an immutable binding to a function which transforms it to a mutable binding (as per #1691)
  3. ... any more? Transmute maybe, but I'm not sure if that's overkill

A second thought was also that this should probably be able to differentiate between Copy and !Copy types, such that you can configure if you only want this for !Copy or for both Copy and !Copy. I don't see the usecase for only checking against Copy types, but maybe I'm missing something.

Feedback is very welcome :)

@oli-obk

This comment has been minimized.

Copy link
Collaborator

oli-obk commented Apr 27, 2017

I don't think 2. is an issue. You are not passing an immutable binding to a function. You are passing a value to said function. Whether the function mutates that value or not is its own choice, not the caller's choice.

We already have a shadowing lint in the clippy_pedantic section. You can probably hook into there and check whether a previously immutable binding was made mutable.

In case it's a function argument, I think it would be best to suggest to make it mutable in the argument list instead of shadowing it.

In case it's a local let binding, suggest to make the original binding mutable.

Or am I misunderstanding your intentions with this lint?

@KasMA1990

This comment has been minimized.

Copy link

KasMA1990 commented Apr 27, 2017

No, you're bang on the money for what I wanted to do initially. But what I realized afterwards was that, in order for this to be really useful, I would probably want to be sure that once I have declared some data as immutable, then it must never be mutated again.

Now that I think it over again, it could just be down to me misunderstanding what immutability in Rust really means. My notion of immutability comes from functional programming, where it means immutable for all eternity. Rust doesn't need that guarantee for owned values, so maybe I just need to adjust my notion of immutability to only be applicable within the current scope. When you can so easily turn something from being immutable to being mutable, the only reason to have immutability in the first place would be to make the code more readable by showing your intent.

In that sense, focusing only on shadowing seems like a good solution, since eternal immutability is not a concept Rust needs. This also solves the need for differentiating Copy and !Copy. (I'm learning a lot here!)

To get back to your input, those are the suggestions I imagined giving the user (and good point about turning the function argument mutable). I'll take a look in clippy_pedantic! :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment