Move placeholder handling to a proper preprocessing step #140466

amandasystems · 2025-04-29T15:24:31Z

This commit breaks out the logic of placheolder rewriting into its own preprocessing step. It's one of the more boring
parts of #130227.

The only functional change from this is that the preprocessing step (where extra r: 'static constraints are added) is performed upstream of Polonius legacy, finally affecting Polonius. That is mostly a by-product, though.

This should be reviewable by anyone in the compiler team, so
r? rust-lang/compiler

compiler/rustc_borrowck/src/eliminate_placeholders.rs

lcnr · 2025-04-30T14:30:41Z

r? lcnr

This commit breaks out the logic of placheolder rewriting into its own preprocessing step. It's one of the more boring parts of rust-lang#130227. The only functional change from this is that the preprocessing step (where extra `r: 'static` constraints are added) is performed upstream of Polonius legacy, finally affecting Polonius. That is mostly a by-product, though.

amandasystems · 2025-05-06T17:19:12Z

Ok; revised SCC annotations have landed and I think I addressed all of your concerns so far @lcnr?

compiler/rustc_borrowck/src/eliminate_placeholders.rs

compiler/rustc_borrowck/src/region_infer/mod.rs

amandasystems · 2025-05-15T20:09:45Z

@lcnr Fixed all the code review stuff now, I think! I amended it into the second commit.

compiler/rustc_borrowck/src/eliminate_placeholders.rs

lcnr · 2025-05-22T09:42:47Z

compiler/rustc_borrowck/src/eliminate_placeholders.rs

+                sup: annotation.representative.rvid(),
+                sub: fr_static,
+                category: ConstraintCategory::IllegalUniverse,
+                locations: Locations::All(rustc_span::DUMMY_SP),


could we track a span related to the placeholder in the scc annotation and use that here?

Sure, I guess. That might be useful for error reporting in #140737, but this PR still relies on the old error reporting machinery.

I'm also not sure what would be a good span. Where the constraint is added? Where it's defined?

There is a cool variant of #140737 where we essentially run the blame search to find a blame span, put that in here, and then later stop constraint search at the outlives-static constraint and just report its span.

This requires quite a lot more refactoring though.

compiler/rustc_borrowck/src/eliminate_placeholders.rs

amandasystems · 2025-05-26T15:18:32Z

@lcnr I think I've addressed all of those now! Except for the one that should probably have a PR of its own or be merged into #140737

lcnr

a few more comment nits, and then finally r=me ☠️ sry

lcnr · 2025-05-29T12:04:25Z

compiler/rustc_borrowck/src/eliminate_placeholders.rs

+//! This logic is provisional and should be removed once the trait
+//! solver can handle this kind of constraint.


not really too relevant for this PR itself. With my current information I think that this will actually be really unlikely 😅 handling opaque types will need to propagate placeholders to the end of typeck

Might make sense to have a few meetings to discuss the longterm plan here again to make sure we're working towards a reachable goal 😅 forgot to talk about that part during the AllHands :<

I’d say ”was too busy” but yes! More! Meetings!!!!

lcnr · 2025-05-29T12:06:52Z

compiler/rustc_borrowck/src/eliminate_placeholders.rs

+    /// The smallest universe nameable from this SCC.
+    /// It is the smallest of all the largest nameable universes
+    /// of any region reachable from it.


Suggested change

/// The smallest universe nameable from this SCC.

/// It is the smallest of all the largest nameable universes

/// of any region reachable from it.

/// The largest universe nameable from this SCC.

/// It is the smallest nameable universes of all

/// existential regions reachable from it.

lcnr · 2025-05-29T12:07:44Z

compiler/rustc_borrowck/src/eliminate_placeholders.rs

+    }
+
+    /// Returns `true` if during the annotated SCC reaches a placeholder
+    /// with a universe larger than the smallest reachable one, `false` otherwise.


Suggested change

/// with a universe larger than the smallest reachable one, `false` otherwise.

/// with a universe larger than the smallest nameable universe of any

/// reachable existential region.

lcnr · 2025-05-29T12:09:04Z

compiler/rustc_borrowck/src/eliminate_placeholders.rs

+    }
+
+    /// Determine if the tracked universes of the two SCCs are compatible.
+    pub(crate) fn universe_compatible_with(&self, other: Self) -> bool {


what does this check? self.max_nameable_universe().can_name(other.max_nameable_universe()) seems like a very weird check 🤔

okay, so it's used in eval_outlives and... just silences errors if the other itself already has a placeholder error?

what is the impact of only checking self.max_nameable_universe().can_name(other.max_placeholder_universe_reached)?

This logic is carefully written to precisely mirror what was there previously. It at some point made sense to me but no longer does. It seemed fine until I renamed the functions to better descriptions at which point it, as you say, sounds preposterous.

I’ll spend some time with the thinking cap about this and get back to you.

But IIRC the impact is ”you get very few spurious errors, for some reason”

Ah yes, this is the logic on current master:

/// Returns `true` if all the elements in the value of `scc_b` are nameable /// in `scc_a`. Used during constraint propagation, and only once /// the value of `scc_b` has been computed. fn universe_compatible(&self, scc_b: ConstraintSccIndex, scc_a: ConstraintSccIndex) -> bool { let a_annotation = self.scc_annotations[scc_a]; let b_annotation = self.scc_annotations[scc_b]; let a_universe = a_annotation.min_universe(); // If scc_b's declared universe is a subset of // scc_a's declared universe (typically, both are ROOT), then // it cannot contain any problematic universe elements. if a_universe.can_name(b_annotation.min_universe()) { return true; } // Otherwise, there can be no placeholder in `b` with a too high // universe index to name from `a`. a_universe.can_name(b_annotation.max_placeholder_universe_reached) }

I mean, I wrote that, but that in turn was similarly a reconstruction of something horribly complicated IIRC

compiler/rustc_borrowck/src/eliminate_placeholders.rs

lcnr · 2025-05-29T12:13:40Z

compiler/rustc_borrowck/src/eliminate_placeholders.rs

+/// This code is a stop-gap measure in preparation for the future trait solver.
+///
+/// Every constraint added by this method is an internal `IllegalUniverse` constraint.
+pub(crate) fn rewrite_higher_kinded_outlives_as_constraints<'tcx>(


vibe: a very specific method name for a method that actually just computes the scc, maybe

Suggested change

pub(crate) fn rewrite_higher_kinded_outlives_as_constraints<'tcx>(

pub(crate) fn compute_scc_applying_placeholder_outlives_constraints<'tcx>(

See, this is why someone who’s been living inside her own head for a year needs code review

lcnr · 2025-05-29T12:17:50Z

compiler/rustc_borrowck/src/region_infer/mod.rs

            RegionVariableOrigin::Nll(origin) => origin,
            _ => NllRegionVariableOrigin::Existential { from_forall: false },
        };

-        Self { origin, universe, external_name: None }
+        Self { origin, universe: rv_info.universe, external_name: None }


vibe: we now immediately set the external_name in the function which creates the definitions. It's probably a lot clearer to just inline RegionDefinition::new in that fn

Co-authored-by: lcnr <rust@lcnr.de>

rustbot · 2025-05-31T06:49:36Z

⚠️ Warning ⚠️

There are issue links (such as #123) in the commit messages of the following commits.
Please remove them as they will spam the issue with references to the commit.
- dbc6b67
This PR is based on an upstream commit that is 29 days old.

It's recommended to update your branch according to the rustc-dev-guide.

rustbot assigned estebank Apr 29, 2025

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 Apr 29, 2025

lcnr reviewed Apr 30, 2025

View reviewed changes

compiler/rustc_borrowck/src/eliminate_placeholders.rs Outdated Show resolved Hide resolved

rustbot assigned lcnr and unassigned estebank Apr 30, 2025

amandasystems force-pushed the move-to-preprocessing-step branch from 8b2cab2 to af75e13 Compare May 2, 2025 08:53

amandasystems changed the title ~~[WIP] Move placeholder handling to a proper preprocessing step~~ Move placeholder handling to a proper preprocessing step May 2, 2025

lqd mentioned this pull request May 5, 2025

Scalable Polonius support on nightly rust-lang/rust-project-goals#118

Open

18 tasks

amandasystems mentioned this pull request May 7, 2025

[WIP] Region inference: Use outlives-static constraints in constraint search #140737

Open

lcnr reviewed May 15, 2025

View reviewed changes

compiler/rustc_borrowck/src/eliminate_placeholders.rs Outdated Show resolved Hide resolved

lcnr reviewed May 15, 2025

View reviewed changes

compiler/rustc_borrowck/src/eliminate_placeholders.rs Outdated Show resolved Hide resolved

lcnr reviewed May 15, 2025

View reviewed changes

compiler/rustc_borrowck/src/region_infer/mod.rs Outdated Show resolved Hide resolved

amandasystems force-pushed the move-to-preprocessing-step branch from af75e13 to ef9cb23 Compare May 15, 2025 20:05

lcnr reviewed May 22, 2025

View reviewed changes

Use an enum for SCC representatives, plus other code review

b1e4ca5

amandasystems force-pushed the move-to-preprocessing-step branch from ef9cb23 to b1e4ca5 Compare May 26, 2025 15:18

This comment has been minimized.

Sign in to view

lcnr reviewed May 29, 2025

View reviewed changes

Update compiler/rustc_borrowck/src/eliminate_placeholders.rs

0fff598

Co-authored-by: lcnr <rust@lcnr.de>

		//! This logic is provisional and should be removed once the trait
		//! solver can handle this kind of constraint.

	/// with a universe larger than the smallest reachable one, `false` otherwise.
	/// with a universe larger than the smallest nameable universe of any
	/// reachable existential region.

	pub(crate) fn rewrite_higher_kinded_outlives_as_constraints<'tcx>(
	pub(crate) fn compute_scc_applying_placeholder_outlives_constraints<'tcx>(

Move placeholder handling to a proper preprocessing step #140466

Are you sure you want to change the base?

Move placeholder handling to a proper preprocessing step #140466

Conversation

amandasystems commented Apr 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

lcnr commented Apr 30, 2025

Uh oh!

amandasystems commented May 6, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

amandasystems commented May 15, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

amandasystems May 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

This comment has been minimized.

amandasystems commented May 26, 2025

Uh oh!

lcnr left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lcnr May 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rustbot commented May 31, 2025

Uh oh!

Uh oh!

amandasystems commented Apr 29, 2025 •

edited

Loading

amandasystems May 26, 2025 •

edited

Loading

lcnr May 29, 2025 •

edited

Loading