Skip to content
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

new solver: prefer trivial builtin impls #135639

Merged
merged 1 commit into from
Jan 18, 2025

Conversation

lqd
Copy link
Member

@lqd lqd commented Jan 17, 2025

As discussed on zulip, this PR:

  • adds a new BuiltinImplSource::Trivial source, and marks the Sized builtin impls as trivial
  • prefers these trivial builtin impls in merge_trait_candidates

The comments can likely be wordsmithed a bit better, and I stole was inspired by the old solver ones. Let me know how you want them improved.

When enabling the new solver for tests, 3 UI tests now pass:

  • regions/issue-26448-1.rs and its sibling regions/issue-26448-2.rs were rejected by the new solver but accepted by the old one
  • and issues/issue-42796.rs where the old solver emitted some overflow errors in addition to the expected error

(For some reason one of these tests is run-pass, but I can take care of that another day)

r? lcnr

@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. WG-trait-system-refactor The Rustc Trait System Refactor Initiative (-Znext-solver) labels Jan 17, 2025
@rustbot
Copy link
Collaborator

rustbot commented Jan 17, 2025

These commits modify the Cargo.lock file. Unintentional changes to Cargo.lock can be introduced when switching branches and rebasing PRs.

If this was unintentional then you should revert the changes before this PR is merged.
Otherwise, you can ignore this comment.

@lcnr
Copy link
Contributor

lcnr commented Jan 17, 2025

please add the following to at least one affected test

//@ revisions: current next
//@[next] compile-flags: -Znext-solver
//@ ignore-compare-mode-next-solver (explicit revisions)

r=me after nits

for now, only builtin `Sized` impls are tracked as being `Trivial`
@lqd lqd force-pushed the trivial-builtin-impls branch from d35df81 to 00844be Compare January 17, 2025 18:53
@lqd
Copy link
Member Author

lqd commented Jan 17, 2025

I added the explicit revisions to the two tests that didn't need blessing for the new solver (and fixed the run-pass I mentioned above), and were initially affected when inhibiting this very preference in the old solver.

@bors r=lcnr rollup

@bors
Copy link
Contributor

bors commented Jan 17, 2025

📌 Commit 00844be has been approved by lcnr

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 17, 2025
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Jan 17, 2025
new solver: prefer trivial builtin impls

As discussed [on zulip](https://rust-lang.zulipchat.com/#narrow/channel/364551-t-types.2Ftrait-system-refactor/topic/needs_help.3A.20trivial.20builtin.20impls), this PR:
- adds a new `BuiltinImplSource::Trivial` source, and marks the `Sized` builtin impls as trivial
- prefers these trivial builtin impls in `merge_trait_candidates`

The comments can likely be wordsmithed a bit better, and I ~stole~ was inspired by the old solver ones. Let me know how you want them improved.

When enabling the new solver for tests, 3 UI tests now pass:
- `regions/issue-26448-1.rs` and its sibling `regions/issue-26448-2.rs` were rejected by the new solver but accepted by the old one
- and `issues/issue-42796.rs` where the old solver emitted some overflow errors in addition to the expected error

(For some reason one of these tests is run-pass, but I can take care of that another day)

r? lcnr
bors added a commit to rust-lang-ci/rust that referenced this pull request Jan 17, 2025
…iaskrgr

Rollup of 7 pull requests

Successful merges:

 - rust-lang#134455 (cleanup promoteds move check)
 - rust-lang#135421 (Make tidy warn on unrecognized directives)
 - rust-lang#135611 (Remove unnecessary assertion for reference error)
 - rust-lang#135620 (ci: improve github action name)
 - rust-lang#135621 (Move some std tests to integration tests)
 - rust-lang#135639 (new solver: prefer trivial builtin impls)
 - rust-lang#135654 (add src/librustdoc and src/rustdoc-json-types to RUSTC_IF_UNCHANGED_ALLOWED_PATHS)

r? `@ghost`
`@rustbot` modify labels: rollup
bors added a commit to rust-lang-ci/rust that referenced this pull request Jan 18, 2025
…iaskrgr

Rollup of 6 pull requests

Successful merges:

 - rust-lang#134455 (cleanup promoteds move check)
 - rust-lang#135421 (Make tidy warn on unrecognized directives)
 - rust-lang#135611 (Remove unnecessary assertion for reference error)
 - rust-lang#135620 (ci: improve github action name)
 - rust-lang#135639 (new solver: prefer trivial builtin impls)
 - rust-lang#135654 (add src/librustdoc and src/rustdoc-json-types to RUSTC_IF_UNCHANGED_ALLOWED_PATHS)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 862a17c into rust-lang:master Jan 18, 2025
6 checks passed
@rustbot rustbot added this to the 1.86.0 milestone Jan 18, 2025
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Jan 18, 2025
Rollup merge of rust-lang#135639 - lqd:trivial-builtin-impls, r=lcnr

new solver: prefer trivial builtin impls

As discussed [on zulip](https://rust-lang.zulipchat.com/#narrow/channel/364551-t-types.2Ftrait-system-refactor/topic/needs_help.3A.20trivial.20builtin.20impls), this PR:
- adds a new `BuiltinImplSource::Trivial` source, and marks the `Sized` builtin impls as trivial
- prefers these trivial builtin impls in `merge_trait_candidates`

The comments can likely be wordsmithed a bit better, and I ~stole~ was inspired by the old solver ones. Let me know how you want them improved.

When enabling the new solver for tests, 3 UI tests now pass:
- `regions/issue-26448-1.rs` and its sibling `regions/issue-26448-2.rs` were rejected by the new solver but accepted by the old one
- and `issues/issue-42796.rs` where the old solver emitted some overflow errors in addition to the expected error

(For some reason one of these tests is run-pass, but I can take care of that another day)

r? lcnr
@lqd lqd deleted the trivial-builtin-impls branch January 18, 2025 15:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. WG-trait-system-refactor The Rustc Trait System Refactor Initiative (-Znext-solver)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants