Unify two for_each_relevant_impl methods - #159836
Conversation
This comment has been minimized.
This comment has been minimized.
|
fun divergence here 🤔 @rustbot author |
|
Reminder, once the PR becomes ready for a review, use |
| | ty::Never | ||
| | ty::Tuple(_) | ||
| | ty::UnsafeBinder(_) => { | ||
| if let Some(simp) = ty::fast_reject::simplify_type( |
There was a problem hiding this comment.
oh, that's existing, but we should really make this an unwrap. We shouldn't allow not having a simplified type here as that indicates a bug 🤔
| ty::Alias(ty::IsRigid::Yes, _) | ty::Placeholder(..) | ty::Error(_) => (), | ||
| // FIXME(-Znext-solver=no): Need to support aliases not marked as | ||
| // rigid for the old solver. | ||
| ty::Alias(ty::IsRigid::No, _) | ty::Param(_) | ty::Infer(ty::TyVar(_)) => (), |
There was a problem hiding this comment.
and that's why old and new impl diverge, we should never call for_each_relevant_impl for infer vars 🤔 would be good to keep TyVar as an ICE and handle it at its call sites
There was a problem hiding this comment.
for_each_relevant_impl is used a lot in diagnostics. Denying ty var would complicate them a bit.
There was a problem hiding this comment.
hmm, I do feel like listing applicable impls for ty vars is kind of meh 🤷 I guess we can actually just iterate all impls when used for a ty var and say how that should never be done in the happy path or sth?
There was a problem hiding this comment.
you mean an early return? we currently iterate all impls at the end, after matching self_ty.
fe41428 to
4a7503c
Compare
This comment has been minimized.
This comment has been minimized.
|
@bors try @rust-timer queue |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Unify two `for_each_relevant_impl` methods
|
@bors cancel |
|
❗ There is currently no auto build in progress on this PR. Hint: There is a pending try build on this PR. Maybe you meant to cancel it? You can do that using |
|
💔 Test for 7017c2c failed: CI. Failed job:
|
This comment has been minimized.
This comment has been minimized.
4a7503c to
7417740
Compare
|
@bors try @rust-timer queue |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Unify two `for_each_relevant_impl` methods
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
7417740 to
a070b70
Compare
|
Finished benchmarking commit (9951eb3): comparison URL. Overall result: ❌✅ regressions and improvements - please read:Benchmarking means the PR may be perf-sensitive. It's automatically marked not fit for rolling up. Overriding is possible but disadvised: it risks changing compiler perf. Next, please: If you can, justify the regressions found in this try perf run in writing along with @bors rollup=never rustc-perf Instruction countOur most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.
Max RSS (memory usage)Results (primary -1.4%, secondary 0.5%)A less reliable metric. May be of interest, but not used to determine the overall result above.
CyclesResults (primary -2.3%, secondary -1.2%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Binary sizeResults (primary -0.0%, secondary -0.0%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Bootstrap: 492.09s -> 490.67s (-0.29%) |
|
@bors r+ rollup=never |
This comment has been minimized.
This comment has been minimized.
What is this?This is an experimental post-merge analysis report that shows differences in test outcomes between the merged PR and its parent PR.Comparing fc0f51f (parent) -> 64af19d (this PR) Test differencesNo test diffs found Test dashboardRun cargo run --manifest-path src/ci/citool/Cargo.toml -- \
test-dashboard 64af19d44c63587455c76f484ce76261a9971e96 --output-dir test-dashboardAnd then open Job duration changes
How to interpret the job duration changes?Job durations can vary a lot, based on the actual runner instance |
|
Finished benchmarking commit (64af19d): comparison URL. Overall result: ❌✅ regressions and improvements - please read:Our benchmarks found a performance regression caused by this PR. Next Steps:
@rustbot label: +perf-regression Instruction countOur most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.
Max RSS (memory usage)Results (primary -1.0%, secondary -1.7%)A less reliable metric. May be of interest, but not used to determine the overall result above.
CyclesResults (primary -3.1%, secondary 0.1%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Binary sizeThis perf run didn't have relevant results for this metric. Bootstrap: 491.246s -> 492.106s (0.18%) |
View all comments
We have two similar methods on
tcxwith the same name. It's confusing.r? lcnr