Skip to content

Use the new solver in the impossible_predicates #136988

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.

Sign up for GitHub

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

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

Use the new solver in the impossible_predicates #136988

wants to merge 1 commit into from
+43 −9

Conversation

compiler-errors
Copy link
Member

@compiler-errors compiler-errors commented Feb 13, 2025 •
edited by lcnr
Loading

The old solver is unsound for many reasons. One of which was weaponized by @lcnr in #140212, where the old solver was incompletely considering a dyn vtable method to be impossible and replacing its vtable entry with a null value. This null function could be called post-mono.

The new solver is expected to be less incomplete due to its correct handling of higher-ranked aliases in relate. This PR switches the impossible_predicates query to use the new solver, which patches this UB.

r? lcnr

@compiler-errors
Copy link
Member Author

@bors try @rust-timer queue

@rust-timer

This comment has been minimized.

Sign in to view
@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. S-waiting-on-perf Status: Waiting on a perf run to be completed. labels Feb 13, 2025
bors added a commit to rust-lang-ci/rust that referenced this pull request Feb 13, 2025
@bors
Auto merge of rust-lang#136988 - compiler-errors:impossible_predicate…
6aac808 
…s, r=<try>

Use the new solver in the `impossible_predicates`

r? lcnr
@bors
Copy link
Collaborator

bors commented Feb 13, 2025

⌛ Trying commit 42565da with merge 6aac808...

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@bors
Copy link
Collaborator

bors commented Feb 13, 2025

☀️ Try build successful - checks-actions
Build commit: 6aac808 (6aac80825ffa245e49ce6e80bdcfdc433dbb3559)

@rust-timer

This comment has been minimized.

Sign in to view
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (6aac808): comparison URL.

Overall result: ❌✅ regressions and improvements - no action needed

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR may lead to changes in compiler perf.

@bors rollup=never
@rustbot label: -S-waiting-on-perf -perf-regression

Instruction count

This is the most reliable metric that we have; it was used to determine the overall result at the top of this comment. However, even this metric can sometimes exhibit noise.

mean range count
Regressions ❌
(primary)		 0.3% [0.3%, 0.3%] 1
Regressions ❌
(secondary)		 0.5% [0.5%, 0.6%] 2
Improvements ✅
(primary)		 - - 0
Improvements ✅
(secondary)		 -0.1% [-0.1%, -0.1%] 3
All ❌✅ (primary) 0.3% [0.3%, 0.3%] 1

Max RSS (memory usage)

Results (primary -1.6%, secondary -2.9%)

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)		 - - 0
Regressions ❌
(secondary)		 - - 0
Improvements ✅
(primary)		 -1.6% [-2.3%, -0.9%] 2
Improvements ✅
(secondary)		 -2.9% [-3.7%, -2.4%] 3
All ❌✅ (primary) -1.6% [-2.3%, -0.9%] 2

Cycles

Results (secondary 2.5%)

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)		 - - 0
Regressions ❌
(secondary)		 2.5% [2.5%, 2.5%] 1
Improvements ✅
(primary)		 - - 0
Improvements ✅
(secondary)		 - - 0
All ❌✅ (primary) - - 0

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 788.486s -> 790.527s (0.26%)
Artifact size: 347.76 MiB -> 347.72 MiB (-0.01%)

@rustbot rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Feb 14, 2025
@compiler-errors
Copy link
Member Author

Ok let me fix AFIDT first.

@rustbot author

@rustbot rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 14, 2025
@rustbot rustbot added the WG-trait-system-refactor The Rustc Trait System Refactor Initiative (-Znext-solver) label Feb 14, 2025
@compiler-errors
Copy link
Member Author

@bors try @rust-timer queue

(and crater)

@rust-timer

This comment has been minimized.

Sign in to view
@rustbot rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Mar 19, 2025
bors added a commit to rust-lang-ci/rust that referenced this pull request Mar 19, 2025
@bors
Auto merge of rust-lang#136988 - compiler-errors:impossible_predicate…
4c525d4 
…s, r=<try>

Use the new solver in the `impossible_predicates`

r? lcnr
@bors
Copy link
Collaborator

bors commented Mar 19, 2025

⌛ Trying commit 40269a1 with merge 4c525d4...

@bors
Copy link
Collaborator

bors commented Mar 19, 2025

☀️ Try build successful - checks-actions
Build commit: 4c525d4 (4c525d43052e921548191f82f6ed9e2f9e98dcb1)

@rust-timer

This comment has been minimized.

Sign in to view
@compiler-errors
Copy link
Member Author

@craterbot check

@craterbot
Copy link
Collaborator

👌 Experiment pr-136988 created and queued.
🤖 Automatically detected try build 4c525d4
🔍 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 removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. S-waiting-on-perf Status: Waiting on a perf run to be completed. labels Mar 19, 2025
@craterbot craterbot added the S-waiting-on-crater Status: Waiting on a crater run to be completed. label Mar 19, 2025
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (4c525d4): comparison URL.

Overall result: ❌✅ regressions and improvements - no action needed

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR may lead to changes in compiler perf.

@bors rollup=never
@rustbot label: -S-waiting-on-perf -perf-regression

Instruction count

This is the most reliable metric that we have; it was used to determine the overall result at the top of this comment. However, even this metric can sometimes exhibit noise.

mean range count
Regressions ❌
(primary)		 - - 0
Regressions ❌
(secondary)		 0.6% [0.2%, 1.2%] 4
Improvements ✅
(primary)		 - - 0
Improvements ✅
(secondary)		 -0.2% [-0.2%, -0.2%] 3
All ❌✅ (primary) - - 0

Max RSS (memory usage)

Results (primary 1.4%, secondary -2.5%)

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)		 1.4% [1.4%, 1.4%] 1
Regressions ❌
(secondary)		 - - 0
Improvements ✅
(primary)		 - - 0
Improvements ✅
(secondary)		 -2.5% [-3.2%, -2.2%] 4
All ❌✅ (primary) 1.4% [1.4%, 1.4%] 1

Cycles

Results (primary -1.5%, secondary 3.1%)

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)		 - - 0
Regressions ❌
(secondary)		 3.1% [2.7%, 3.5%] 3
Improvements ✅
(primary)		 -1.5% [-1.5%, -1.5%] 1
Improvements ✅
(secondary)		 - - 0
All ❌✅ (primary) -1.5% [-1.5%, -1.5%] 1

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 773.717s -> 773.018s (-0.09%)
Artifact size: 365.52 MiB -> 365.55 MiB (0.01%)

@craterbot
Copy link
Collaborator

🚧 Experiment pr-136988 is now running

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

@craterbot
Copy link
Collaborator

🎉 Experiment pr-136988 is completed!
📊 6 regressed and 5 fixed (600425 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 Mar 21, 2025
@wesleywiser
Copy link
Member

Checking in from compiler team triage, it looks like crater did find an ICE perhaps. I think this is probably waiting on the author to comment on the crater results.

@rustbot author

@rustbot rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 17, 2025
@rustbot
Copy link
Collaborator

rustbot commented Apr 17, 2025

Reminder, once the PR becomes ready for a review, use @rustbot ready.

@compiler-errors compiler-errors 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 Apr 23, 2025
@compiler-errors
Copy link
Member Author

@rfcbot fcp merge

@rfcbot
Copy link
Collaborator

rfcbot commented Apr 23, 2025 •
edited by oli-obk
Loading

Team member @compiler-errors 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 Apr 23, 2025
@compiler-errors
Copy link
Member Author

This was the last decision we made about utilizing the new solver in the compiler: #123594 (comment)

Going to start a types FCP to officially state that going forward, the @rust-lang/initiative-trait-system-refactor has the authority to enable the use of the new solver on stable without FCPs as long as t-types is pinged in the PR and the affected area is neither relevant for soundness nor backwards compatibility.

Given that this is a soundness fix, I think it warrants a re-FCP for this specific case.

@compiler-errors compiler-errors linked an issue Apr 23, 2025 that may be closed by this pull request
incompletely relating alias args is unsound during vtable creation #140212
Open
@lcnr
Copy link
Contributor

lcnr commented Apr 23, 2025

We're checking fully concrete goals in an empty environment. I believe that the new solver is able to handle this case well and certainly expect it to be more complete than the old solver. There should be far fewer, if any, cases, where we incorrectly return NoSolution even if the goal should actually hold.

This only avoids monomorphizing impossible functions, so it should not be user-visible.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@lcnr lcnr

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-author Status: This is awaiting some action (such as code changes or more information) from the author. T-types Relevant to the types 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
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

incompletely relating alias args is unsound during vtable creation
9 participants
@compiler-errors @rust-timer @bors @rust-log-analyzer @craterbot @wesleywiser @rustbot @rfcbot @lcnr