avoid generalization inside of aliases #119106

lcnr · 2023-12-19T07:23:08Z

The basic idea of this PR is that we don't generalize aliases when the instantiation could fail later on, either due to the occurs check or because of a universe error. We instead replace the whole alias with an inference variable and emit a nested AliasRelate goal. This AliasRelate then fully normalizes the alias before equating it with the inference variable, at which point the alias can be treated like any other rigid type.

We now treat aliases differently depending on whether they are rigid or not. To detect whether an alias is rigid we check whether NormalizesTo fails. While we already do so inside of AliasRelate anyways, also doing so when instantiating a query response would be both ugly/difficult and likely inefficient. To avoid that I change instantiate_and_apply_query_response to relate types completely structurally. This change generally removes a lot of annoying complexity, which is nice. It's implemented by adding a flag to Equate to change it to structurally handle aliases.

We currently always apply constraints from canonical queries right away. By providing all the necessary information to the canonical query, we can guarantee that instantiating the query response never fails, which further simplifies the implementation. This does add the invariant that any information which could cause instantiating type variables to fail must also be available inside of the query.

While it's acceptable for canonicalization to result in more ambiguity, we must not cause the solver to incompletely structurally relate aliases by erasing information. This means we have to be careful when merging universes during canonicalization. As we only generalize for type and const variables we have to make sure that anything nameable by such a type or const variable inside of the canonical query is also nameable outside of it. Because of this we both stop merging universes of existential variables when canonicalizing inputs, we put all uniquified regions into a higher universe which is not nameable by any type or const variable.

I will look into always replacing aliases with inference variables when generalizing in a later PR unless the alias references bound variables. This should both pretty much fix rust-lang/trait-system-refactor-initiative#4. This may allow us to merge the universes of existential variables again by changing generalize to not consider their universe when deciding whether to generalize aliases. This requires some additional non-trivial changes to alias-relate, so I am leaving that as future work.

Fixes rust-lang/trait-system-refactor-initiative#79. While it would be nice to decrement universe indices when existing a forall, that was surprisingly difficult and not necessary to fix this issue. I am really happy with the approach in this PR think it is the correct way forward to also fix the remaining cases of rust-lang/trait-system-refactor-initiative#8.

compiler-errors · 2023-12-21T15:39:59Z

gna mark this as author until it's out of draft?

@rustbot author

…ty-eagerly, r=oli-obk Construct closure type eagerly Construct the returned closure type *before* checking the body, in the same match as we were previously deducing the coroutine types based off of the closure kind. This simplifies some changes I'm doing in the async closure PR, and imo just seems easier to read (since we only need one match on closure kind, instead of two). There's no reason I can tell that we needed to create the closure type *after* the body was checked. ~~This also has the side-effect of making it so that the universe of the closure synthetic infer vars are lower than any infer vars that come from checking the body. We can also get rid of `next_root_ty_var` hack from closure checking (though in general we still need this, rust-lang#119106). cc `@lcnr` since you may care about this hack 😆~~ r? `@oli-obk`

…ty-eagerly, r=oli-obk Construct closure type eagerly Construct the returned closure type *before* checking the body, in the same match as we were previously deducing the coroutine types based off of the closure kind. This simplifies some changes I'm doing in the async closure PR, and imo just seems easier to read (since we only need one match on closure kind, instead of two). There's no reason I can tell that we needed to create the closure type *after* the body was checked. ~~This also has the side-effect of making it so that the universe of the closure synthetic infer vars are lower than any infer vars that come from checking the body. We can also get rid of `next_root_ty_var` hack from closure checking (though in general we still need this, rust-lang#119106). cc ``@lcnr`` since you may care about this hack 😆~~ r? ``@oli-obk``

…ty-eagerly, r=oli-obk Construct closure type eagerly Construct the returned closure type *before* checking the body, in the same match as we were previously deducing the coroutine types based off of the closure kind. This simplifies some changes I'm doing in the async closure PR, and imo just seems easier to read (since we only need one match on closure kind, instead of two). There's no reason I can tell that we needed to create the closure type *after* the body was checked. ~~This also has the side-effect of making it so that the universe of the closure synthetic infer vars are lower than any infer vars that come from checking the body. We can also get rid of `next_root_ty_var` hack from closure checking (though in general we still need this, rust-lang#119106). cc ```@lcnr``` since you may care about this hack 😆~~ r? ```@oli-obk```

Rollup merge of rust-lang#120031 - compiler-errors:construct-closure-ty-eagerly, r=oli-obk Construct closure type eagerly Construct the returned closure type *before* checking the body, in the same match as we were previously deducing the coroutine types based off of the closure kind. This simplifies some changes I'm doing in the async closure PR, and imo just seems easier to read (since we only need one match on closure kind, instead of two). There's no reason I can tell that we needed to create the closure type *after* the body was checked. ~~This also has the side-effect of making it so that the universe of the closure synthetic infer vars are lower than any infer vars that come from checking the body. We can also get rid of `next_root_ty_var` hack from closure checking (though in general we still need this, rust-lang#119106). cc ```@lcnr``` since you may care about this hack 😆~~ r? ```@oli-obk```

…ar-helper, r=lcnr Remove `next_root_ty_var` Uhh we seem to not have any test that relies on this anymore. Maybe due to the way we changed alias-relate or whatever. Removing this hack helper fn because rust-lang#119106 is the general solution. r? lcnr

Rollup merge of rust-lang#120037 - compiler-errors:remove-next-root-var-helper, r=lcnr Remove `next_root_ty_var` Uhh we seem to not have any test that relies on this anymore. Maybe due to the way we changed alias-relate or whatever. Removing this hack helper fn because rust-lang#119106 is the general solution. r? lcnr

rustbot · 2024-02-02T12:58:26Z

Some changes occurred to the core trait solver

cc @rust-lang/initiative-trait-system-refactor

Type relation code was changed

cc @compiler-errors, @lcnr

BoxyUwU · 2024-02-26T11:43:40Z

@bors r+ rollup=never (seems liable to conflict with stuff)

bors · 2024-02-26T11:43:43Z

📌 Commit 3b3514a has been approved by BoxyUwU

It is now in the queue for this repository.

bors · 2024-02-26T12:12:01Z

⌛ Testing commit 3b3514a with merge b8de591...

bors · 2024-02-26T14:42:53Z

☀️ Test successful - checks-actions
Approved by: BoxyUwU
Pushing b8de591 to master...

rust-timer · 2024-02-26T16:37:33Z

Finished benchmarking commit (b8de591): 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)	4.1%	[4.1%, 4.1%]	1
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-3.7%	[-4.2%, -3.2%]	2
All ❌✅ (primary)	4.1%	[4.1%, 4.1%]	1

Cycles

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

Binary size

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

Bootstrap: 653.235s -> 649.794s (-0.53%)
Artifact size: 311.20 MiB -> 311.12 MiB (-0.03%)

…e, r=lcnr Process alias-relate obligations in CoerceUnsized loop After rust-lang#119106, we now emit `AliasRelate` goals when relating `?0` and `Alias<T, ..>` in the new solver. In the ad-hoc `CoerceUnsized` selection loop, we now may have `AliasRelate` goals which must be processed to constrain type variables which are mentioned in other goals. --- For example, in the included test, we try to coerce `&<ManuallyDrop<T> as Deref>::Target` to `&dyn Foo`. This requires proving: * 1 `&<ManuallyDrop<T> as Deref>::Target: CoerceUnsized<&dyn Foo>` * 2 `<ManuallyDrop<T> as Deref>::Target alias-relate ?0` * 3 `?0: Unsize<dyn Foo>` * 4 `?0: Foo` * 5 `?0: Sized` If we don't process goal (2.) before processing goal (3.), then we hit ambiguity since `?0` is never constrained, and therefore we bail out, refusing to coerce the types. After processing (2.), we know `?0 := T`, and the rest of the goals can be processed normally.

Rollup merge of rust-lang#121702 - compiler-errors:coerce-alias-relate, r=lcnr Process alias-relate obligations in CoerceUnsized loop After rust-lang#119106, we now emit `AliasRelate` goals when relating `?0` and `Alias<T, ..>` in the new solver. In the ad-hoc `CoerceUnsized` selection loop, we now may have `AliasRelate` goals which must be processed to constrain type variables which are mentioned in other goals. --- For example, in the included test, we try to coerce `&<ManuallyDrop<T> as Deref>::Target` to `&dyn Foo`. This requires proving: * 1 `&<ManuallyDrop<T> as Deref>::Target: CoerceUnsized<&dyn Foo>` * 2 `<ManuallyDrop<T> as Deref>::Target alias-relate ?0` * 3 `?0: Unsize<dyn Foo>` * 4 `?0: Foo` * 5 `?0: Sized` If we don't process goal (2.) before processing goal (3.), then we hit ambiguity since `?0` is never constrained, and therefore we bail out, refusing to coerce the types. After processing (2.), we know `?0 := T`, and the rest of the goals can be processed normally.

rustbot assigned compiler-errors Dec 19, 2023

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 Dec 19, 2023

This comment has been minimized.

Sign in to view

lcnr force-pushed the decrement-universes branch from fe6db6e to 01ec1ab Compare December 19, 2023 08:10

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 Dec 21, 2023

compiler-errors mentioned this pull request Dec 25, 2023

-Znext-solver=globally failed to satisfy std::ops::Residual #119293

Closed

This was referenced Jan 16, 2024

Construct closure type eagerly #120031

Merged

Remove next_root_ty_var #120037

Merged

lcnr force-pushed the decrement-universes branch from 01ec1ab to 213a42f Compare January 29, 2024 11:55

rustbot added the WG-trait-system-refactor The Rustc Trait System Refactor Initiative label Jan 29, 2024

lcnr changed the title ~~[DRAFT] add a generalizer hack in new solver for reasons~~ do not create fresh infer vars when generalizing aliases Jan 29, 2024

This comment has been minimized.

Sign in to view

lcnr force-pushed the decrement-universes branch 3 times, most recently from eabd1a6 to feff26b Compare February 2, 2024 12:56

lcnr marked this pull request as ready for review February 2, 2024 12:58

lcnr mentioned this pull request Feb 2, 2024

remove sub_relations from the InferCtxt #119989

Merged

lcnr changed the title ~~do not create fresh infer vars when generalizing aliases~~ avoid generalization inside of aliases Feb 2, 2024

lcnr force-pushed the decrement-universes branch from feff26b to c683cac Compare February 2, 2024 14:55

add regression tests

1c264ca

lcnr force-pushed the decrement-universes branch from 9733e7a to 1c264ca Compare February 26, 2024 10:01

consider placeholders in fn term_is_fully_unconstrained

3b3514a

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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Feb 26, 2024

bors added the merged-by-bors This PR was explicitly merged by bors. label Feb 26, 2024

bors merged commit b8de591 into rust-lang:master Feb 26, 2024
12 checks passed

rustbot added this to the 1.78.0 milestone Feb 26, 2024

lcnr deleted the decrement-universes branch February 26, 2024 14:50

matthiaskrgr mentioned this pull request Feb 26, 2024

ICE: obligations.is_empty() #121649

Closed

compiler-errors mentioned this pull request Feb 27, 2024

Process alias-relate obligations in CoerceUnsized loop #121702

Merged

5225225 mentioned this pull request Mar 4, 2024

ICE when compiling aho-corasick-1.1.2 with next-resolver=globally #121178

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

avoid generalization inside of aliases #119106

avoid generalization inside of aliases #119106

lcnr commented Dec 19, 2023 •

edited

Loading

This comment has been minimized.

compiler-errors commented Dec 21, 2023

This comment has been minimized.

This comment has been minimized.

rustbot commented Feb 2, 2024

BoxyUwU commented Feb 26, 2024

bors commented Feb 26, 2024

bors commented Feb 26, 2024

bors commented Feb 26, 2024

rust-timer commented Feb 26, 2024

avoid generalization inside of aliases #119106

avoid generalization inside of aliases #119106

Conversation

lcnr commented Dec 19, 2023 • edited Loading

This comment has been minimized.

compiler-errors commented Dec 21, 2023

This comment has been minimized.

This comment has been minimized.

rustbot commented Feb 2, 2024

BoxyUwU commented Feb 26, 2024

bors commented Feb 26, 2024

bors commented Feb 26, 2024

bors commented Feb 26, 2024

rust-timer commented Feb 26, 2024

Overall result: no relevant changes - no action needed

Instruction count

Max RSS (memory usage)

Cycles

Binary size

lcnr commented Dec 19, 2023 •

edited

Loading