-
Notifications
You must be signed in to change notification settings - Fork 12.9k
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
remove TyS::same_type
#93290
remove TyS::same_type
#93290
Conversation
Some changes occurred in src/tools/clippy. cc @rust-lang/clippy |
(rust-highfive has picked a reviewer for you, use r? to override) |
@bors try @rust-timer queue |
Awaiting bors try build completion. @rustbot label: +S-waiting-on-perf |
⌛ Trying commit 59d14bf0136a0093c3678a664a2f30d876722e2b with merge 5102db108540f7012ca2dec4d07b61ac376df6ce... |
☀️ Try build successful - checks-actions |
Queued 5102db108540f7012ca2dec4d07b61ac376df6ce with parent e7825f2, future comparison URL. |
Finished benchmarking commit (5102db108540f7012ca2dec4d07b61ac376df6ce): comparison url. Summary: This benchmark run did not return any relevant changes. If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf. Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR led to changes in compiler perf. @bors rollup=never |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This overall seems good to me -- definitely a confusing function that seems likely to be at least partially wrong in many use cases.
It looks like the majority of the clippy users arose as a result of rust-lang/rust-clippy#5528, which seems like it's under the impression that this function was intentional -- but I admit I am not fully following the conversation there. Maybe worth considering whether we need to provide a lightweight alternative that does whatever clippy was going for? cc @phansch @flip1995
The uses in this PR seem like they are probably OK with the "full" equality proposed by this PR, but it's hard to be sure. I guess tests are passing (including Clippy tests), though. I think it'd be good to get another set of eyes on these changes (particularly those in Clippy) before merging -- maybe r? @jackh726
@@ -2833,7 +2833,7 @@ impl ClashingExternDeclarations { | |||
return true; | |||
} | |||
let tcx = cx.tcx; | |||
if a == b || rustc_middle::ty::TyS::same_type(a, b) { | |||
if a == b { | |||
// All nominally-same types are structurally same, too. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: "nominally" here is potentially confusing, maybe good to adjust this to say "literally the same type" or similar?
I think for Clippy what we nearly always want is "equal ignoring regions". We sometimes use |
I agree that this PR looks fine. I didn't look much at the clippy changes - there's a lot of background there that I don't know what each of these instances are supposed to do. I think there are maybe two ways to go forward with this: 1) Move this function into clippy itself, so there isn't a behavior change (then later that can be amended to an erasing regions version) 2) just land this; I would feel better getting a sign off from a clippy maintainer first. But as @Mark-Simulacrum said, the tests are passing so this is probably fine If you go with option 1, then r=me with that changed. If you with option 2, r=me with clippy maintainer signoff. |
it ignored regions and constants in adts, but didn't do so for references or any other types. This seemed quite weird
Yeah, using
I am interpreting the ":+1:" by @flip1995 on #93290 (comment) as approval 😆 feel free to |
📌 Commit 7ebd48d has been approved by |
Yes. Since there are no test changes in Clippy, it doesn't seem like this introduced any regressions in Clippy. With "trivial" changes like this I usually just leave my 👍 and unsubscribe from the PR 😄
This is a good question, to which I don't have the answer to. But I don't think this is something that should block this PR. |
remove `TyS::same_type` This function ignored regions and constants in adts, but didn't do so for references or any other types. cc rust-lang#93148 (comment)
…askrgr Rollup of 7 pull requests Successful merges: - rust-lang#86374 (Enable combining `+crt-static` and `relocation-model=pic` on `x86_64-unknown-linux-gnu`) - rust-lang#91828 (Implement `RawWaker` and `Waker` getters for underlying pointers) - rust-lang#92021 (Eliminate duplicate codes of is_single_fp_element) - rust-lang#92584 (add rustc lint, warning when iterating over hashmaps 2) - rust-lang#93267 (implement a lint for suspicious auto trait impls) - rust-lang#93290 (remove `TyS::same_type`) - rust-lang#93436 (Update compiler_builtins to fix duplicate symbols in `armv7-linux-androideabi` rlib) Failed merges: r? `@ghost` `@rustbot` modify labels: rollup
remove `TyS::same_type` This function ignored regions and constants in adts, but didn't do so for references or any other types. cc rust-lang#93148 (comment)
This function ignored regions and constants in adts, but didn't do so for references or any other types. cc #93148 (comment)