Skip to content

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

lcnr
Copy link
Contributor

@lcnr lcnr commented May 21, 2025

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

@lcnr
Copy link
Contributor Author

lcnr commented May 21, 2025

@bors try

@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 May 21, 2025
bors added a commit to rust-lang-ci/rust that referenced this pull request May 21, 2025
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`
@bors
Copy link
Collaborator

bors commented May 21, 2025

⌛ Trying commit a7e2981 with merge 15e8bc6...

@bors
Copy link
Collaborator

bors commented May 21, 2025

☀️ Try build successful - checks-actions
Build commit: 15e8bc6 (15e8bc6907ab2b3623cb52acc726f4787458df64)

@compiler-errors
Copy link
Member

@craterbot check

@craterbot
Copy link
Collaborator

👌 Experiment pr-141352 created and queued.
🤖 Automatically detected try build 15e8bc6
🔍 You can check out the queue and this experiment's details.

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

@craterbot craterbot added S-waiting-on-crater Status: Waiting on a crater run to be completed. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 21, 2025
@craterbot
Copy link
Collaborator

🚧 Experiment pr-141352 is now running

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

@craterbot
Copy link
Collaborator

🎉 Experiment pr-141352 is completed!
📊 4 regressed and 5 fixed (635013 total)
📰 Open the full report.

⚠️ If you notice any spurious failure please add them to the denylist!
ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

@craterbot craterbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-crater Status: Waiting on a crater run to be completed. labels May 22, 2025
@compiler-errors
Copy link
Member

That dyn ToJs<...>: ToJs<_> example may be literally the only one that relies on inference here lol: https://crater-reports.s3.amazonaws.com/pr-141352/try%2315e8bc6907ab2b3623cb52acc726f4787458df64/reg/px-wsdom-javascript-0.0.5/log.txt

@lcnr lcnr added T-types Relevant to the types team, which will review and decide on the PR/issue. and removed T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels May 23, 2025
@lcnr
Copy link
Contributor Author

lcnr commented May 23, 2025

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

@rfcbot
Copy link
Collaborator

rfcbot commented May 23, 2025

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.

@rfcbot rfcbot added proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. labels May 23, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-types Relevant to the types team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants