Rework negative coherence to properly consider impls that only partly overlap #112875

compiler-errors · 2023-06-21T04:36:25Z

This PR implements a modified negative coherence that handles impls that only have partial overlap.

It does this by:

taking both impl trait refs, instantiating them with infer vars
equating both trait refs
taking the equated trait ref (which represents the two impls' intersection), and resolving any vars
plugging all remaining infer vars with placeholder types

these placeholder-plugged trait refs can then be used normally with the new trait solver, since we no longer have to worry about the issue with infer vars in param-envs.

We use the new trait solver to reason correctly about unnormalized trait refs (due to deferred projection equality), since this avoid having to normalize anything under param-envs with infer vars in them.

This PR then additionally:

removes the FnPtr knowable hack by implementing proper negative FnPtr trait bounds for rigid types.

An example:

Consider these two partially overlapping impls:

impl<T, U> PartialEq<&U> for &T where T: PartialEq<U> {}
impl<F> PartialEq<F> for F where F: FnPtr {}

Under the old algorithm, we would take one of these impls and replace it with infer vars, then try unifying it with the other impl under identity substitutions. This is not possible in either direction, since it either sets T = U, or tries to equate F = &?0.

Under the new algorithm, we try to unify ?0: PartialEq<?0> with &?1: PartialEq<&?2>. This gives us ?0 = &?1 = &?2 and thus ?1 = ?2. The intersection of these two trait refs therefore looks like: &?1: PartialEq<&?1>. After plugging this with placeholders, we get a trait ref that looks like &!0: PartialEq<&!0>, with the first impl having substs ?T = ?U = !0 and the second having substs ?F = &!0¹.

Then we can take the param-env from the first impl, and try to prove the negated where clause of the second.

We know that &!0: !FnPtr never holds, since it's a rigid type that is also not a fn ptr, we successfully detect that these impls may never overlap.

For the purposes of this example, I just ignored lifetimes, since it doesn't really matter. ↩

bors · 2023-06-27T07:01:05Z

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

rustbot · 2023-07-19T17:29:08Z

Some changes occurred to the core trait solver

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

Some changes occurred in engine.rs, potentially modifying the public API of ObligationCtxt.

cc @lcnr, @compiler-errors

compiler-errors · 2023-07-19T17:41:21Z

This is obviously not ready to land, and at the very least needs an FCP, but I think it could use a review from someone like r? @lcnr.

compiler/rustc_trait_selection/src/traits/coherence.rs

compiler/rustc_trait_selection/src/solve/assembly/mod.rs

compiler/rustc_trait_selection/src/traits/engine.rs

compiler/rustc_trait_selection/src/traits/coherence.rs

bors · 2023-07-30T22:30:12Z

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

compiler-errors · 2023-08-02T02:10:52Z

sooo.... i rebased this with #114355, which makes core bootstrap

... however, f09aa8360afc9f3202e593d30af2ceb6e702c5ea shows that the new coherence algorithm doesn't like region constraints. We need to modify trait_ref_is_knowable to discard that unknowable candidate if the trait ref is fully covered by another impl.

spastorino · 2023-08-03T21:24:07Z

This looks good to me modulo lcnr's comments. Looking forward to have #114355 in, so we can also merge this one and I can start experimenting with this.

bors · 2023-08-04T11:33:48Z

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

spastorino · 2023-08-04T13:13:05Z

#114355 is merged, so after this one is rebased and lcnr's comments addressed I think it's good to r+

compiler-errors · 2023-10-23T23:42:01Z

Oh huh, those new "upstream crates may add a new impl" started after #116879. That kind of makes sense -- that regressed test has two TAITs in there.

lcnr · 2023-10-26T08:55:01Z

@bors r+

bors · 2023-10-26T08:55:03Z

📌 Commit 0626f2e has been approved by lcnr

It is now in the queue for this repository.

bors · 2023-10-26T10:57:24Z

⌛ Testing commit 0626f2e with merge 9ab0749...

bors · 2023-10-26T12:53:14Z

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

rust-timer · 2023-10-26T14:23:45Z

Finished benchmarking commit (9ab0749): 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)	-	-	0
Regressions ❌ (secondary)	1.2%	[1.2%, 1.2%]	2
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-	-	0

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

Binary size

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

Bootstrap: 634.461s -> 633.297s (-0.18%)
Artifact size: 304.47 MiB -> 304.48 MiB (0.00%)

…-in-coherence, r=lcnr Ignore but do not assume region obligations from unifying headers in negative coherence Partly addresses a FIXME that was added in rust-lang#112875. Just as we can throw away the nested trait/projection obligations from unifying two impl headers, we can also just throw away the region obligations too. I removed part of the FIXME that was incorrect, namely: > Given that the only region constraints we get are involving inference regions in the root, it shouldn't matter, but still sus. This is not true when unifying `fn(A)` and `for<'b> fn(&'b B)` which ends up with placeholder region outlives from non-root universes. I'm pretty sure this is okay, though it would be nice if we were to use them as assumptions. See the `explicit` revision of the test I committed, which still fails. Fixes rust-lang#117986 r? lcnr, feel free to reassign tho.

Rollup merge of rust-lang#117994 - compiler-errors:throw-away-regions-in-coherence, r=lcnr Ignore but do not assume region obligations from unifying headers in negative coherence Partly addresses a FIXME that was added in rust-lang#112875. Just as we can throw away the nested trait/projection obligations from unifying two impl headers, we can also just throw away the region obligations too. I removed part of the FIXME that was incorrect, namely: > Given that the only region constraints we get are involving inference regions in the root, it shouldn't matter, but still sus. This is not true when unifying `fn(A)` and `for<'b> fn(&'b B)` which ends up with placeholder region outlives from non-root universes. I'm pretty sure this is okay, though it would be nice if we were to use them as assumptions. See the `explicit` revision of the test I committed, which still fails. Fixes rust-lang#117986 r? lcnr, feel free to reassign tho.

Pkgsrc changes: * Adjust patches and cargo checksums to new versions. * For an external LLVM, set dependency of llvm >= 16, in accordance with the upstream changes. * Mark that on NetBSD we now need >= 9.0, so 8.x is no longer supported. * On NetBSD/sparc64 10.x, we now need GCC 12 to build the embedded LLVM, which is version 17; apparently GCC 10.4 or 10.5 mis-compiles it, resulting in an illegal instruction fault during the build. Ref. rust-lang/rust#117231 Upstream changes: Version 1.75.0 (2023-12-28) ========================== - [Stabilize `async fn` and return-position `impl Trait` in traits.] (rust-lang/rust#115822) - [Allow function pointer signatures containing `&mut T` in `const` contexts.] (rust-lang/rust#116015) - [Match `usize`/`isize` exhaustively with half-open ranges.] (rust-lang/rust#116692) - [Guarantee that `char` has the same size and alignment as `u32`.] (rust-lang/rust#116894) - [Document that the null pointer has the 0 address.] (rust-lang/rust#116988) - [Allow partially moved values in `match`.] (rust-lang/rust#103208) - [Add notes about non-compliant FP behavior on 32bit x86 targets.] (rust-lang/rust#113053) - [Stabilize ratified RISC-V target features.] (rust-lang/rust#116485) Compiler -------- - [Rework negative coherence to properly consider impls that only partly overlap.] (rust-lang/rust#112875) - [Bump `COINDUCTIVE_OVERLAP_IN_COHERENCE` to deny, and warn in dependencies.] (rust-lang/rust#116493) - [Consider alias bounds when computing liveness in NLL.] (rust-lang/rust#116733) - [Add the V (vector) extension to the `riscv64-linux-android` target spec.] (rust-lang/rust#116618) - [Automatically enable cross-crate inlining for small functions] (rust-lang/rust#116505) - Add several new tier 3 targets: - [`csky-unknown-linux-gnuabiv2hf`] (rust-lang/rust#117049) - [`i586-unknown-netbsd`] (rust-lang/rust#117170) - [`mipsel-unknown-netbsd`] (rust-lang/rust#117356) Refer to Rust's [platform support page][platform-support-doc] for more information on Rust's tiered platform support. Libraries --------- - [Override `Waker::clone_from` to avoid cloning `Waker`s unnecessarily.] (rust-lang/rust#96979) - [Implement `BufRead` for `VecDeque<u8>`.] (rust-lang/rust#110604) - [Implement `FusedIterator` for `DecodeUtf16` when the inner iterator does.] (rust-lang/rust#110729) - [Implement `Not, Bit{And,Or}{,Assign}` for IP addresses.] (rust-lang/rust#113747) - [Implement `Default` for `ExitCode`.] (rust-lang/rust#114589) - [Guarantee representation of None in NPO] (rust-lang/rust#115333) - [Document when atomic loads are guaranteed read-only.] (rust-lang/rust#115577) - [Broaden the consequences of recursive TLS initialization.] (rust-lang/rust#116172) - [Windows: Support sub-millisecond sleep.] (rust-lang/rust#116461) - [Fix generic bound of `str::SplitInclusive`'s `DoubleEndedIterator` impl] (rust-lang/rust#100806) - [Fix exit status / wait status on non-Unix `cfg(unix)` platforms.] (rust-lang/rust#115108) Stabilized APIs --------------- - [`Atomic*::from_ptr`] (https://doc.rust-lang.org/stable/core/sync/atomic/struct.AtomicUsize.html#method.from_ptr) - [`FileTimes`] (https://doc.rust-lang.org/stable/std/fs/struct.FileTimes.html) - [`FileTimesExt`] (https://doc.rust-lang.org/stable/std/os/windows/fs/trait.FileTimesExt.html) - [`File::set_modified`] (https://doc.rust-lang.org/stable/std/fs/struct.File.html#method.set_modified) - [`File::set_times`] (https://doc.rust-lang.org/stable/std/fs/struct.File.html#method.set_times) - [`IpAddr::to_canonical`] (https://doc.rust-lang.org/stable/core/net/enum.IpAddr.html#method.to_canonical) - [`Ipv6Addr::to_canonical`] (https://doc.rust-lang.org/stable/core/net/struct.Ipv6Addr.html#method.to_canonical) - [`Option::as_slice`] (https://doc.rust-lang.org/stable/core/option/enum.Option.html#method.as_slice) - [`Option::as_mut_slice`] (https://doc.rust-lang.org/stable/core/option/enum.Option.html#method.as_mut_slice) - [`pointer::byte_add`] (https://doc.rust-lang.org/stable/core/primitive.pointer.html#method.byte_add) - [`pointer::byte_offset`] (https://doc.rust-lang.org/stable/core/primitive.pointer.html#method.byte_offset) - [`pointer::byte_offset_from`] (https://doc.rust-lang.org/stable/core/primitive.pointer.html#method.byte_offset_from) - [`pointer::byte_sub`] (https://doc.rust-lang.org/stable/core/primitive.pointer.html#method.byte_sub) - [`pointer::wrapping_byte_add`] (https://doc.rust-lang.org/stable/core/primitive.pointer.html#method.wrapping_byte_add) - [`pointer::wrapping_byte_offset`] (https://doc.rust-lang.org/stable/core/primitive.pointer.html#method.wrapping_byte_offset) - [`pointer::wrapping_byte_sub`] (https://doc.rust-lang.org/stable/core/primitive.pointer.html#method.wrapping_byte_sub) These APIs are now stable in const contexts: - [`Ipv6Addr::to_ipv4_mapped`] (https://doc.rust-lang.org/stable/core/net/struct.Ipv6Addr.html#method.to_ipv4_mapped) - [`MaybeUninit::assume_init_read`] (https://doc.rust-lang.org/stable/core/mem/union.MaybeUninit.html#method.assume_init_read) - [`MaybeUninit::zeroed`] (https://doc.rust-lang.org/stable/core/mem/union.MaybeUninit.html#method.zeroed) - [`mem::discriminant`] (https://doc.rust-lang.org/stable/core/mem/fn.discriminant.html) - [`mem::zeroed`] (https://doc.rust-lang.org/stable/core/mem/fn.zeroed.html) Cargo ----- - [Add new packages to `[workspace.members]` automatically.] (rust-lang/cargo#12779) - [Allow version-less `Cargo.toml` manifests.] (rust-lang/cargo#12786) - [Make browser links out of HTML file paths.] (rust-lang/cargo#12889) Rustdoc ------- - [Accept less invalid Rust in rustdoc.] (rust-lang/rust#117450) - [Document lack of object safety on affected traits.] (rust-lang/rust#113241) - [Hide `#[repr(transparent)]` if it isn't part of the public ABI.] (rust-lang/rust#115439) - [Show enum discriminant if it is a C-like variant.] (rust-lang/rust#116142) Compatibility Notes ------------------- - [FreeBSD targets now require at least version 12.] (rust-lang/rust#114521) - [Formally demote tier 2 MIPS targets to tier 3.] (rust-lang/rust#115238) - [Make misalignment a hard error in `const` contexts.] (rust-lang/rust#115524) - [Fix detecting references to packed unsized fields.] (rust-lang/rust#115583) - [Remove support for compiler plugins.] (rust-lang/rust#116412)

yoshuawuyts mentioned this pull request Jul 4, 2023

Negative bounds (with feature gate) rust-lang/a-mir-formality#129

Open

rust-cloud-vms bot force-pushed the negative-coherence-rework branch 2 times, most recently from 7c4308e to bd8425f Compare July 19, 2023 17:28

compiler-errors marked this pull request as ready for review July 19, 2023 17:29

rustbot assigned lcnr Jul 19, 2023

compiler-errors added S-blocked Status: Blocked on something else such as an RFC or other implementation work. S-waiting-on-fcp Status: PR is in FCP and is awaiting for FCP to complete. labels Jul 19, 2023

lcnr reviewed Jul 20, 2023

View reviewed changes

compiler/rustc_trait_selection/src/traits/coherence.rs Show resolved Hide resolved

compiler/rustc_trait_selection/src/solve/assembly/mod.rs Outdated Show resolved Hide resolved

compiler/rustc_trait_selection/src/traits/engine.rs Outdated Show resolved Hide resolved

lcnr reviewed Jul 20, 2023

View reviewed changes

compiler/rustc_trait_selection/src/traits/coherence.rs Outdated Show resolved Hide resolved

rust-cloud-vms bot force-pushed the negative-coherence-rework branch 2 times, most recently from b8042f2 to 1fcfb27 Compare July 26, 2023 02:39

This comment has been minimized.

Sign in to view

compiler-errors mentioned this pull request Jul 28, 2023

fn try_merge_responses behavior rust-lang/trait-system-refactor-initiative#53

Open

lcnr 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 Aug 1, 2023

rust-cloud-vms bot force-pushed the negative-coherence-rework branch from 1fcfb27 to bc8bab0 Compare August 2, 2023 02:07

This comment has been minimized.

Sign in to view

rust-cloud-vms bot force-pushed the negative-coherence-rework branch from bc8bab0 to f09aa83 Compare August 2, 2023 02:24

compiler-errors added 4 commits October 23, 2023 23:35

Make things work by using the new solver

8597bf1

Consider regions

1d99ddb

coherence doesn't like region constraints

e1edefc

nits

0626f2e

rust-cloud-vms bot force-pushed the negative-coherence-rework branch from 98a0dd1 to 0626f2e Compare October 23, 2023 23:46

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 Oct 26, 2023

lcnr mentioned this pull request Oct 26, 2023

Tracking Issue for -Znext-solver #107374

Open

14 tasks

bors added the merged-by-bors This PR was explicitly merged by bors. label Oct 26, 2023

bors merged commit 9ab0749 into rust-lang:master Oct 26, 2023
11 checks passed

rustbot added this to the 1.75.0 milestone Oct 26, 2023

bors mentioned this pull request Oct 26, 2023

Lazily normalize inside trait ref during orphan check & consider ty params in rigid alias types to be uncovered #117164

Merged

github-actions bot mentioned this pull request Oct 31, 2023

Automated pull from upstream master ferrocene/ferrocene#74

Merged

compiler-errors mentioned this pull request Nov 16, 2023

Ignore but do not assume region obligations from unifying headers in negative coherence #117994

Merged

finnbear mentioned this pull request Apr 27, 2024

regression: conflicting implementations between #[fundamental] generic type and FnPtr #124449

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rework negative coherence to properly consider impls that only partly overlap #112875

Rework negative coherence to properly consider impls that only partly overlap #112875

compiler-errors commented Jun 21, 2023 •

edited

Loading

bors commented Jun 27, 2023

rustbot commented Jul 19, 2023

compiler-errors commented Jul 19, 2023

This comment has been minimized.

bors commented Jul 30, 2023

compiler-errors commented Aug 2, 2023 •

edited

Loading

This comment has been minimized.

spastorino commented Aug 3, 2023

bors commented Aug 4, 2023

spastorino commented Aug 4, 2023 •

edited

Loading

compiler-errors commented Oct 23, 2023

lcnr commented Oct 26, 2023

bors commented Oct 26, 2023

bors commented Oct 26, 2023

bors commented Oct 26, 2023

rust-timer commented Oct 26, 2023

Rework negative coherence to properly consider impls that only partly overlap #112875

Rework negative coherence to properly consider impls that only partly overlap #112875

Conversation

compiler-errors commented Jun 21, 2023 • edited Loading

Footnotes

bors commented Jun 27, 2023

rustbot commented Jul 19, 2023

compiler-errors commented Jul 19, 2023

This comment has been minimized.

bors commented Jul 30, 2023

compiler-errors commented Aug 2, 2023 • edited Loading

This comment has been minimized.

spastorino commented Aug 3, 2023

bors commented Aug 4, 2023

spastorino commented Aug 4, 2023 • edited Loading

compiler-errors commented Oct 23, 2023

lcnr commented Oct 26, 2023

bors commented Oct 26, 2023

bors commented Oct 26, 2023

bors commented Oct 26, 2023

rust-timer commented Oct 26, 2023

Overall result: no relevant changes - no action needed

Instruction count

Max RSS (memory usage)

Cycles

Binary size

compiler-errors commented Jun 21, 2023 •

edited

Loading

compiler-errors commented Aug 2, 2023 •

edited

Loading

spastorino commented Aug 4, 2023 •

edited

Loading