more eagerly instantiate binders by lcnr · Pull Request #119849 · rust-lang/rust (original) (raw)

Conversation

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters

[ Show hidden characters]({{ revealButtonHref }})

lcnr

The old solver sometimes incorrectly used sub, change it to explicitly instantiate binders and use eq instead. While doing so I also moved the instantiation before the normalize calls. This caused some observable changes, will explain these inline. This PR therefore requires a crater run and an FCP.

r? types

@lcnr

@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

Jan 11, 2024

@lcnr lcnr added the needs-fcp

This change is insta-stable, so needs a completed FCP to proceed.

label

Jan 11, 2024

@bors

This comment was marked as outdated.

bors added a commit to rust-lang-ci/rust that referenced this pull request

Jan 11, 2024

@bors

…

eagerly instantiate binders to avoid relying on sub

The old solver sometimes incorrectly used sub, change it to explicitly instantiate binders and use eq instead. While doing so I also moved the instantiation before the normalize calls. This caused some observable changes, will explain these inline. This PR therefore requires a crater run and an FCP.

r? types

lcnr

lcnr

obligation.cause.span,
HigherRankedType,
candidate,
);

Choose a reason for hiding this comment

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

By instantiating before we normalize, we allow more projections to be replaced by infer vars if their normalization is ambiguous, e.g. for<'a> <?0 as Trait<'a>>::Assoc stays as is while <?0 as Trait<'!a>>::Assoc gets replaced with an inference variable ?term and result in a nested Projection(<?0 as Trait<'!a>>::Assoc, ?term) goal.

lcnr

obligation.cause.span,
HigherRankedType,
unnormalized_upcast_trait_ref,
);

Choose a reason for hiding this comment

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

same here, can allow additional normalization

lcnr

obligation.cause.span,
HigherRankedType,
self_ty_trait_ref,
);

Choose a reason for hiding this comment

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

and here

lcnr

Choose a reason for hiding this comment

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

and here

lcnr
fn build_expression<A: Scalar, B: Scalar, O: Scalar>(
) -> impl Fn(A::RefType<'_>, B::RefType<'_>) -> O {
cmp_eq

Choose a reason for hiding this comment

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

This previously compiled because of incompleteness when proving for<'a, 'b> cmp_eq<?0, ?1, ?2>: Fn(A::RefType<'a>, B::RefType<'b>) -> O

This constrained ?0 to A even though there could have been another type for which <?x as Scalar>::RefType<'a> eq <A as Scalar>::RefType<'a>> holds, this is related to #102048 and is fixed because we now normalize for<'a> <?x as Scalar>::RefType<'a> after instantiating the binder, causing it to be replaced with a fresh infer var.

Choose a reason for hiding this comment

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

I believe the (new) behavior is correct -- #108918 (comment).

I don't believe this test should have ever compiled because it relies on the incompleteness of not normalizing aliases with escaping bound regions.

Choose a reason for hiding this comment

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

Yes, should have stated so explicitly, this breakage is desired imo

// implied bound due to the unconstrained lifetime.
//
// The `<Q as AsyncQueryFunction<'?y>>::SendDb`: 'a` bound then results in an error
// during typeck.
pub fn get_async<'a>(&'a mut self) {

Choose a reason for hiding this comment

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

this one is interesting, have to figure out why this test previously compiled

Choose a reason for hiding this comment

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

this previously compiled as in match_poly_trait_ref we used sup which ends up using Sub with lhs and rhs flipped. Not flipping them results in the region infer var of the normalized self type to not be the root var. We then use this self type for the implied bounds, canonicalization of which does opportunistically resolve regions, so the resulting implied bound uses the semantically equal, but not structurally equal root infer var.

Computing the verify bounds for the alias outlives then does a structural equality check which now fails, as we never opportunistically resolve regions in outlives bounds.

The following diff fixes this:

diff --git a/compiler/rustc_infer/src/infer/outlives/obligations.rs b/compiler/rustc_infer/src/infer/outlives/obligations.rs index c0a99e5cc41..a17b099bf4f 100644 --- a/compiler/rustc_infer/src/infer/outlives/obligations.rs +++ b/compiler/rustc_infer/src/infer/outlives/obligations.rs @@ -62,6 +62,7 @@ use crate::infer::outlives::components::{push_outlives_components, Component}; use crate::infer::outlives::env::RegionBoundPairs; use crate::infer::outlives::verify::VerifyBoundCx; +use crate::infer::resolve::OpportunisticRegionResolver; use crate::infer::{ self, GenericKind, InferCtxt, RegionObligation, SubregionOrigin, UndoLog, VerifyBound, }; @@ -71,6 +72,7 @@ use rustc_middle::traits::query::NoSolution; use rustc_middle::ty::{self, GenericArgsRef, Region, Ty, TyCtxt, TypeVisitableExt}; use rustc_middle::ty::{GenericArgKind, PolyTypeOutlivesPredicate}; +use rustc_middle::ty::TypeFoldable; use rustc_span::DUMMY_SP; use smallvec::smallvec;

@@ -177,6 +179,7 @@ pub fn process_registered_region_obligations( .no_bound_vars() .expect("started with no bound vars, should end with no bound vars");

Choose a reason for hiding this comment

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

Does that diff result in any other differences?

Choose a reason for hiding this comment

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

See the diagnostic changes in #121743

@jackh726

Test changes overall look fine, other than gluon_salsa that I have to think more about.

Haven't really reviewed the code changes, but I'll do that soonish, since we have to wait for process anyways.

lcnr

@@ -2532,7 +2515,7 @@ impl<'tcx> SelectionContext<'_, 'tcx> {
nested.extend(
self.infcx
.at(&obligation.cause, obligation.param_env)
.sup(
.eq(

Choose a reason for hiding this comment

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

these changes are currently breaking, changed them to match the new solver for now (but both solvers should probably instantiate eagerly to allow instantiating hr trait objects when upcasting)

we need tests for all of this though 😁

Choose a reason for hiding this comment

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

Consequence of this is what? That we can't do:

trait Supertrait<'a, 'b> {}
trait Subtrait: for<'a, 'b> Supertrait<'a, 'b> {}
let upcast: &dyn for<'a> Supertrait<'a, 'a> = todo!() as &dyn Subtrait;

Because of higher-ranked eq?

Choose a reason for hiding this comment

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

Nope, the consequence is that it fixes a previously unknown unsoundness 🤣

#![feature(trait_upcasting)] trait Supertrait<'a, 'b> { fn cast(&self, x: &'a str) -> &'b str; }

trait Subtrait<'a, 'b>: Supertrait<'a, 'b> {}

impl<'a> Supertrait<'a, 'a> for () { fn cast(&self, x: &'a str) -> &'a str { x } } impl<'a> Subtrait<'a, 'a> for () {} fn unsound(x: &dyn for<'a> Subtrait<'a, 'a>) -> &dyn for<'a, 'b> Supertrait<'a, 'b> { x }

fn transmute<'a, 'b>(x: &'a str) -> &'b str { unsound(&()).cast(x) }

fn main() { let x; { let mut temp = String::from("hello there"); x = transmute(temp.as_str()); } println!("{x}"); }

@lcnr

bors added a commit to rust-lang-ci/rust that referenced this pull request

Jan 12, 2024

@bors

…

eagerly instantiate binders to avoid relying on sub

The old solver sometimes incorrectly used sub, change it to explicitly instantiate binders and use eq instead. While doing so I also moved the instantiation before the normalize calls. This caused some observable changes, will explain these inline. This PR therefore requires a crater run and an FCP.

r? types

@bors

@bors

☀️ Try build successful - checks-actions
Build commit: 21bc403 (21bc403d557a2516df70ea80fb19b94177beede5)

@lcnr

@rust-timer

This comment has been minimized.

@craterbot

@rust-timer

Finished benchmarking commit (21bc403): comparison URL.

Overall result: ❌✅ regressions and improvements - 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.5% [0.2%, 0.9%] 13
Regressions ❌ (secondary) 1.2% [0.3%, 1.5%] 7
Improvements ✅ (primary) - - 0
Improvements ✅ (secondary) -2.1% [-2.5%, -1.8%] 6
All ❌✅ (primary) 0.5% [0.2%, 0.9%] 13

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

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) 1.7% [1.7%, 1.7%] 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: 666.39s -> 665.793s (-0.09%)
Artifact size: 308.38 MiB -> 308.37 MiB (-0.00%)

@craterbot

🚧 Experiment pr-119849 is now running

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

@craterbot

@lcnr

compiler-errors

Choose a reason for hiding this comment

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

r=me when green

@compiler-errors

@bors

📌 Commit c8f0f17 has been approved by compiler-errors

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-author

Status: This is awaiting some action (such as code changes or more information) from the author.

labels

Mar 14, 2024

@bors

@bors

@lcnr lcnr deleted the eagerly-instantiate-binders branch

March 14, 2024 22:00

@rust-timer

Finished benchmarking commit (fd27e87): comparison URL.

Overall result: ❌✅ regressions and improvements - 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.7% [0.2%, 1.1%] 14
Regressions ❌ (secondary) 0.5% [0.4%, 1.0%] 7
Improvements ✅ (primary) - - 0
Improvements ✅ (secondary) -1.2% [-1.4%, -1.1%] 6
All ❌✅ (primary) 0.7% [0.2%, 1.1%] 14

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) 2.1% [2.1%, 2.1%] 1
Improvements ✅ (primary) -3.8% [-3.8%, -3.8%] 1
Improvements ✅ (secondary) - - 0
All ❌✅ (primary) -3.8% [-3.8%, -3.8%] 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: 668s -> 668.42s (0.06%)
Artifact size: 311.58 MiB -> 311.47 MiB (-0.04%)

bors added a commit to rust-lang-ci/rust that referenced this pull request

Mar 18, 2024

@bors

…r=

Eagerly instantiate closure/coroutine-like bounds with placeholders to deal with binders correctly

A follow-up to rust-lang#119849, however it aims to fix a different set of issues. Currently, we have trouble confirming goals where built-in closure/fnptr/coroutine signatures are compared against higher-ranked goals.

Currently, we don't support goals like for<'a> fn(&'a ()): Fn(&'a ()) because we don't expect the self type of goal to reference any bound regions from the goal, because we don't really know how to deal with the double binder of predicate + self type. However, this definitely can be reached (rust-lang#121653) -- and in fact, it results in post-mono errors in the case of rust-lang#112347 where the builtin type (e.g. a coroutine) is hidden behind a TAIT.

The proper fix here is to instantiate the goal before trying to extract the signature from the self type. See final two commits.

r? lcnr

@Kobzol

@lcnr I assume that the perf. hit was expected, given that this is a trait solver fix?

@lcnr

yeah. I think so. By being able to replace more associated types with inference vars, the trait solver has to do more work

@Kobzol

Ok, marking as triaged.

@rustbot label: +perf-regression-triaged

bors added a commit to rust-lang-ci/rust that referenced this pull request

Apr 2, 2024

@bors

…r=lcnr

Eagerly instantiate closure/coroutine-like bounds with placeholders to deal with binders correctly

A follow-up to rust-lang#119849, however it aims to fix a different set of issues. Currently, we have trouble confirming goals where built-in closure/fnptr/coroutine signatures are compared against higher-ranked goals.

Currently, we don't support goals like for<'a> fn(&'a ()): Fn(&'a ()) because we don't expect the self type of goal to reference any bound regions from the goal, because we don't really know how to deal with the double binder of predicate + self type. However, this definitely can be reached (rust-lang#121653) -- and in fact, it results in post-mono errors in the case of rust-lang#112347 where the builtin type (e.g. a coroutine) is hidden behind a TAIT.

The proper fix here is to instantiate the goal before trying to extract the signature from the self type. See final two commits.

r? lcnr

wip-sync pushed a commit to NetBSD/pkgsrc-wip that referenced this pull request

May 4, 2024

@he32

Pkgsrc changes:

Upstream chnages:

Version 1.78.0 (2024-05-02)

Language

Compiler

Target changes:

Refer to Rust's [platform support page][platform-support-doc] for more information on Rust's tiered platform support.

Libraries

Stabilized APIs

These APIs are now stable in const contexts:

Cargo

Misc

Compatibility Notes

Internal Changes

These changes do not affect any public interfaces of Rust, but they represent significant improvements to the performance or internals of rustc and related tools.

@lcnr lcnr mentioned this pull request

Sep 26, 2024

netbsd-srcmastr pushed a commit to NetBSD/pkgsrc that referenced this pull request

Oct 13, 2024

@he32

This is based on the pkgsrc-wip rust180 package, retaining the main pkgsrc changes as best as I could.

Pkgsrc changes:

Upstream chnages:

Version 1.80.1 (2024-08-08)

Version 1.80.0 (2024-07-25)

Language

Compiler

Libraries

Stabilized APIs

These APIs are now stable in const contexts:

Cargo

Rustdoc

Compatibility Notes

Internal Changes

These changes do not affect any public interfaces of Rust, but they represent significant improvements to the performance or internals of rustc and related tools.

Version 1.79.0 (2024-06-13)

Language

Compiler

Refer to Rust's [platform support page][platform-support-doc] for more information on Rust's tiered platform support.

Libraries

Stabilized APIs

These APIs are now stable in const contexts:

Cargo

Rustdoc

Misc

Compatibility Notes

Version 1.78.0 (2024-05-02)

Language

Compiler

Target changes:

Refer to Rust's [platform support page][platform-support-doc] for more information on Rust's tiered platform support.

Libraries

Stabilized APIs

These APIs are now stable in const contexts:

Cargo

Misc

Compatibility Notes

Internal Changes

These changes do not affect any public interfaces of Rust, but they represent significant improvements to the performance or internals of rustc and related tools.

Version 1.77.0 (2024-03-21)

Compiler

Refer to Rust's [platform support page][platform-support-doc] for more information on Rust's tiered platform support.

Libraries

Stabilized APIs

Cargo

Rustdoc

Misc

Internal Changes

These changes do not affect any public interfaces of Rust, but they represent significant improvements to the performance or internals of rustc and related tools.

Labels

A-testsuite

Area: The testsuite used to check the correctness of rustc

disposition-merge

This issue / PR is in PFCP or FCP with a disposition to merge it.

finished-final-comment-period

The final comment period is finished for this PR / Issue.

merged-by-bors

This PR was explicitly merged by bors.

needs-fcp

This change is insta-stable, so needs a completed FCP to proceed.

perf-regression

Performance regression.

perf-regression-triaged

The performance regression has been triaged.

S-waiting-on-bors

Status: Waiting on bors to run and complete tests. Bors will change the label on completion.

T-bootstrap

Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap)

T-types

Relevant to the types team, which will review and decide on the PR/issue.