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

Rename and allow cast_ref_to_mut lint #113422

Merged
merged 3 commits into from
Jul 29, 2023

Conversation

Urgau
Copy link
Member

@Urgau Urgau commented Jul 6, 2023

This PR is a small subset of #112431, that is the renaming of the lint (cast_ref_to_mut -> invalid_reference_casting).

BUT also temporarily change the default level of the lint from deny-by-default to allow-by-default until #112431 is merged.

r? @Nilstrieb

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Jul 6, 2023
@rustbot
Copy link
Collaborator

rustbot commented Jul 6, 2023

Some changes occurred in src/tools/clippy

cc @rust-lang/clippy

@Noratrieb
Copy link
Member

cc @rust-lang/lang not sure whether this needs any signoff

@rust-log-analyzer

This comment has been minimized.

@Urgau Urgau force-pushed the cast_ref_to_mut-pre-beta branch from 934121f to 9285acb Compare July 6, 2023 20:26
@rustbot
Copy link
Collaborator

rustbot commented Jul 6, 2023

The Miri subtree was changed

cc @rust-lang/miri

@Urgau
Copy link
Member Author

Urgau commented Jul 6, 2023

cc @rust-lang/lang not sure whether this needs any signoff

I don't think it needs another T-lang signoff, this is purely a compiler change (well, the lint level change is technically T-lang purview, but it's deny->allow, and it's temporary before of beta).

Anyway, I think it's better to get this PR merged into 1.72 and if T-lang wants to do another signoff they can do it in the other PR.

@Noratrieb
Copy link
Member

Question for T-lang: is the lint rename fine? (And also additionally, is T-lang input required for these sort of things?).

@Noratrieb Noratrieb added the I-lang-nominated Nominated for discussion during a lang team meeting. label Jul 10, 2023
@bors
Copy link
Contributor

bors commented Jul 11, 2023

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

@bors
Copy link
Contributor

bors commented Jul 13, 2023

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

@scottmcm
Copy link
Member

Lints aren't "one-way doors", so personally I'm not concerned here. The worst thing that can happen is we end up with another lint name in the renamed or removed tables. To me, the important thing is that lang agrees that whatever lang thing being linting about really is something that people oughtn't ever be doing -- which might be easy if it's definitely UB -- and if the compiler diagnostics folk think a different name is better for the lint then go for it. (If we already have a bunch of things named invalid and this is like those, then great.)

I'll leave it nominated for the team to discuss Tuesday in case someone has strong objections to the name, but I don't think you need to wait for that input.

@Noratrieb
Copy link
Member

Looks like there were no objections and I agree, the new name is better.
@bors r+

@bors
Copy link
Contributor

bors commented Jul 28, 2023

📌 Commit f25ad54 has been approved by Nilstrieb

It is now in the queue for this repository.

@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 Jul 28, 2023
@Noratrieb
Copy link
Member

I also want to nominate this for beta backport so that people don't need to deal with the lint rename by getting the new name directly. The lint does have false positives on master so it's possible that people would want to allow it. That said, maybe the rename coming to stable together with the false positive fixes would make people remove the allows.
I have no strong recommendation whether to accept but I think it's worth nominating.

@Noratrieb Noratrieb added the beta-nominated Nominated for backporting to the compiler in the beta channel. label Jul 28, 2023
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Jul 28, 2023
…Nilstrieb

Rename and allow `cast_ref_to_mut` lint

This PR is a small subset of rust-lang#112431, that is the renaming of the lint (`cast_ref_to_mut` -> `invalid_reference_casting`).

BUT also temporarily change the default level of the lint from deny-by-default to allow-by-default until rust-lang#112431 is merged.

r? `@Nilstrieb`
@bors
Copy link
Contributor

bors commented Jul 29, 2023

⌛ Testing commit f25ad54 with merge 0441150...

@bors
Copy link
Contributor

bors commented Jul 29, 2023

☀️ Test successful - checks-actions
Approved by: Nilstrieb
Pushing 0441150 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Jul 29, 2023
@bors bors merged commit 0441150 into rust-lang:master Jul 29, 2023
@rustbot rustbot added this to the 1.73.0 milestone Jul 29, 2023
@Urgau Urgau deleted the cast_ref_to_mut-pre-beta branch July 29, 2023 09:56
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (0441150): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
1.7% [1.4%, 2.1%] 3
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
-2.0% [-2.0%, -2.0%] 1
Improvements ✅
(secondary)
-3.2% [-3.2%, -3.2%] 1
All ❌✅ (primary) 0.8% [-2.0%, 2.1%] 4

Cycles

This benchmark run did not return any relevant results for this metric.

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 652.195s -> 652.022s (-0.03%)

bors added a commit to rust-lang-ci/rust that referenced this pull request Aug 2, 2023
…=Nilstrieb

Improve `invalid_reference_casting` lint

This PR is a follow-up to rust-lang#111567 and rust-lang#113422.

This PR does multiple things:
 - First it adds support for deferred de-reference, the goal is to support code like this, where the casting and de-reference are not done on the same expression
    ```rust
    let myself = self as *const Self as *mut Self;
    *myself = Self::Ready(value);
    ```
 - Second it does not lint anymore on SB/TB UB code by only checking assignments (`=`, `+=`, ...) and creation of mutable references `&mut *`
 - Thirdly it greatly improves the diagnostics in particular for cast from `&mut` to `&mut` or assignments
 - ~~And lastly it renames the lint from `cast_ref_to_mut` to `invalid_reference_casting` which is more consistent with the ["rules"](rust-lang/rust-clippy#2845) and also more consistent with what the lint checks~~ *rust-lang#113422

This PR is best reviewed commit by commit.

r? compiler
github-actions bot pushed a commit to rust-lang/miri that referenced this pull request Aug 3, 2023
Improve `invalid_reference_casting` lint

This PR is a follow-up to rust-lang/rust#111567 and rust-lang/rust#113422.

This PR does multiple things:
 - First it adds support for deferred de-reference, the goal is to support code like this, where the casting and de-reference are not done on the same expression
    ```rust
    let myself = self as *const Self as *mut Self;
    *myself = Self::Ready(value);
    ```
 - Second it does not lint anymore on SB/TB UB code by only checking assignments (`=`, `+=`, ...) and creation of mutable references `&mut *`
 - Thirdly it greatly improves the diagnostics in particular for cast from `&mut` to `&mut` or assignments
 - ~~And lastly it renames the lint from `cast_ref_to_mut` to `invalid_reference_casting` which is more consistent with the ["rules"](rust-lang/rust-clippy#2845) and also more consistent with what the lint checks~~ *rust-lang/rust#113422

This PR is best reviewed commit by commit.

r? compiler
@apiraino
Copy link
Contributor

apiraino commented Aug 4, 2023

Beta backport accepted as per compiler team on Zulip

@rustbot label +beta-accepted

@rustbot rustbot added the beta-accepted Accepted for backporting to the compiler in the beta channel. label Aug 4, 2023
flip1995 pushed a commit to flip1995/rust that referenced this pull request Aug 11, 2023
…lstrieb

Rename and allow `cast_ref_to_mut` lint

This PR is a small subset of rust-lang#112431, that is the renaming of the lint (`cast_ref_to_mut` -> `invalid_reference_casting`).

BUT also temporarily change the default level of the lint from deny-by-default to allow-by-default until rust-lang#112431 is merged.

r? `@Nilstrieb`
@cuviper cuviper mentioned this pull request Aug 12, 2023
@cuviper cuviper modified the milestones: 1.73.0, 1.72.0 Aug 12, 2023
@cuviper cuviper removed the beta-nominated Nominated for backporting to the compiler in the beta channel. label Aug 12, 2023
bors added a commit to rust-lang-ci/rust that referenced this pull request Aug 12, 2023
[beta] backport

* Restrict linker version script of proc-macro crates to just its two symbols rust-lang#114470
* bootstrap: config: fix version comparison bug rust-lang#114440
* lint/ctypes: only try normalize rust-lang#113921
* Avoid tls access while iterating through mpsc thread entries rust-lang#113861
* Substitute types before checking inlining compatibility. rust-lang#113802
* Revert "fix: bug etc/bash_complettion -> src/etc/... to avoid copy error" rust-lang#113579
* lint/ctypes: fix () return type checks rust-lang#113457
* Rename and allow cast_ref_to_mut lint rust-lang#113422
* Ignore flaky clippy tests. rust-lang#113621

r? cuviper
thomcc pushed a commit to tcdi/postgrestd that referenced this pull request Oct 17, 2023
Improve `invalid_reference_casting` lint

This PR is a follow-up to rust-lang/rust#111567 and rust-lang/rust#113422.

This PR does multiple things:
 - First it adds support for deferred de-reference, the goal is to support code like this, where the casting and de-reference are not done on the same expression
    ```rust
    let myself = self as *const Self as *mut Self;
    *myself = Self::Ready(value);
    ```
 - Second it does not lint anymore on SB/TB UB code by only checking assignments (`=`, `+=`, ...) and creation of mutable references `&mut *`
 - Thirdly it greatly improves the diagnostics in particular for cast from `&mut` to `&mut` or assignments
 - ~~And lastly it renames the lint from `cast_ref_to_mut` to `invalid_reference_casting` which is more consistent with the ["rules"](rust-lang/rust-clippy#2845) and also more consistent with what the lint checks~~ *rust-lang/rust#113422

This PR is best reviewed commit by commit.

r? compiler
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
beta-accepted Accepted for backporting to the compiler in the beta channel. I-lang-nominated Nominated for discussion during a lang team meeting. 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. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants