-
Notifications
You must be signed in to change notification settings - Fork 12.5k
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
more eagerly instantiate binders #119849
more eagerly instantiate binders #119849
Conversation
@bors try |
This comment was marked as outdated.
This comment was marked as outdated.
…<try> 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
obligation.cause.span, | ||
HigherRankedType, | ||
candidate, | ||
); |
There was a problem hiding this comment.
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.
obligation.cause.span, | ||
HigherRankedType, | ||
unnormalized_upcast_trait_ref, | ||
); |
There was a problem hiding this comment.
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
obligation.cause.span, | ||
HigherRankedType, | ||
self_ty_trait_ref, | ||
); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
and here
} | ||
}) | ||
.map_err(|_| ()) | ||
); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
and here
c010b64
to
23e69c0
Compare
|
||
fn build_expression<A: Scalar, B: Scalar, O: Scalar>( | ||
) -> impl Fn(A::RefType<'_>, B::RefType<'_>) -> O { | ||
cmp_eq |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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) { |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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");
+ let (sup_type, sub_region) = (sup_type, sub_region).fold_with(&mut OpportunisticRegionResolver::new(self));
debug!(?sup_type, ?sub_region, ?origin);
let outlives = &mut TypeOutlives::new(
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
Test changes overall look fine, other than Haven't really reviewed the code changes, but I'll do that soonish, since we have to wait for process anyways. |
@@ -2532,7 +2515,7 @@ impl<'tcx> SelectionContext<'_, 'tcx> { | |||
nested.extend( | |||
self.infcx | |||
.at(&obligation.cause, obligation.param_env) | |||
.sup( | |||
.eq( |
There was a problem hiding this comment.
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 😁
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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}");
}
@bors try |
…<try> 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
☀️ Try build successful - checks-actions |
@rust-timer build 21bc403 @craterbot check |
This comment has been minimized.
This comment has been minimized.
👌 Experiment ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more |
Finished benchmarking commit (21bc403): comparison URL. Overall result: ❌✅ regressions and improvements - ACTION NEEDEDBenchmarking 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 @bors rollup=never Instruction countThis is a highly reliable metric that was used to determine the overall result at the top of this comment.
Max RSS (memory usage)ResultsThis 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.
CyclesResultsThis 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.
Binary sizeThis benchmark run did not return any relevant results for this metric. Bootstrap: 666.39s -> 665.793s (-0.09%) |
🚧 Experiment ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more |
🎉 Experiment
|
650afe8
to
323069f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
r=me when green
@bors r+ |
☀️ Test successful - checks-actions |
Finished benchmarking commit (fd27e87): comparison URL. Overall result: ❌✅ regressions and improvements - ACTION NEEDEDNext Steps: If you can justify the regressions found in this perf run, please indicate this with @rustbot label: +perf-regression Instruction countThis is a highly reliable metric that was used to determine the overall result at the top of this comment.
Max RSS (memory usage)ResultsThis 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.
CyclesThis benchmark run did not return any relevant results for this metric. Binary sizeThis benchmark run did not return any relevant results for this metric. Bootstrap: 668s -> 668.42s (0.06%) |
…r=<try> 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
@lcnr I assume that the perf. hit was expected, given that this is a trait solver fix? |
yeah. I think so. By being able to replace more associated types with inference vars, the trait solver has to do more work |
Ok, marking as triaged. @rustbot label: +perf-regression-triaged |
…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
Pkgsrc changes: * Adapt checksums and patches, some have beene intregrated upstream. Upstream chnages: Version 1.78.0 (2024-05-02) =========================== Language -------- - [Stabilize `#[cfg(target_abi = ...)]`] (rust-lang/rust#119590) - [Stabilize the `#[diagnostic]` namespace and `#[diagnostic::on_unimplemented]` attribute] (rust-lang/rust#119888) - [Make async-fn-in-trait implementable with concrete signatures] (rust-lang/rust#120103) - [Make matching on NaN a hard error, and remove the rest of `illegal_floating_point_literal_pattern`] (rust-lang/rust#116284) - [static mut: allow mutable reference to arbitrary types, not just slices and arrays] (rust-lang/rust#117614) - [Extend `invalid_reference_casting` to include references casting to bigger memory layout] (rust-lang/rust#118983) - [Add `non_contiguous_range_endpoints` lint for singleton gaps after exclusive ranges] (rust-lang/rust#118879) - [Add `wasm_c_abi` lint for use of older wasm-bindgen versions] (rust-lang/rust#117918) This lint currently only works when using Cargo. - [Update `indirect_structural_match` and `pointer_structural_match` lints to match RFC] (rust-lang/rust#120423) - [Make non-`PartialEq`-typed consts as patterns a hard error] (rust-lang/rust#120805) - [Split `refining_impl_trait` lint into `_reachable`, `_internal` variants] (rust-lang/rust#121720) - [Remove unnecessary type inference when using associated types inside of higher ranked `where`-bounds] (rust-lang/rust#119849) - [Weaken eager detection of cyclic types during type inference] (rust-lang/rust#119989) - [`trait Trait: Auto {}`: allow upcasting from `dyn Trait` to `dyn Auto`] (rust-lang/rust#119338) Compiler -------- - [Made `INVALID_DOC_ATTRIBUTES` lint deny by default] (rust-lang/rust#111505) - [Increase accuracy of redundant `use` checking] (rust-lang/rust#117772) - [Suggest moving definition if non-found macro_rules! is defined later] (rust-lang/rust#121130) - [Lower transmutes from int to pointer type as gep on null] (rust-lang/rust#121282) Target changes: - [Windows tier 1 targets now require at least Windows 10] (rust-lang/rust#115141) - [Enable CMPXCHG16B, SSE3, SAHF/LAHF and 128-bit Atomics in tier 1 Windows] (rust-lang/rust#120820) - [Add `wasm32-wasip1` tier 2 (without host tools) target] (rust-lang/rust#120468) - [Add `wasm32-wasip2` tier 3 target] (rust-lang/rust#119616) - [Rename `wasm32-wasi-preview1-threads` to `wasm32-wasip1-threads`] (rust-lang/rust#122170) - [Add `arm64ec-pc-windows-msvc` tier 3 target] (rust-lang/rust#119199) - [Add `armv8r-none-eabihf` tier 3 target for the Cortex-R52] (rust-lang/rust#110482) - [Add `loongarch64-unknown-linux-musl` tier 3 target] (rust-lang/rust#121832) Refer to Rust's [platform support page][platform-support-doc] for more information on Rust's tiered platform support. Libraries --------- - [Bump Unicode to version 15.1.0, regenerate tables] (rust-lang/rust#120777) - [Make align_offset, align_to well-behaved in all cases] (rust-lang/rust#121201) - [PartialEq, PartialOrd: document expectations for transitive chains] (rust-lang/rust#115386) - [Optimize away poison guards when std is built with panic=abort] (rust-lang/rust#100603) - [Replace pthread `RwLock` with custom implementation] (rust-lang/rust#110211) - [Implement unwind safety for Condvar on all platforms] (rust-lang/rust#121768) - [Add ASCII fast-path for `char::is_grapheme_extended`] (rust-lang/rust#121138) Stabilized APIs --------------- - [`impl Read for &Stdin`] (https://doc.rust-lang.org/stable/std/io/struct.Stdin.html#impl-Read-for-%26Stdin) - [Accept non `'static` lifetimes for several `std::error::Error` related implementations] (rust-lang/rust#113833) - [Make `impl<Fd: AsFd>` impl take `?Sized`] (rust-lang/rust#114655) - [`impl From<TryReserveError> for io::Error`] (https://doc.rust-lang.org/stable/std/io/struct.Error.html#impl-From%3CTryReserveError%3E-for-Error) These APIs are now stable in const contexts: - [`Barrier::new()`] (https://doc.rust-lang.org/stable/std/sync/struct.Barrier.html#method.new) Cargo ----- - [Stabilize lockfile v4](rust-lang/cargo#12852) - [Respect `rust-version` when generating lockfile] (rust-lang/cargo#12861) - [Control `--charset` via auto-detecting config value] (rust-lang/cargo#13337) - [Support `target.<triple>.rustdocflags` officially] (rust-lang/cargo#13197) - [Stabilize global cache data tracking] (rust-lang/cargo#13492) Misc ---- - [rustdoc: add `--test-builder-wrapper` arg to support wrappers such as RUSTC_WRAPPER when building doctests] (rust-lang/rust#114651) Compatibility Notes ------------------- - [Many unsafe precondition checks now run for user code with debug assertions enabled] (rust-lang/rust#120594) This change helps users catch undefined behavior in their code, though the details of how much is checked are generally not stable. - [riscv only supports split_debuginfo=off for now] (rust-lang/rust#120518) - [Consistently check bounds on hidden types of `impl Trait`] (rust-lang/rust#121679) - [Change equality of higher ranked types to not rely on subtyping] (rust-lang/rust#118247) - [When called, additionally check bounds on normalized function return type] (rust-lang/rust#118882) - [Expand coverage for `arithmetic_overflow` lint] (rust-lang/rust#119432) 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. - [Update to LLVM 18](rust-lang/rust#120055) - [Build `rustc` with 1CGU on `x86_64-pc-windows-msvc`] (rust-lang/rust#112267) - [Build `rustc` with 1CGU on `x86_64-apple-darwin`] (rust-lang/rust#112268) - [Introduce `run-make` V2 infrastructure, a `run_make_support` library and port over 2 tests as example] (rust-lang/rust#113026) - [Windows: Implement condvar, mutex and rwlock using futex] (rust-lang/rust#121956)
The old solver sometimes incorrectly used
sub
, change it to explicitly instantiate binders and useeq
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