More robust debug assertions for Instance::resolve on built-in traits with non-standard trait items
#111279
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…ts with custom items
|
r? @TaKO8Ki (rustbot has picked a reviewer for you, use r? to override) |
rustbot
added
S-waiting-on-review
compiler-errors
changed the title
Instance::resolve on built-in traits with custom itemsInstance::resolve on built-in traits with non-standard trait items
|
I agree there is no need for a test. Those assertions are here to help rustc devs. |
bors
added
S-waiting-on-bors
matthiaskrgr
added a commit
to matthiaskrgr/rust
that referenced
this pull request
May 6, 2023
… r=cjgillot More robust debug assertions for `Instance::resolve` on built-in traits with non-standard trait items In rust-lang#111264, a user added a new item to the `Future` trait, but the code in [`resolve_associated_item`](https://doc.rust-lang.org/nightly/nightly-rustc/rustc_ty_utils/instance/fn.resolve_associated_item.html) implicitly assumes that the `Future` trait is defined with only one method (`Future::poll`) and treats the generator body as the implementation of that method. This PR adds some debug assertions to make sure that that new methods defined on `Future`/`Generator`/etc. don't accidentally resolve to the wrong item when they are added, and adds a helpful comment guiding a compiler dev (or curious `#![no_core]` user) to what must be done to support adding new associated items to these built-in implementations. I am open to discuss whether a test should be added, but I chose against it because I opted to make these `bug!()`s instead of, e.g., diagnostics or fatal errors. Arguably it doesn't need a test because it's not a bug that can be triggered by an end user, and internal-facing misuses of core kind of touch on rust-lang/compiler-team#620 -- however, I think the assertions I added in this PR are still a very useful way to make sure this bug doesn't waste debugging resources down the line. Fixes rust-lang#111264
bors
added a commit
to rust-lang-ci/rust
that referenced
this pull request
May 6, 2023
…iaskrgr Rollup of 7 pull requests Successful merges: - rust-lang#110577 (Use fulfillment to check `Drop` impl compatibility) - rust-lang#110610 (Add Terminator conversion from MIR to SMIR, part #1) - rust-lang#110985 (Fix spans in LLVM-generated inline asm errors) - rust-lang#110989 (Make the BUG_REPORT_URL configurable by tools ) - rust-lang#111167 (debuginfo: split method declaration and definition) - rust-lang#111230 (add hint for =< as <=) - rust-lang#111279 (More robust debug assertions for `Instance::resolve` on built-in traits with non-standard trait items) Failed merges: r? `@ghost` `@rustbot` modify labels: rollup
compiler-errors
added a commit
to compiler-errors/rust
that referenced
this pull request
May 8, 2023
…inators, r=compiler-errors Fix miscompilation when calling default methods on `Future` In rust-lang#111264 I discovered a lingering miscompilation when calling a default method on `Future` (none currently exist). rust-lang#111279 added a debug assertion, which sadly doesn't help much since to my knowledge stage0 is not built with them enabled, and it still doesn't make default methods work like they should. This PR fixes `resolve_instance` to resolve default methods on `Future` correctly, allowing library contributors to add `Future` combinators without running into ICEs or miscompilations. I've tested this as part of rust-lang#111347, but no test is included here (assuming that future methods include their own tests that would cover this sufficiently). r? `@compiler-errors`
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
In #111264, a user added a new item to the
Futuretrait, but the code inresolve_associated_itemimplicitly assumes that theFuturetrait is defined with only one method (Future::poll) and treats the generator body as the implementation of that method.This PR adds some debug assertions to make sure that that new methods defined on
Future/Generator/etc. don't accidentally resolve to the wrong item when they are added, and adds a helpful comment guiding a compiler dev (or curious#![no_core]user) to what must be done to support adding new associated items to these built-in implementations.I am open to discuss whether a test should be added, but I chose against it because I opted to make these
bug!()s instead of, e.g., diagnostics or fatal errors. Arguably it doesn't need a test because it's not a bug that can be triggered by an end user, and internal-facing misuses of core kind of touch on rust-lang/compiler-team#620 -- however, I think the assertions I added in this PR are still a very useful way to make sure this bug doesn't waste debugging resources down the line.Fixes #111264