From 963d624d63e75d1984e3f9a24a942034f901c586 Mon Sep 17 00:00:00 2001 From: jdonszelmann Date: Tue, 13 Aug 2024 00:49:23 +0200 Subject: [PATCH 1/4] add section on overlap checks --- src/SUMMARY.md | 1 + src/solve/invariants.md | 2 + src/traits/overlap.md | 72 ++++++++++++++++++++++++++++++++++++ src/traits/specialization.md | 4 +- 4 files changed, 77 insertions(+), 2 deletions(-) create mode 100644 src/traits/overlap.md diff --git a/src/SUMMARY.md b/src/SUMMARY.md index 8d9f94653..ec67f3677 100644 --- a/src/SUMMARY.md +++ b/src/SUMMARY.md @@ -137,6 +137,7 @@ - [Caching subtleties](./traits/caching.md) - [Implied bounds](./traits/implied-bounds.md) - [Specialization](./traits/specialization.md) + - [Overlap checks](./traits/overlap.md) - [Chalk-based trait solving](./traits/chalk.md) - [Lowering to logic](./traits/lowering-to-logic.md) - [Goals and clauses](./traits/goals-and-clauses.md) diff --git a/src/solve/invariants.md b/src/solve/invariants.md index a28c9ecbf..3157d163e 100644 --- a/src/solve/invariants.md +++ b/src/solve/invariants.md @@ -113,6 +113,8 @@ in the trait solver #### The type system is complete during the implicit negative overlap check in coherence ✅ +For more on overlap checking: [./overlap.md] + During the implicit negative overlap check in coherence we must never return *error* for goals which can be proven. This would allow for overlapping impls with potentially different associated items, breaking a bunch of other invariants. diff --git a/src/traits/overlap.md b/src/traits/overlap.md new file mode 100644 index 000000000..bc001f40e --- /dev/null +++ b/src/traits/overlap.md @@ -0,0 +1,72 @@ +# Overlap checks + +As part of checking items (specifically: structs, enums, traits, unions), +the compiler checks whether its impl blocks overlap, for example because they define the same functions. +This is an example an overlap check. +The same overlap check is done when constructing a [specialization graph](./specialization.md). +Here, traits implementations could overlap because of a conflicting blanket implementation overlapping with some specific implementation. + +The overlap check always compares two impls. +In the case of inherent impl blocks, this means that for small n, +rustc quite literally compares each impl to each other impl block in an n^2 loop +(see `fn check_item` in coherence/inherent_impls_overlap.rs). + +Overlapping is sometimes partially allowed: +1. for maker traits +2. under [specialization](./specialization.md) + +but normally isn't. + +The overlap check has various modes (see [`OverlapMode`]). +Importantly, there's the explicit negative impl check, and the implicit negative impl check. +Both try to apply negative reasoning to prove that an overlap is definitely impossible. + +[`OverlapMode`]: https://doc.rust-lang.org/beta/nightly-rustc/rustc_middle/traits/specialization_graph/enum.OverlapMode.html + +## The explicit negative impl check + +This check is done in [`impl_intersection_has_negative_obligation`]. + +This check tries to find a negative trait implementation. +For example: + +```rust +struct MyCustomBox(Box) + +// both in your own crate +impl From<&str> for MyCustomBox {} +impl From for MyCustomBox where E: Error {} +``` + +In this example, we'd get: +`MyCustomBox: From<&str>` and `MyCustomBox: From`, giving `?E = &str`. + +And thus, these two implementations would overlap. +However, libstd provides `&str: !Error`, and therefore guarantees that there +will never be a positive implementation of `&str: Error`, and thus there is no overlap. + +Note that for this kind of negative impl check, we must have explicit negative implementations provided. +This is not currently stable. + +[`impl_intersection_has_negative_obligation`]: https://doc.rust-lang.org/beta/nightly-rustc/rustc_trait_selection/traits/coherence/fn.impl_intersection_has_impossible_obligation.htmlhttps://doc.rust-lang.org/beta/nightly-rustc/rustc_trait_selection/traits/coherence/fn.impl_intersection_has_negative_obligation.html + +## The implicit negative impl check + +This check is done in [`impl_intersection_has_impossible_obligation`], +and does not rely on negative trait implementations and is stable. + +Let's say there's a +```rust +impl From for Box {} // in your own crate +impl From for Box where E: Error {} // in std +``` + +This would give: `Box: From`, and `Box: From`, +giving `?E = MyLocalType`. + +In your crate there's no `MyLocalType: Error`, downstream crates cannot implement `Error` (a remote trait) for `MyLocalType` (a remote type). +Therefore, these two impls do not overlap. +Importantly, this works even if there isn't a `impl !Error for MyLocalType`. + +[`impl_intersection_has_impossible_obligation`]: https://doc.rust-lang.org/beta/nightly-rustc/rustc_trait_selection/traits/coherence/fn.impl_intersection_has_impossible_obligation.html + diff --git a/src/traits/specialization.md b/src/traits/specialization.md index 7cae5e9c1..fb6dded46 100644 --- a/src/traits/specialization.md +++ b/src/traits/specialization.md @@ -5,8 +5,8 @@ Defined in the `specialize` module. The basic strategy is to build up a *specialization graph* during -coherence checking (recall that coherence checking looks for overlapping -impls). Insertion into the graph locates the right place +coherence checking (coherence checking looks for [overlapping impls](./overlap.md)). +Insertion into the graph locates the right place to put an impl in the specialization hierarchy; if there is no right place (due to partial overlap but no containment), you get an overlap error. Specialization is consulted when selecting an impl (of course), From a29b0ae8a5ca233d4df4db8a8d65af505532c774 Mon Sep 17 00:00:00 2001 From: jdonszelmann Date: Tue, 13 Aug 2024 10:13:12 +0200 Subject: [PATCH 2/4] fix some typos --- src/traits/overlap.md | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/traits/overlap.md b/src/traits/overlap.md index bc001f40e..ebd7df8ad 100644 --- a/src/traits/overlap.md +++ b/src/traits/overlap.md @@ -1,14 +1,14 @@ # Overlap checks As part of checking items (specifically: structs, enums, traits, unions), -the compiler checks whether its impl blocks overlap, for example because they define the same functions. +the compiler checks whether impl blocks overlap, for example because they define the same functions. This is an example an overlap check. The same overlap check is done when constructing a [specialization graph](./specialization.md). -Here, traits implementations could overlap because of a conflicting blanket implementation overlapping with some specific implementation. +Here, trait implementations could overlap, for example because of a conflicting blanket implementation overlapping with some specific implementation. The overlap check always compares two impls. -In the case of inherent impl blocks, this means that for small n, -rustc quite literally compares each impl to each other impl block in an n^2 loop +In the case of inherent impl blocks, this means that at least for small n, +rustc quite literally compares each impl to each other impl block in an `n^2` loop (see `fn check_item` in coherence/inherent_impls_overlap.rs). Overlapping is sometimes partially allowed: From 8fc655492b93a56dd11afdd726fd61c785158f1b Mon Sep 17 00:00:00 2001 From: jdonszelmann Date: Tue, 13 Aug 2024 11:50:31 +0200 Subject: [PATCH 3/4] merge piece on overlap checks with docs about coherence (based on review comments) --- src/SUMMARY.md | 2 +- src/{traits/overlap.md => coherence.md} | 49 +++++++++++++++++++------ src/solve/invariants.md | 2 +- src/traits/specialization.md | 2 +- 4 files changed, 40 insertions(+), 15 deletions(-) rename src/{traits/overlap.md => coherence.md} (57%) diff --git a/src/SUMMARY.md b/src/SUMMARY.md index ec67f3677..ca0317bb8 100644 --- a/src/SUMMARY.md +++ b/src/SUMMARY.md @@ -137,7 +137,6 @@ - [Caching subtleties](./traits/caching.md) - [Implied bounds](./traits/implied-bounds.md) - [Specialization](./traits/specialization.md) - - [Overlap checks](./traits/overlap.md) - [Chalk-based trait solving](./traits/chalk.md) - [Lowering to logic](./traits/lowering-to-logic.md) - [Goals and clauses](./traits/goals-and-clauses.md) @@ -157,6 +156,7 @@ - [Type checking](./type-checking.md) - [Method Lookup](./method-lookup.md) - [Variance](./variance.md) + - [Coherence Checking](./coherence.md) - [Opaque Types](./opaque-types-type-alias-impl-trait.md) - [Inference details](./opaque-types-impl-trait-inference.md) - [Return Position Impl Trait In Trait](./return-position-impl-trait-in-trait.md) diff --git a/src/traits/overlap.md b/src/coherence.md similarity index 57% rename from src/traits/overlap.md rename to src/coherence.md index ebd7df8ad..a34e1e8dd 100644 --- a/src/traits/overlap.md +++ b/src/coherence.md @@ -1,17 +1,40 @@ -# Overlap checks -As part of checking items (specifically: structs, enums, traits, unions), -the compiler checks whether impl blocks overlap, for example because they define the same functions. -This is an example an overlap check. -The same overlap check is done when constructing a [specialization graph](./specialization.md). -Here, trait implementations could overlap, for example because of a conflicting blanket implementation overlapping with some specific implementation. +# Coherence -The overlap check always compares two impls. -In the case of inherent impl blocks, this means that at least for small n, -rustc quite literally compares each impl to each other impl block in an `n^2` loop -(see `fn check_item` in coherence/inherent_impls_overlap.rs). +> NOTE: this is based on [notes by @lcnr](https://github.com/rust-lang/rust/pull/121848) + +Coherence checking is what detects both of trait impls and inherent impls overlapping with others. +(reminder: [inherent impls](https://doc.rust-lang.org/reference/items/implementations.html#inherent-implementations) are impls of concrete types like `impl MyStruct {}`) + +Overlapping trait impls always produce an error, +while overlapping inherent impls result in an error only if they have methods with the same name. + +Checking for overlaps is split in two parts. First there's the [overlap check(s)](#overlap-checks), +which finds overlaps between traits and inherent implementations that the compiler currently knows about. + +However, Coherence also results in an error if any other impls **could** exist, +even if they are currently unknown. +This affects impls which may get added to upstream crates in a backwards compatible way, +and impls from downstream crates. +This is called the Orphan check. + +## Overlap checks + +Overlap checks are performed for both inherent impls, and for trait impls. +This uses the same overlap checking code, really done as two separate analyses. +Overlap checks always consider pairs of implementations, comparing them to each other. + +Overlap checking for inherent impl blocks is done through `fn check_item` in coherence/inherent_impls_overlap.rs), +where you can very clearly see that (at least for small `n`), the check really performs `n^2` +comparisons between impls. + +In the case of traits, this check is currently done as part of building the [specialization graph](./specialization.md), +to handle specializing impls overlapping with their parent, but this may change in the future. + +In both cases, all pairs of impls are checked for overlap. Overlapping is sometimes partially allowed: + 1. for maker traits 2. under [specialization](./specialization.md) @@ -23,7 +46,7 @@ Both try to apply negative reasoning to prove that an overlap is definitely impo [`OverlapMode`]: https://doc.rust-lang.org/beta/nightly-rustc/rustc_middle/traits/specialization_graph/enum.OverlapMode.html -## The explicit negative impl check +### The explicit negative impl check This check is done in [`impl_intersection_has_negative_obligation`]. @@ -50,7 +73,7 @@ This is not currently stable. [`impl_intersection_has_negative_obligation`]: https://doc.rust-lang.org/beta/nightly-rustc/rustc_trait_selection/traits/coherence/fn.impl_intersection_has_impossible_obligation.htmlhttps://doc.rust-lang.org/beta/nightly-rustc/rustc_trait_selection/traits/coherence/fn.impl_intersection_has_negative_obligation.html -## The implicit negative impl check +### The implicit negative impl check This check is done in [`impl_intersection_has_impossible_obligation`], and does not rely on negative trait implementations and is stable. @@ -70,3 +93,5 @@ Importantly, this works even if there isn't a `impl !Error for MyLocalType`. [`impl_intersection_has_impossible_obligation`]: https://doc.rust-lang.org/beta/nightly-rustc/rustc_trait_selection/traits/coherence/fn.impl_intersection_has_impossible_obligation.html + + diff --git a/src/solve/invariants.md b/src/solve/invariants.md index 3157d163e..3d29b26ac 100644 --- a/src/solve/invariants.md +++ b/src/solve/invariants.md @@ -113,7 +113,7 @@ in the trait solver #### The type system is complete during the implicit negative overlap check in coherence ✅ -For more on overlap checking: [./overlap.md] +For more on overlap checking: [../coherence.md] During the implicit negative overlap check in coherence we must never return *error* for goals which can be proven. This would allow for overlapping impls with potentially different diff --git a/src/traits/specialization.md b/src/traits/specialization.md index fb6dded46..99b3cda27 100644 --- a/src/traits/specialization.md +++ b/src/traits/specialization.md @@ -5,7 +5,7 @@ Defined in the `specialize` module. The basic strategy is to build up a *specialization graph* during -coherence checking (coherence checking looks for [overlapping impls](./overlap.md)). +coherence checking (coherence checking looks for [overlapping impls](../coherence.md)). Insertion into the graph locates the right place to put an impl in the specialization hierarchy; if there is no right place (due to partial overlap but no containment), you get an overlap From a328b7de9fdf5047dbed5bb63053003f145f773b Mon Sep 17 00:00:00 2001 From: jdonszelmann Date: Fri, 6 Sep 2024 15:57:56 +0200 Subject: [PATCH 4/4] fix comments after discussion --- src/coherence.md | 14 ++++++-------- 1 file changed, 6 insertions(+), 8 deletions(-) diff --git a/src/coherence.md b/src/coherence.md index a34e1e8dd..024716f53 100644 --- a/src/coherence.md +++ b/src/coherence.md @@ -42,7 +42,7 @@ but normally isn't. The overlap check has various modes (see [`OverlapMode`]). Importantly, there's the explicit negative impl check, and the implicit negative impl check. -Both try to apply negative reasoning to prove that an overlap is definitely impossible. +Both try to prove that an overlap is definitely impossible. [`OverlapMode`]: https://doc.rust-lang.org/beta/nightly-rustc/rustc_middle/traits/specialization_graph/enum.OverlapMode.html @@ -54,15 +54,15 @@ This check tries to find a negative trait implementation. For example: ```rust -struct MyCustomBox(Box) +struct MyCustomErrorType; // both in your own crate -impl From<&str> for MyCustomBox {} -impl From for MyCustomBox where E: Error {} +impl From<&str> for MyCustomErrorType {} +impl From for MyCustomErrorType where E: Error {} ``` In this example, we'd get: -`MyCustomBox: From<&str>` and `MyCustomBox: From`, giving `?E = &str`. +`MyCustomErrorType: From<&str>` and `MyCustomErrorType: From`, giving `?E = &str`. And thus, these two implementations would overlap. However, libstd provides `&str: !Error`, and therefore guarantees that there @@ -71,7 +71,7 @@ will never be a positive implementation of `&str: Error`, and thus there is no o Note that for this kind of negative impl check, we must have explicit negative implementations provided. This is not currently stable. -[`impl_intersection_has_negative_obligation`]: https://doc.rust-lang.org/beta/nightly-rustc/rustc_trait_selection/traits/coherence/fn.impl_intersection_has_impossible_obligation.htmlhttps://doc.rust-lang.org/beta/nightly-rustc/rustc_trait_selection/traits/coherence/fn.impl_intersection_has_negative_obligation.html +[`impl_intersection_has_negative_obligation`]: https://doc.rust-lang.org/beta/nightly-rustc/rustc_trait_selection/traits/coherence/fn.impl_intersection_has_negative_obligation.html ### The implicit negative impl check @@ -93,5 +93,3 @@ Importantly, this works even if there isn't a `impl !Error for MyLocalType`. [`impl_intersection_has_impossible_obligation`]: https://doc.rust-lang.org/beta/nightly-rustc/rustc_trait_selection/traits/coherence/fn.impl_intersection_has_impossible_obligation.html - -