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

Handle recursion limit for subtype and well-formed predicates #117754

Merged
merged 1 commit into from Dec 2, 2023

Conversation

matthewjasper
Copy link
Contributor

Adds a recursion limit check for subtype predicates and well-formed predicates.
-Ztrait-solver=next currently panics with unimplemented for these cases.
These cases are arguably bugs in the occurs check but:

  • I could not find a simple way to fix the occurs check
  • There should still be a recursion limit check to prevent hangs anyway.

closes #117151

r? types

@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 Nov 9, 2023
@compiler-errors
Copy link
Member

I think this makes sense but @lcnr knows overflow handling better so passing to them

r? lcnr

@rustbot rustbot assigned lcnr and unassigned compiler-errors Nov 16, 2023
// Test that generalize handles variables that have a subtype relation to an
// already resolved value, but not an equality relation.
// Found when considering fixes to #117151
// check-pass
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test feels a bit :/ currently the way we generalize higher ranked types is wrong when subtyping.

we currently do the following (using x for the infer var in x: None<_>):

  • emit (and stall on) a Subtype(x <: y) obligation
  • the w = a assignment sets x to for<'a> fn(&'a ()), emits a Subtype(y <: z) obligation and generalizes z to fn(&'0 ()).
  • we then prove Subtype(x <: y) setting y to for<'a> fn(&'a ()) and then prove Subtype(y <: z) which is now trivial as both types are rigid.

If we were to prove the Subtype obligations before generalizing z we'd get an incorrect error, e.g:

fn main() {
    let mut x = None;
    let y = x;
    let z = Default::default();
    let mut w = (&mut x, z, z);
    let mut temp = None::<for<'a> fn(&'a ())>;
    w.0 = &mut temp;
    w.1 = y;
    w.2 = None::<fn(&'static ())>; //~ ERROR mismatched types
}

It feels like this test mostly tests a very specific impl detail of our subtyping algorithm where we do not try to eagerly make progress on previous Subtype obligations while relating.

I think the explanation here is somewhat misleading. The reason this works is because generalization does not care about subtype relations (apart from the occurs check). I would instead use sth like "Subtyping of inference variables is currently handled via delayed Subtype obligations. We do not immediately process them after constraining one of the involved inference variables. We'd otherwise incorrectly constrain the type of z as generalization does not handle binders correctly".

@lcnr lcnr added the T-types Relevant to the types team, which will review and decide on the PR/issue. label Nov 21, 2023
@lcnr
Copy link
Contributor

lcnr commented Nov 21, 2023

r=me on the impl changes, would like to improve the test explanation however

@bors
Copy link
Contributor

bors commented Nov 23, 2023

☔ The latest upstream changes (presumably #118120) made this pull request unmergeable. Please resolve the merge conflicts.

@matthewjasper matthewjasper force-pushed the subtype-overflow branch 2 times, most recently from e164c54 to b5da932 Compare December 1, 2023 17:00
@rust-log-analyzer

This comment has been minimized.

@lcnr
Copy link
Contributor

lcnr commented Dec 1, 2023

@bors r+ rollup

thanks 👍

@bors
Copy link
Contributor

bors commented Dec 1, 2023

📌 Commit 942e939 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 Dec 1, 2023
@bors
Copy link
Contributor

bors commented Dec 2, 2023

⌛ Testing commit 942e939 with merge 0919ad1...

@bors
Copy link
Contributor

bors commented Dec 2, 2023

☀️ Test successful - checks-actions
Approved by: lcnr
Pushing 0919ad1 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Dec 2, 2023
@bors bors merged commit 0919ad1 into rust-lang:master Dec 2, 2023
12 checks passed
@rustbot rustbot added this to the 1.76.0 milestone Dec 2, 2023
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (0919ad1): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

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

Max RSS (memory usage)

Results

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)
-2.5% [-2.5%, -2.5%] 1
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) -2.5% [-2.5%, -2.5%] 1

Cycles

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

Binary size

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

Bootstrap: 671.261s -> 672.207s (0.14%)
Artifact size: 313.93 MiB -> 313.91 MiB (-0.01%)

@matthewjasper matthewjasper deleted the subtype-overflow branch December 4, 2023 11:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged-by-bors This PR was explicitly merged by bors. 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. 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.

Compiler Hang Issue when Using the iso_un_option Function
7 participants