Combine three generalizer implementations #111221

compiler-errors · 2023-05-04T21:21:50Z

This code is a bit delicate and there were subtle changes between them, so I'll leave inline comments where further inspection is needed.

Regarding this comment from #109813 -- "add tests triggering all codepaths: at least the combine and the const generalizer", can't really do that now, and I don't really know how we'd get a higher-ranked const error since non-lifetime binders doesn't really support for<const ..> (it errors out when you try to use it).

r? @lcnr

compiler/rustc_infer/src/infer/combine.rs

compiler/rustc_infer/src/infer/nll_relate/mod.rs

compiler-errors · 2023-05-04T21:25:22Z

compiler/rustc_infer/src/infer/generalize.rs

+                    }
+                }
+            }
+            ty::ConstKind::Unevaluated(ty::UnevaluatedConst { def, substs }) => {


The old behavior of the NLL generalizer did this:

ty::ConstKind::Unevaluated(..) if self.tcx().lazy_normalization() => Ok(a),

Not exactly sure what's more correct here. I guess we probably shouldn't be generalizing aliases? But in that case, we need to be emitting a const vid and an alias relate (or const equate) here, right?

We should only get ConstKind::Unevaluated with subst if generic_const_exprs is enabled afaik so this shouldnt affect stable I think. Generalizing alias substs seems wrong but I think creating an infer var is also wrong since if the alias is underneath a binder the infer var would not be able to name any of its placeholders when we later relate the original ty with the generalised ty. Probably just doesn't matter to try and make generalizing aliases work correctly in this PR since its already not great and doing it correctly seems hard xd

Specifically if we have:
for<'a> fn(<?0.0 as Trait<'a>>::Assoc) <: ?1.0
we generalize the fn ptr to get for<'a> fn(?2.0)
and then we end up relating for<'a> fn(<?0.0 as Trait<'a>>::Assoc) eq for<'a> fn(?2.0)
once the binders are instantiated we'll end up with fn(<?0.0 as Trait<'!a.1>>::Assoc) eq fn(?2.0)
the alias-relate(<?0.0 as Trait<'!a.1>>::Assoc, eq, ?2.0) is kinda messed up,

we cant instantiate ?2.0 with '!a.1 if the alias ends up normalizing to the trait argument because of universe checks

we cant just Err(NoSolution) as that's unsound in coherence since for<'a> fn(<?0.0 as Trait<'a>>::Assoc) eq ?1.0 should be allowed to end up successful with inferring ?1.0 = for<'a> fn(Foo<'a>) if that's how the alias ends up normalizing (not actually sure if the fact that this is lifetimes means this would play out differently, just sub it all for type placeholders I guess if so lol)

This whole thing just reminds me of the current "replace aliases with infer vars" hack in project and how it doesn't work for projections under binders.

I'm pretty sure that this just fundamentally cant work because of universes and actually we would need to be generalizing types with binders to an infer var (didnt realise this til lcnr said it, had just thought everything was doomed) and emitting a general relate obligation not alias relate, so that we'd have relate(for<'a> fn(<?0.0 as Trait<'a>>::Assoc, <:, ?1.0). We need alias-relate to be general purpose relate anyway to correctly handle occurs check and projections not being unsound in coherence (Foo<<?0 as Trait>::Assoc> eq ?0> would emit a relate instead of a Err(NoSolution))

converting types with binders (trait objects and fn ptrs) into infer vars during generalization would inhibit inference probably in ways that are backwards incompatible, no idea to what extent this would be backwards compatible (or put another way: if its realistically something that can be broken). So it may be that its not a viable solution and actually we just want to accept generalization being incomplete and make occurs check and universe check into ambiguity during coherence instead of NoSolution

I think we shouldn't change this behavior in this PR even if it's incomplete.

However, similar to the opaque branch above, super_relate_consts is already purely structural, so we can remove that match arm as its equivalent to super_relate_consts.

super_relate_consts is unfortuantely not structural. We are eagerly evaluating ty::UnevaluatedConst there. But I agree, it should be made purely structural.

super_relate_consts has a bunch of weird stuff going on that it probably shouldnt be doing. I'd be in favor of renaming to structurally_relate_consts even though super_relate_consts isnt very structural right now, it would also make it a bit more obvious that what super_relate_consts is doing is weird.

Renamed to structurally_relate_{tys,consts} but left a FIXME on structurally_relate_consts that says that it's not currently structural and that should be fixed.

compiler-errors · 2023-05-04T21:26:32Z

compiler/rustc_infer/src/infer/generalize.rs

+        Ok(g)
+    }
+
+    fn regions(


The old implementation of the NLL generalizer always generalized regions -- there was some comment:

// For now, we just always create a fresh region variable to // replace all the regions in the source type. In the main // type checker, we special case the case where the ambient // variance is `Invariant` and try to avoid creating a fresh // region variable, but since this comes up so much less in // NLL (only when users use `_` etc) it is much less // important. // // As an aside, since these new variables are created in // `self.universe` universe, this also serves to enforce the // universe scoping rules. // // FIXME(#54105) -- if the ambient variance is bivariant, // though, we may however need to check well-formedness or // risk a problem like #41677 again.

Also comment about bivariance, perhaps we should be setting needs_wf here too?

needs_wf is kind of a mess and mostly there to guide type inference during hir typeck. It is probably also necessary when equating user type annotations during nll typeck, but given that this happens inside of a query, the nll generalizer does not need add special wf obligations.

I believe that we should instead check that all locals in the mir body are well formed. But well 🤷 i guess lets keep the status quo in this PR.

compiler/rustc_infer/src/infer/generalize.rs

bors · 2023-05-05T08:34:46Z

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

compiler/rustc_infer/src/infer/generalize.rs

lcnr

realized that nll relate does not explicitly generalize const inference vars but instead relies on super_combine_consts which then generalizes const inference vars using the ordinary typeck generalizer which feels like it should cause bugs if that code is ever reachable?

a few nits then r=me.

compiler/rustc_infer/src/infer/generalize.rs

lcnr · 2023-05-09T13:23:59Z

compiler/rustc_infer/src/infer/generalize.rs

+        Ok(g)
+    }
+
+    fn regions(


needs_wf is kind of a mess and mostly there to guide type inference during hir typeck. It is probably also necessary when equating user type annotations during nll typeck, but given that this happens inside of a query, the nll generalizer does not need add special wf obligations.

I believe that we should instead check that all locals in the mir body are well formed. But well 🤷 i guess lets keep the status quo in this PR.

compiler/rustc_infer/src/infer/generalize.rs

lcnr · 2023-05-09T13:30:19Z

compiler/rustc_infer/src/infer/generalize.rs

+                    }
+                }
+            }
+            ty::ConstKind::Unevaluated(ty::UnevaluatedConst { def, substs }) => {


I think we shouldn't change this behavior in this PR even if it's incomplete.

However, similar to the opaque branch above, super_relate_consts is already purely structural, so we can remove that match arm as its equivalent to super_relate_consts.

compiler/rustc_infer/src/infer/generalize.rs

compiler/rustc_infer/src/infer/combine.rs

lcnr · 2023-05-09T13:47:20Z

@bors rollup=never this change is complex enough that i can imagine us missing something, so let's make this easier to bisect.

@bors try @rust-timer queue

lcnr · 2023-05-09T15:51:37Z

?bors

@bors try

bors · 2023-05-09T15:51:49Z

⌛ Trying commit 8d2ae13f8353f50f00a300d64124d3f26c5fc426 with merge 971d96e4613fd0cf06584ae3654679cbb360969e...

bors · 2023-05-09T18:06:49Z

☀️ Try build successful - checks-actions
Build commit: 971d96e4613fd0cf06584ae3654679cbb360969e (971d96e4613fd0cf06584ae3654679cbb360969e)

rust-timer · 2023-05-09T19:21:18Z

Finished benchmarking commit (971d96e4613fd0cf06584ae3654679cbb360969e): comparison URL.

Overall result: ❌ regressions - 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.

Next Steps: If you can justify the regressions found in this try perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please fix the regressions and do another perf run. If the next run shows neutral or positive results, the label will be automatically removed.

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

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

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

Max RSS (memory usage)

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

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: 658.646s -> 659.139s (0.07%)

lcnr · 2023-05-09T20:51:05Z

i think perf is good enough. 3 minimal regressions to incr-patched: println are irrelevant imo

compiler-errors · 2023-05-10T01:46:29Z

I think I addressed all the nits? I won't r=@lcnr until they take another quick look at the final diff, though no rush! 😄

bors · 2023-05-12T15:54:55Z

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

lcnr

some nits, r=me

lcnr · 2023-05-15T16:24:57Z

compiler/rustc_infer/src/infer/combine.rs

@@ -456,11 +442,11 @@ impl<'infcx, 'tcx> CombineFields<'infcx, 'tcx> {
    /// - `for_vid` is a "root vid"
    #[instrument(skip(self), level = "trace", ret)]
    fn generalize(


does that method still make sense? might be easier to just inline that into instantiate. Also I am not sure whether RelationDir is particularily useful, so maybe just using Variance is easier?

or well said differently, why do we prefer RelationDir over variance here but still use Variance in other parts of the code?

I'm just gonna remove RelationDir lol

compiler/rustc_infer/src/infer/generalize.rs

compiler/rustc_middle/src/ty/relate.rs

compiler-errors · 2023-05-15T19:47:40Z

@bors r=lcnr

bors · 2023-05-15T19:47:41Z

📌 Commit a2678e1 has been approved by lcnr

It is now in the queue for this repository.

bors · 2023-05-15T22:16:53Z

⌛ Testing commit a2678e1 with merge ea54255...

bors · 2023-05-16T01:07:33Z

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

rust-timer · 2023-05-16T04:17:20Z

Finished benchmarking commit (ea54255): comparison URL.

Overall result: ❌ regressions - ACTION NEEDED

Next Steps: If you can justify the regressions found in this perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please open an issue or create a new PR that fixes the regressions, add a comment linking to the newly created issue or PR, and then add the perf-regression-triaged label to this PR.

@rustbot label: +perf-regression
cc @rust-lang/wg-compiler-performance

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	0.5%	[0.3%, 0.8%]	11
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	0.5%	[0.3%, 0.8%]	11

Max RSS (memory usage)

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

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)	-	-	0
Regressions ❌ (secondary)	5.1%	[5.1%, 5.1%]	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: 644.092s -> 642.928s (-0.18%)

nnethercote · 2023-05-16T04:44:36Z

@compiler-errors: any idea what happened with bitmaps perf? It wasn't affected in the pre-merge perf run.

compiler-errors · 2023-05-16T04:45:46Z

No, I didn't really do anything functional in the nits/changes that lcnr requested. Just some renames and moving things around mostly.

rustbot assigned lcnr May 4, 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 May 4, 2023

compiler-errors commented May 4, 2023

View reviewed changes

jyn514 added the A-inference Area: Type inference label May 4, 2023

compiler-errors added T-types Relevant to the types team, which will review and decide on the PR/issue. A-type-system Area: Type system and removed T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. A-inference Area: Type inference labels May 5, 2023

BoxyUwU mentioned this pull request May 5, 2023

generalization is incomplete for aliases rust-lang/trait-system-refactor-initiative#8

Open

aliemjay reviewed May 6, 2023

View reviewed changes

compiler/rustc_infer/src/infer/generalize.rs Outdated Show resolved Hide resolved

rust-cloud-vms bot force-pushed the yeet-generalizer branch from 964ca17 to 8d2ae13 Compare May 8, 2023 03:53

lcnr reviewed May 9, 2023

View reviewed changes

This comment has been minimized.

Sign in to view

rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label May 9, 2023

This comment has been minimized.

Sign in to view

rustbot added perf-regression Performance regression. and removed S-waiting-on-perf Status: Waiting on a perf run to be completed. labels May 9, 2023

compiler-errors force-pushed the yeet-generalizer branch 2 times, most recently from 65d6ccf to 97fedf1 Compare May 10, 2023 01:45

rust-cloud-vms bot force-pushed the yeet-generalizer branch from 97fedf1 to 26db751 Compare May 12, 2023 18:45

This comment has been minimized.

Sign in to view

rust-cloud-vms bot force-pushed the yeet-generalizer branch from 26db751 to bd1b41c Compare May 12, 2023 19:31

lcnr reviewed May 15, 2023

View reviewed changes

compiler-errors added 6 commits May 15, 2023 16:40

Combine TypeGeneralizer and Generalizer

338e764

Simplify delegate

c270b0a

yeet ConstInferUnifier

4ce2123

Tweaks and a test

6d0b6c0

Rename super_relate_* to structurally_relate_*

41501c7

Replace RelationDir with Variance

a2678e1

rust-cloud-vms bot force-pushed the yeet-generalizer branch from bd1b41c to a2678e1 Compare May 15, 2023 17:06

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 May 15, 2023

bors added the merged-by-bors This PR was explicitly merged by bors. label May 16, 2023

bors merged commit ea54255 into rust-lang:master May 16, 2023

rustbot added this to the 1.71.0 milestone May 16, 2023

compiler-errors deleted the yeet-generalizer branch May 16, 2023 01:23

Combine three generalizer implementations #111221

Combine three generalizer implementations #111221

Conversation

compiler-errors commented May 4, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bors commented May 5, 2023

lcnr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lcnr commented May 9, 2023

This comment has been minimized.

lcnr commented May 9, 2023

bors commented May 9, 2023

bors commented May 9, 2023

This comment has been minimized.

rust-timer commented May 9, 2023

Overall result: ❌ regressions - ACTION NEEDED

lcnr commented May 9, 2023

compiler-errors commented May 10, 2023

bors commented May 12, 2023

This comment has been minimized.

lcnr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

compiler-errors commented May 15, 2023

bors commented May 15, 2023

bors commented May 15, 2023

bors commented May 16, 2023

rust-timer commented May 16, 2023

Overall result: ❌ regressions - ACTION NEEDED

nnethercote commented May 16, 2023

compiler-errors commented May 16, 2023

compiler-errors commented May 4, 2023 •

edited

Loading