Skip to content
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

Disallow taking non zero-sized constants by mutable reference #885

Closed
petrochenkov opened this issue Feb 18, 2015 · 11 comments
Closed

Disallow taking non zero-sized constants by mutable reference #885

petrochenkov opened this issue Feb 18, 2015 · 11 comments
Labels
breaking-change The RFC proposes a breaking change. T-lang Relevant to the language team, which will review and decide on the RFC.

Comments

@petrochenkov
Copy link
Contributor

As @eddyb explained here, constants (const items) no longer behave as rvalues when taken by immutable reference (or by mutable reference in case of zero-sized constants).
Yet they still behave as rvalues and create a temporary on the stack when taken by mutable reference:

const C: u8 = 0;
fn main() {
    let c = &mut C; // A mutable temporary is created
    *c = 1;
}

One more example where the mutable borrowing is implicit and the result is very confusing:

const A: [u8; 3] = [1, 2, 3];
fn main() {
    A[0] = 4; // Surprise!
}

I suggest to prohibit taking constants by mutable reference.
If necessary, the mutable temporary can be created explicitly.
It is rarely needed and the prohibition would not affect ergonomics.
The achieved explicitness would be beneficial for both a reader (it's clearly visible that the reference doesn't bind directly and temporary is created) and a writer (no potential mistakes with surprising consequences) of the code.


A more radical solution would be to disallow taking all rvalues by mutable reference, like C++ does.
If you know the history, early C++ allowed it for a long time, but in the end it was prohibited anyway during the standardization. The consequences are still visible today - Visual C++ still has to allow binding mutable references to temporaries to preserve backward compatibility with older code.

@glaebhoerl
Copy link
Contributor

Out of curiosity, is there a compelling reason to allow it for zero-sized constants as a special case, as in the title? What's the use case?

@eddyb
Copy link
Member

eddyb commented Feb 19, 2015

@glaebhoerl &mut || {....} and &mut [] are the two main usecases, the latter having worked for a longer time.

@glaebhoerl
Copy link
Contributor

Closures make sense, but what is &mut [] useful for?

@petrochenkov
Copy link
Contributor Author

I'd like to bring someone's attention to this issue, because the proposed change is backward incompatible.
ping @nikomatsakis

@reem
Copy link

reem commented Feb 23, 2015

@glaebhoerl sometimes you need to conjure an empty slice out of thin air to fulfill all kinds of types and APIs.

@sanmai-NL
Copy link

sanmai-NL commented Feb 20, 2017

@eddyb, @petrochenkov: So mutably referencing zero size literals is useful (at least, used). But mutably referencing const items still isn't, right? Maybe the distinction is not so easy to make by the compiler, but I would think that the latter should be forbidden in principle. It's interesting to try to think of examples where some programmers could introduce bugs by not being prevented to do so by the compiler. A slight bug would be besides executing an ‘effectless’ statement also leaking memory by creating a hidden temporary to a large constant in a long-lived scope (e.g. main() in some daemon).

In case the distinction I drew between literals and const items is sensible, may I ask, what are your (current) views on whether this should be allowed?

@petrochenkov
Copy link
Contributor Author

My views didn't change since Feb 18, 2015.
This should be reported at least as a warning.

@burdges
Copy link

burdges commented Feb 20, 2017

I'm dubious that let c = &mut C; should be disallowed because one does that with literals all the time, ala let c = &mut [0u8; 16];.

I certainly agree your line marked "Surprise!" should be an error though. Could the resulting rvalue simply be marked as #[must_use]?

@nikomatsakis
Copy link
Contributor

@petrochenkov hmm I missed these pings first time around. I agree there is some potential for confusion. I'm curious, would you be up for preparing a branch to make this change? I'd like to measure the potential impact. It'd be good to know how much code is affected.

@nikomatsakis
Copy link
Contributor

I suspect a lint is more likely than banning such a construct altogether. I'm not sure what would be the best way to target the lint. Either way, seeing cases of code that breaks might help to figure that out.

@petrochenkov petrochenkov added breaking-change The RFC proposes a breaking change. T-lang Relevant to the language team, which will review and decide on the RFC. labels Feb 21, 2017
@petrochenkov
Copy link
Contributor Author

Closing in favor of a rustc warning (rust-lang/rust#40543) or clippy warning (rust-lang/rust-clippy#829).
The warning may be implemented as a compatibility lint if this is decided to be deprecated.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking-change The RFC proposes a breaking change. T-lang Relevant to the language team, which will review and decide on the RFC.
Projects
None yet
Development

No branches or pull requests

7 participants