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

Add CONST_ITEM_MUTATION lint #75573

Merged
merged 2 commits into from
Sep 10, 2020

Conversation

Aaron1011
Copy link
Member

Fixes #74053
Fixes #55721

This PR adds a new lint CONST_ITEM_MUTATION.
Given an item const FOO: SomeType = .., this lint fires on:

  • Attempting to write directly to a field (FOO.field = some_val) or
    array entry (FOO.array_field[0] = val)
  • Taking a mutable reference to the const item (&mut FOO), including
    through an autoderef FOO.some_mut_self_method()

The lint message explains that since each use of a constant creates a
new temporary, the original const item will not be modified.

@rust-highfive
Copy link
Collaborator

r? @oli-obk

(rust_highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 15, 2020
LL | const MY_STRUCT: MyStruct = MyStruct { field: true, inner_array: ['a'] };
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

warning: taking a mutable reference to a `const` item
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems quite verbose - I'm open to suggestions as to how to condense this information.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Eventually in the next Rust edition this warning could become an error?

Comment on lines +12 to +13
LL | const ARRAY: [u8; 1] = [25];
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Copy link
Contributor

@tesuji tesuji Aug 16, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would like the "^" to be cover the name and type, like this:

Suggested change
LL | const ARRAY: [u8; 1] = [25];
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
LL | const ARRAY: [u8; 1] = [25];
| ^^^^^^^^^^^^^^^^^^^^

The reason is the const expression in RHS could be a big block.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we decide to do this, I think it should be done consistently (e.g. as a part of #75465), rather than special-casing this lint.

MY_STRUCT.field = false; //~ WARN attempting to modify
MY_STRUCT.inner_array[0] = 'b'; //~ WARN attempting to modify
MY_STRUCT.use_mut(); //~ WARN taking
&mut MY_STRUCT; //~ WARN taking
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think your lint will also catch

let x = &mut MY_STRUCT;

which seems to me to be equivalent to let x = &mut Default::default() or similar things that (arguably rarely) are legitimite use cases.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's intentional - I think let x = &mut MY_STRUCT looks extremely misleading. If it there's ever the need for it in practice (maybe macro-generated code?), users can add #[allow(const_item_mutation)].

However, this shouldn't become a hard error.

@oli-obk
Copy link
Contributor

oli-obk commented Aug 17, 2020

We have rules about new lints, I'm not sure this warrants a rustc lint. I'd be fine with adding the relevant information to MIR but adding the lint to clippy (cc rust-lang/rust-clippy#829). @rust-lang/clippy what do you think?

@Aaron1011
Copy link
Member Author

I think directly assigning to fields of a const should be a rust lint - it's completely useless (other than consuming the RHS), and always indicates a bug. I think this should be considered a natural extension of forbidding using a constant directly in the LHS of an assignment.

Calling a &mut self method might be intentional and legitimate, if the method is called for reasons unrelated to the &mut self (e.g. mutating global state or performing I/O). However, I think the danger of a questionable false-positive is far outweighed by the risk of accepting code like:

const MY_VEC: Vec<u8> = Vec::new();
MY_VEC.push(25);

which might come about as a result of several unrelated refactoring (changing a static to a const, and changing a method to take &mut self rather than use interior mutability).

I don't know what the official policy is w.r.t adding new lints to rustc vs clippy. IMHO, 'correctness' lints which detect obviously broken code belong in rustc, not clippy. Assuming that there are no false positives, I see no downsides other than breaking #![deny(warnings)] crates (which explicitly opted into such breakage).

@oli-obk
Copy link
Contributor

oli-obk commented Aug 17, 2020

Well... I'm not sure how strongly that rule is followed, but new lints in rustc need an RFC. Maybe an MCP is good enough here though. You can open an MCP here. It's not that much effort, just writing a few sentences of explanation and linking to the clippy and rustc issues and this PR should be enough. We'll then bring it up at the compiler team mtg

@Aaron1011
Copy link
Member Author

@oli-obk MCP opened at rust-lang/compiler-team#345

@flip1995
Copy link
Member

Clippy already has a lint which implements part of the proposed lint: Playground

I would be good with stop linting assignments to constants in the temporary_assignment lint and move it to a new lint.

There was an issue opened in Clippy before, asking for the temporary_assignment to get uplifted: rust-lang/rust-clippy#4497

@flip1995
Copy link
Member

I'm ok with adding this lint to Clippy. We can implement it in the rust repo and then sync it back later.


If it gets implemented directly in rustc, it would be great to stop linting on const assignments in clippy::temporary_assignment already in this PR.

@Manishearth
Copy link
Member

I concur with @flip1995

I'm not against adding it to rustc directly, however rustc typically has a more onerous process for adding lints, so it's up to you!

@Aaron1011
Copy link
Member Author

@oli-obk: The MCP has been accepted at rust-lang/compiler-team#345 - what is the next step?

@oli-obk
Copy link
Contributor

oli-obk commented Aug 28, 2020

Now we review the impl and merge it. I won't be able to get to this until the 7th though, so if someone else grabs it in that time, I won't complain.

@bors
Copy link
Contributor

bors commented Aug 30, 2020

☔ The latest upstream changes (presumably #74862) made this pull request unmergeable. Please resolve the merge conflicts.

@flip1995
Copy link
Member

It would be great if you could assure in this PR, that the cases in src/tools/clippy/tests/ui/temporary_assignment.rs doesn't get linted twice by both Clippy and rustc.

@jyn514 jyn514 added the A-lint Area: Lints (warnings about flaws in source code) such as unused_mut. label Sep 3, 2020
@tesuji

This comment has been minimized.

@Aaron1011
Copy link
Member Author

@lzutao: This does not actually fix #65822 - that issue deals with interior mutability, while this PR only deals with normal 'exterior' mutability (&mut references)

@tesuji
Copy link
Contributor

tesuji commented Sep 4, 2020

Oh, my mistake. Probably another orthogonal lint but requires sign off from language team.

@bors
Copy link
Contributor

bors commented Sep 4, 2020

☔ The latest upstream changes (presumably #75077) made this pull request unmergeable. Please resolve the merge conflicts.

Copy link
Contributor

@oli-obk oli-obk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This lgtm now. Just some nits, then we can merge it

Fixes rust-lang#74053
Fixes rust-lang#55721

This PR adds a new lint `CONST_ITEM_MUTATION`.
Given an item `const FOO: SomeType = ..`, this lint fires on:

* Attempting to write directly to a field (`FOO.field = some_val`) or
  array entry (`FOO.array_field[0] = val`)
* Taking a mutable reference to the `const` item (`&mut FOO`), including
  through an autoderef `FOO.some_mut_self_method()`

The lint message explains that since each use of a constant creates a
new temporary, the original `const` item will not be modified.
@flip1995
Copy link
Member

flip1995 commented Sep 7, 2020

Clippy stderr diff:

temporary_assignment.rs
-error: assignment to temporary
-  --> $DIR/temporary_assignment.rs:47:5
-   |
-LL |     Struct { field: 0 }.field = 1;
-   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
-   |
-   = note: `-D clippy::temporary-assignment` implied by `-D warnings`
-
-error: assignment to temporary
-  --> $DIR/temporary_assignment.rs:48:5
-   |
-LL | /     MultiStruct {
-LL | |         structure: Struct { field: 0 },
-LL | |     }
-LL | |     .structure
-LL | |     .field = 1;
-   | |______________^
-
-error: assignment to temporary
-  --> $DIR/temporary_assignment.rs:53:5
-   |
-LL |     ArrayStruct { array: [0] }.array[0] = 1;
-   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
-
-error: assignment to temporary
-  --> $DIR/temporary_assignment.rs:54:5
-   |
-LL |     (0, 0).0 = 1;
-   |     ^^^^^^^^^^^^
-
-error: assignment to temporary
+error: attempting to modify a `const` item
   --> $DIR/temporary_assignment.rs:56:5
    |
 LL |     A.0 = 2;
    |     ^^^^^^^
+   |
+   = note: `-D const-item-mutation` implied by `-D warnings`
+   = note: each usage of a `const` item creates a new temporary - the original `const` item will not be modified
+note: `const` item defined here
+  --> $DIR/temporary_assignment.rs:36:1
+   |
+LL | const A: TupleStruct = TupleStruct(1);
+   | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

-error: assignment to temporary
+error: attempting to modify a `const` item
   --> $DIR/temporary_assignment.rs:57:5
    |
 LL |     B.field = 2;
    |     ^^^^^^^^^^^
+   |
+   = note: each usage of a `const` item creates a new temporary - the original `const` item will not be modified
+note: `const` item defined here
+  --> $DIR/temporary_assignment.rs:37:1
+   |
+LL | const B: Struct = Struct { field: 1 };
+   | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

-error: assignment to temporary
+error: attempting to modify a `const` item
   --> $DIR/temporary_assignment.rs:58:5
    |
 LL |     C.structure.field = 2;
    |     ^^^^^^^^^^^^^^^^^^^^^
+   |
+   = note: each usage of a `const` item creates a new temporary - the original `const` item will not be modified
+note: `const` item defined here
+  --> $DIR/temporary_assignment.rs:38:1
+   |
+LL | / const C: MultiStruct = MultiStruct {
+LL | |     structure: Struct { field: 1 },
+LL | | };
+   | |__^

-error: assignment to temporary
+error: attempting to modify a `const` item
   --> $DIR/temporary_assignment.rs:59:5
    |
 LL |     D.array[0] = 2;
    |     ^^^^^^^^^^^^^^
+   |
+   = note: each usage of a `const` item creates a new temporary - the original `const` item will not be modified
+note: `const` item defined here
+  --> $DIR/temporary_assignment.rs:41:1
+   |
+LL | const D: ArrayStruct = ArrayStruct { array: [1] };
+   | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

-error: aborting due to 8 previous errors
+error: aborting due to 4 previous errors
borrow_interior_mutable_const.rs
-error: a `const` item with interior mutability should not be borrowed
-  --> $DIR/borrow_interior_mutable_const.rs:65:5
+error: taking a mutable reference to a `const` item
+  --> $DIR/borrow_interior_mutable_const.rs:72:21
    |
-LL |     ATOMIC.store(1, Ordering::SeqCst); //~ ERROR interior mutability
-   |     ^^^^^^
-   |
-   = note: `-D clippy::borrow-interior-mutable-const` implied by `-D warnings`
-   = help: assign this const to a local or static variable, and use the variable here
-
-error: a `const` item with interior mutability should not be borrowed
-  --> $DIR/borrow_interior_mutable_const.rs:66:16
-   |
-LL |     assert_eq!(ATOMIC.load(Ordering::SeqCst), 5); //~ ERROR interior mutability
-   |                ^^^^^^
-   |
-   = help: assign this const to a local or static variable, and use the variable here
-
-error: a `const` item with interior mutability should not be borrowed
-  --> $DIR/borrow_interior_mutable_const.rs:69:22
-   |
-LL |     let _once_ref = &ONCE_INIT; //~ ERROR interior mutability
-   |                      ^^^^^^^^^
-   |
-   = help: assign this const to a local or static variable, and use the variable here
-
-error: a `const` item with interior mutability should not be borrowed
-  --> $DIR/borrow_interior_mutable_const.rs:70:25
-   |
-LL |     let _once_ref_2 = &&ONCE_INIT; //~ ERROR interior mutability
-   |                         ^^^^^^^^^
-   |
-   = help: assign this const to a local or static variable, and use the variable here
-
-error: a `const` item with interior mutability should not be borrowed
-  --> $DIR/borrow_interior_mutable_const.rs:71:27
-   |
-LL |     let _once_ref_4 = &&&&ONCE_INIT; //~ ERROR interior mutability
-   |                           ^^^^^^^^^
-   |
-   = help: assign this const to a local or static variable, and use the variable here
-
-error: a `const` item with interior mutability should not be borrowed
-  --> $DIR/borrow_interior_mutable_const.rs:72:26
-   |
 LL |     let _once_mut = &mut ONCE_INIT; //~ ERROR interior mutability
-   |                          ^^^^^^^^^
+   |                     ^^^^^^^^^^^^^^
    |
-   = help: assign this const to a local or static variable, and use the variable here
-
-error: a `const` item with interior mutability should not be borrowed
-  --> $DIR/borrow_interior_mutable_const.rs:83:14
+   = note: `-D const-item-mutation` implied by `-D warnings`
+   = note: each usage of a `const` item creates a new temporary
+   = note: the mutable reference will refer to this temporary, not the original `const` item
+note: `const` item defined here
+  --> $DIR/borrow_interior_mutable_const.rs:19:1
    |
-LL |     let _ = &ATOMIC_TUPLE; //~ ERROR interior mutability
-   |              ^^^^^^^^^^^^
-   |
-   = help: assign this const to a local or static variable, and use the variable here
+LL | const ONCE_INIT: Once = Once::new();
+   | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

-error: a `const` item with interior mutability should not be borrowed
-  --> $DIR/borrow_interior_mutable_const.rs:84:14
-   |
-LL |     let _ = &ATOMIC_TUPLE.0; //~ ERROR interior mutability
-   |              ^^^^^^^^^^^^
-   |
-   = help: assign this const to a local or static variable, and use the variable here
-
-error: a `const` item with interior mutability should not be borrowed
-  --> $DIR/borrow_interior_mutable_const.rs:85:19
-   |
-LL |     let _ = &(&&&&ATOMIC_TUPLE).0; //~ ERROR interior mutability
-   |                   ^^^^^^^^^^^^
-   |
-   = help: assign this const to a local or static variable, and use the variable here
-
-error: a `const` item with interior mutability should not be borrowed
-  --> $DIR/borrow_interior_mutable_const.rs:86:14
-   |
-LL |     let _ = &ATOMIC_TUPLE.0[0]; //~ ERROR interior mutability
-   |              ^^^^^^^^^^^^
-   |
-   = help: assign this const to a local or static variable, and use the variable here
-
-error: a `const` item with interior mutability should not be borrowed
-  --> $DIR/borrow_interior_mutable_const.rs:87:13
-   |
-LL |     let _ = ATOMIC_TUPLE.0[0].load(Ordering::SeqCst); //~ ERROR interior mutability
-   |             ^^^^^^^^^^^^
-   |
-   = help: assign this const to a local or static variable, and use the variable here
-
-error: a `const` item with interior mutability should not be borrowed
-  --> $DIR/borrow_interior_mutable_const.rs:93:13
-   |
-LL |     let _ = ATOMIC_TUPLE.0[0]; //~ ERROR interior mutability
-   |             ^^^^^^^^^^^^
-   |
-   = help: assign this const to a local or static variable, and use the variable here
-
-error: a `const` item with interior mutability should not be borrowed
-  --> $DIR/borrow_interior_mutable_const.rs:98:5
-   |
-LL |     CELL.set(2); //~ ERROR interior mutability
-   |     ^^^^
-   |
-   = help: assign this const to a local or static variable, and use the variable here
-
-error: a `const` item with interior mutability should not be borrowed
-  --> $DIR/borrow_interior_mutable_const.rs:99:16
-   |
-LL |     assert_eq!(CELL.get(), 6); //~ ERROR interior mutability
-   |                ^^^^
-   |
-   = help: assign this const to a local or static variable, and use the variable here
-
-error: a `const` item with interior mutability should not be borrowed
-  --> $DIR/borrow_interior_mutable_const.rs:112:5
-   |
-LL |     u64::ATOMIC.store(5, Ordering::SeqCst); //~ ERROR interior mutability
-   |     ^^^^^^^^^^^
-   |
-   = help: assign this const to a local or static variable, and use the variable here
-
-error: a `const` item with interior mutability should not be borrowed
-  --> $DIR/borrow_interior_mutable_const.rs:113:16
-   |
-LL |     assert_eq!(u64::ATOMIC.load(Ordering::SeqCst), 9); //~ ERROR interior mutability
-   |                ^^^^^^^^^^^
-   |
-   = help: assign this const to a local or static variable, and use the variable here
-
-error: aborting due to 16 previous errors
+error: aborting due to previous error

@Aaron1011
Copy link
Member Author

I don't understand why the CONST_ITEM_MUTATION lint is causing some Clippy lints to not run - does -D warnings prevent other lints from running after one causes an error?

@Aaron1011 Aaron1011 added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Sep 7, 2020
@flip1995
Copy link
Member

flip1995 commented Sep 8, 2020

Maybe rustc lints are executed before Clippy lints and since these lints error (-D warnings), the Clippy lints are never executed.

The fix for this is, that the temporary_assignment lint should either be deprecated or only lint in those cases, where the CONST_ITEM_MUTATION lint doesn't trigger.

In the borrow_interior_mutable_const.rs test case the CONST_ITEM_MUTATION should just be allowed.

We no longer lint assignments to const item fields in the
`temporary_assignment` lint, since this is now covered by the
`CONST_ITEM_MUTATION` lint.

Additionally, we `#![allow(const_item_mutation)]` in the
`borrow_interior_mutable_const.rs` test. Clippy UI tests are run with
`-D warnings`, which seems to cause builtin lints to prevent Clippy
lints from running.
@Aaron1011
Copy link
Member Author

@oli-obk @flip1995: I've updated Clippy.

@oli-obk
Copy link
Contributor

oli-obk commented Sep 9, 2020

@bors r+

@bors
Copy link
Contributor

bors commented Sep 9, 2020

📌 Commit 4434e8c has been approved by oli-obk

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Sep 9, 2020
@bors
Copy link
Contributor

bors commented Sep 10, 2020

⌛ Testing commit 4434e8c with merge 8819721...

@bors
Copy link
Contributor

bors commented Sep 10, 2020

☀️ Test successful - checks-actions, checks-azure
Approved by: oli-obk
Pushing 8819721 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Sep 10, 2020
@bors bors merged commit 8819721 into rust-lang:master Sep 10, 2020
@rustbot rustbot added this to the 1.48.0 milestone Sep 10, 2020
flip1995 pushed a commit to flip1995/rust that referenced this pull request Sep 10, 2020
…, r=oli-obk

Add CONST_ITEM_MUTATION lint

Fixes rust-lang#74053
Fixes rust-lang#55721

This PR adds a new lint `CONST_ITEM_MUTATION`.
Given an item `const FOO: SomeType = ..`, this lint fires on:

* Attempting to write directly to a field (`FOO.field = some_val`) or
  array entry (`FOO.array_field[0] = val`)
* Taking a mutable reference to the `const` item (`&mut FOO`), including
  through an autoderef `FOO.some_mut_self_method()`

The lint message explains that since each use of a constant creates a
new temporary, the original `const` item will not be modified.
retrage added a commit to retrage/rust-hypervisor-firmware that referenced this pull request Oct 1, 2020
CONST_ITEM_MUTATION list[1] warns mutable copy of STDIN and STDOUT when
creating UEFI SystemTable. This commit suppresses the warnings.

[1] rust-lang/rust#75573

Signed-off-by: Akira Moroo <retrage01@gmail.com>
josephlr pushed a commit to retrage/rust-hypervisor-firmware that referenced this pull request Oct 1, 2020
CONST_ITEM_MUTATION list[1] warns mutable copy of STDIN and STDOUT when
creating UEFI SystemTable. This commit fixes these warnings.

[1] rust-lang/rust#75573

Signed-off-by: Akira Moroo <retrage01@gmail.com>
Signed-off-by: Joe Richey <joerichey@google.com>
josephlr pushed a commit to retrage/rust-hypervisor-firmware that referenced this pull request Oct 1, 2020
CONST_ITEM_MUTATION list[1] warns mutable copy of STDIN and STDOUT when
creating UEFI SystemTable. This commit fixes these warnings.

[1] rust-lang/rust#75573

Signed-off-by: Akira Moroo <retrage01@gmail.com>
Signed-off-by: Joe Richey <joerichey@google.com>
josephlr pushed a commit to retrage/rust-hypervisor-firmware that referenced this pull request Oct 2, 2020
CONST_ITEM_MUTATION list[1] warns mutable copy of STDIN and STDOUT when
creating UEFI SystemTable. This commit fixes these warnings.

[1] rust-lang/rust#75573

Signed-off-by: Akira Moroo <retrage01@gmail.com>
Signed-off-by: Joe Richey <joerichey@google.com>
josephlr pushed a commit to cloud-hypervisor/rust-hypervisor-firmware that referenced this pull request Oct 2, 2020
CONST_ITEM_MUTATION list[1] warns mutable copy of STDIN and STDOUT when
creating UEFI SystemTable. This commit fixes these warnings.

[1] rust-lang/rust#75573

Signed-off-by: Akira Moroo <retrage01@gmail.com>
Signed-off-by: Joe Richey <joerichey@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-lint Area: Lints (warnings about flaws in source code) such as unused_mut. merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
10 participants