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

Simplify some of the logic in the invalid_reference_casting lint #116199

Merged
merged 3 commits into from Sep 28, 2023

Conversation

Urgau
Copy link
Contributor

@Urgau Urgau commented Sep 27, 2023

This PR simplifies 2 areas of the logic for the invalid_reference_casting lint:

  • The init detection: we now use the newly added expr_or_init function instead of a manual detection
  • The ref-to-mut-ptr casting detection logic: I simplified this logic by caring less hardly about the order of the casting operations

Those two simplifications permits us to detect more cases, as can be seen in the test output changes.

@rustbot
Copy link
Collaborator

rustbot commented Sep 27, 2023

r? @cjgillot

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

@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 Sep 27, 2023
@rust-log-analyzer

This comment has been minimized.

@Urgau
Copy link
Contributor Author

Urgau commented Sep 27, 2023

I'm not so sure about the last commit, it fixes the ICEs in #116199 (comment) but at the cost a new scaled down function, just to avoid outside expr outside the current body.

Maybe we should check that the result of expr_or_init has same the owner as the expression passed in? (but then we are potentially doing more work than necessary)

compiler/rustc_lint/src/reference_casting.rs Show resolved Hide resolved
tests/ui/lint/reference_casting.rs Outdated Show resolved Hide resolved
/// dbg!(def);
/// // ^^^ input
/// ```
pub fn expr_or_init<'a>(&self, mut expr: &'a hir::Expr<'tcx>) -> &'a hir::Expr<'tcx> {
Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't it a bit dangerous to change the other method's name and reusing it for this one?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, a bit, but the function was added very recently, has only one user (the utf8 lints) and it is now better matching the similar function from clippy: clippy_utils::expr_or_init.

arg
} else if had_at_least_one_cast {
Copy link
Contributor

Choose a reason for hiding this comment

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

IIUC, this flag is an optimization to avoid calling node_type if we haven't seed any cast. Is this worth it? node_type is a simple hashmap lookup.
This deserves at least a comment.

Copy link
Contributor Author

@Urgau Urgau Sep 27, 2023

Choose a reason for hiding this comment

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

Since we are now checking the end type at the start, I agree keeping this flag doesn't seems compelling enough. Removed.

@Urgau Urgau force-pushed the simplify-invalid_ref_casting branch from ffb8f04 to 1b2c1a8 Compare September 27, 2023 16:59
@cjgillot
Copy link
Contributor

@bors r+

@bors
Copy link
Contributor

bors commented Sep 28, 2023

📌 Commit 1b2c1a8 has been approved by cjgillot

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 Sep 28, 2023
@bors
Copy link
Contributor

bors commented Sep 28, 2023

⌛ Testing commit 1b2c1a8 with merge 1393ef1...

@bors
Copy link
Contributor

bors commented Sep 28, 2023

☀️ Test successful - checks-actions
Approved by: cjgillot
Pushing 1393ef1 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Sep 28, 2023
@bors bors merged commit 1393ef1 into rust-lang:master Sep 28, 2023
12 checks passed
@rustbot rustbot added this to the 1.74.0 milestone Sep 28, 2023
@Urgau Urgau deleted the simplify-invalid_ref_casting branch September 28, 2023 21:41
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (1393ef1): 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)
0.7% [0.7%, 0.7%] 1
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) 0.7% [0.7%, 0.7%] 1

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: 631.024s -> 630.971s (-0.01%)
Artifact size: 317.33 MiB -> 317.33 MiB (-0.00%)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.

None yet

6 participants