-
Notifications
You must be signed in to change notification settings - Fork 13.4k
builtin dyn impl no guide inference #141352
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
Conversation
@bors try |
builtin dyn impl no guide inference cc rust-lang#141347 we can already slightly restrict this behavior in the old solver, so why not do so. Needs crater and an FCP. r? `@compiler-errors`
☀️ Try build successful - checks-actions |
@craterbot check |
👌 Experiment ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more |
🚧 Experiment ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more |
🎉 Experiment
|
That |
This PR disables the incomplete inference guidance if there's both a user-written and builtin trait object implementation in the old solver: trait Equals<T: ?Sized> {}
impl<T: ?Sized> Equals<T> for T {}
fn impls_equals<T: Equals<U> + ?Sized, U: ?Sized>() {}
fn main() {
// Two candidates:
// - via user impl, constrains `U` to `dyn Equals<u32>`
// - via builtin impl, constrains `U` to `u32`
//
// We previously constrained it to `dyn Equals<u32>`, with this change
// its ambiguous.
impls_equals::<dyn Equals<u32>, _>();
} This behavior should be unsound in coherence with the old solver, luckily we no longer use that. I don't think this guidance is desirable and explicitly do not do so in the new solver. Crater found a single regression: https://crates.io/crates/px-wsdom-javascript/0.0.5. We should open a PR fixing the regression before landing this change. @rfcbot fcp merge |
Team member @lcnr has proposed to merge this. The next step is review by the rest of the tagged team members: No concerns currently listed. Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up! See this document for info about what commands tagged team members can give me. |
🔔 This is now entering its final comment period, as per the review above. 🔔 |
a7e2981
to
bde63bb
Compare
r=me after fcp |
The final comment period, with a disposition to merge, as per the review above, is now complete. As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed. This will be merged soon. |
@bors r=compiler-errors rollup |
Rollup of 16 pull requests Successful merges: - #140969 (Allow initializing logger with additional tracing Layer) - #141352 (builtin dyn impl no guide inference) - #142046 (add Vec::peek_mut) - #142273 (tests: Minicore `extern "gpu-kernel"` feature test) - #142302 (Rework how the disallowed qualifier in function type diagnostics are generated) - #142405 (Don't hardcode the intrinsic return types twice in the compiler) - #142434 ( Pre-install JS dependencies in tidy Dockerfile) - #142439 (doc: mention that intrinsics should not be called in user code) - #142441 (Delay replacing escaping bound vars in `FindParamInClause`) - #142449 (Require generic params for const generic params) - #142452 (Remove "intermittent" wording from `ReadDir`) - #142459 (Remove output helper bootstrap) - #142460 (cleanup search graph impl) - #142461 (compiletest: Clarify that `--no-capture` is needed with `--verbose`) - #142475 (Add platform support docs & maintainers for *-windows-msvc) - #142480 (tests: Convert two handwritten minicores to add-core-stubs) r? `@ghost` `@rustbot` modify labels: rollup
cc #141347
we can already slightly restrict this behavior in the old solver, so why not do so. Needs crater and an FCP.
r? @compiler-errors