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

Merged
merged 1 commit into from
Jun 14, 2025
Merged

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
@rfcbot rfcbot added final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. and removed proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. labels Jun 3, 2025
@rfcbot
Copy link
Collaborator

rfcbot commented Jun 3, 2025

🔔 This is now entering its final comment period, as per the review above. 🔔

@compiler-errors
Copy link
Member

r=me after fcp

@compiler-errors compiler-errors removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jun 11, 2025
@compiler-errors compiler-errors added the S-waiting-on-fcp Status: PR is in FCP and is awaiting for FCP to complete. label Jun 11, 2025
@rfcbot rfcbot added finished-final-comment-period The final comment period is finished for this PR / Issue. to-announce Announce this issue on triage meeting and removed final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. labels Jun 13, 2025
@rfcbot
Copy link
Collaborator

rfcbot commented Jun 13, 2025

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.

@lcnr
Copy link
Contributor Author

lcnr commented Jun 13, 2025

@bors r=compiler-errors rollup

@bors
Copy link
Collaborator

bors commented Jun 13, 2025

📌 Commit bde63bb has been approved by compiler-errors

It is now in the queue for this repository.

@bors bors added the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label Jun 13, 2025
bors added a commit that referenced this pull request Jun 14, 2025
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
@bors bors merged commit efc55fa into rust-lang:master Jun 14, 2025
10 checks passed
@rustbot rustbot added this to the 1.89.0 milestone Jun 14, 2025
rust-timer added a commit that referenced this pull request Jun 14, 2025
Rollup merge of #141352 - lcnr:no-builtin-preference, r=compiler-errors

builtin dyn impl no guide inference

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`
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. finished-final-comment-period The final comment period is finished for this PR / Issue. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. S-waiting-on-fcp Status: PR is in FCP and is awaiting for FCP to complete. T-types Relevant to the types team, which will review and decide on the PR/issue. to-announce Announce this issue on triage meeting
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants