Uh oh!
There was an error while loading. Please reload this page.
Improve opaque type higher-ranked region error message under NLL - #92306
Merged
Conversation
rust-highfive
commented
Dec 27, 2021
Contributor
r? @davidtwco (rust-highfive has picked a reviewer for you, use r? to override) |
Aaron1011
commented
Dec 27, 2021
ContributorAuthor
Aaron1011
commented
Dec 27, 2021
ContributorAuthor
There was a problem hiding this comment.
This span isn't great, but it's the same one that we get without #![feature(nll)]. I'll probably work on improving it in a follow-up PR.
Contributor
There was a problem hiding this comment.
Most such spans will get better with my lazy TAIT PR
Aaron1011
commented
Dec 27, 2021
ContributorAuthor
@bors try @rust-timer queue |
rust-timer
commented
Dec 27, 2021
Collaborator
Awaiting bors try build completion. @rustbot label: +S-waiting-on-perf |
bors
commented
Dec 27, 2021
Collaborator
⌛ Trying commit 1bd8e29ed18ad76b00d71d3d51b1d5a94888e6d4 with merge 9cb40efda24c4b68d6c59a8e3d6d294ac2f86e67... |
bors
commented
Dec 27, 2021
Collaborator
☀️ Try build successful - checks-actions |
rust-timer
commented
Dec 27, 2021
Collaborator
Queued 9cb40efda24c4b68d6c59a8e3d6d294ac2f86e67 with parent f8abed9, future comparison URL. |
rust-timer
commented
Dec 27, 2021
Collaborator
Finished benchmarking commit (9cb40efda24c4b68d6c59a8e3d6d294ac2f86e67): comparison url. Summary: This change led to moderate relevant regressions 😿 in compiler performance.
If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf. 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 led 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 |
oli-obk
reviewed
Dec 27, 2021
oli-obk
left a comment
Contributor
There was a problem hiding this comment.
This is really cool and may make some things easier for me actually
Uh oh!
There was an error while loading. Please reload this page.
oli-obk
commented
Dec 27, 2021
Contributor
r? @oli-obk |
bors
commented
Jan 14, 2022
Collaborator
☔ The latest upstream changes (presumably #92844) made this pull request unmergeable. Please resolve the merge conflicts. |
Aaron1011force-pushed
the
opaque-type-op
branch
from
January 14, 2022 19:25
d264b14 to
d2871e9Comparebors
commented
Feb 8, 2022
Collaborator
☔ The latest upstream changes (presumably #92007) made this pull request unmergeable. Please resolve the merge conflicts. |
Aaron1011force-pushed
the
opaque-type-op
branch
from
February 8, 2022 05:36
d2871e9 to
880d8e2CompareAaron1011
commented
Feb 8, 2022
ContributorAuthor
oli-obk
commented
Feb 8, 2022
Contributor
@bors try @rust-timer queue |
rust-timer
commented
Feb 8, 2022
Collaborator
Awaiting bors try build completion. @rustbot label: +S-waiting-on-perf |
bors
commented
Feb 8, 2022
Collaborator
⌛ Trying commit 880d8e2a06157563f1fe47fb5125a96e7037fced with merge 0fd10de714ecd9c91b80fb63c4fffd13e1f68f56... |
Currently, any higher-ranked region errors involving opaque types
fall back to a generic "higher-ranked subtype error" message when
run under NLL. This PR adds better error message handling for this
case, giving us the same kinds of error messages that we currently
get without NLL:
```
error: implementation of `MyTrait` is not general enough
--> $DIR/opaque-hrtb.rs:12:13
|
LL | fn foo() -> impl for<'a> MyTrait<&'a str> {
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ implementation of `MyTrait` is not general enough
|
= note: `impl MyTrait<&'2 str>` must implement `MyTrait<&'1 str>`, for any lifetime `'1`...
= note: ...but it actually implements `MyTrait<&'2 str>`, for some specific lifetime `'2`
error: aborting due to previous error
```
To accomplish this, several different refactoring needed to be made:
* We now have a dedicated `InstantiateOpaqueType` struct which
implements `TypeOp`. This is used to invoke `instantiate_opaque_types`
during MIR type checking.
* `TypeOp` is refactored to pass around a `MirBorrowckCtxt`, which is
needed to report opaque type region errors.
* We no longer assume that all `TypeOp`s correspond to canonicalized
queries. This allows us to properly handle opaque type instantiation
(which does not occur in a query) as a `TypeOp`.
A new `ErrorInfo` associated type is used to determine what
additional information is used during higher-ranked region error
handling.
* The body of `try_extract_error_from_fulfill_cx`
has been moved out to a new function `try_extract_error_from_region_constraints`.
This allows us to re-use the same error reporting code between
canonicalized queries (which can extract region constraints directly
from a fresh `InferCtxt`) and opaque type handling (which needs to take
region constraints from the pre-existing `InferCtxt` that we use
throughout MIR borrow checking).Aaron1011force-pushed
the
opaque-type-op
branch
from
February 8, 2022 17:36
880d8e2 to
48a48fdCompareAaron1011
commented
Feb 8, 2022
ContributorAuthor
@bors r=oli-obk |
bors
commented
Feb 8, 2022
Collaborator
📌 Commit 48a48fd has been approved by |
bors
commented
Feb 8, 2022
Collaborator
⌛ Testing commit 48a48fd with merge 27ee5ca00e0d78f12757b37a3a8592495491c1f2... |
rust-log-analyzer
commented
Feb 8, 2022
Collaborator
The job Click to see the possible cause of the failure (guessed by this bot) |
bors
commented
Feb 8, 2022
Collaborator
💔 Test failed - checks-actions |
ContributorAuthor
bors
commented
Feb 9, 2022
Collaborator
bors
commented
Feb 9, 2022
Collaborator
☀️ Test successful - checks-actions |
rust-timer
commented
Feb 9, 2022
Collaborator
Finished benchmarking commit (1f0a968): comparison url. Summary: This benchmark run did not return any relevant results. If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf. @rustbot label: -perf-regression |
Merged
bors added a commit
to rust-lang-ci/rust
that referenced
this pull request
Feb 11, 2022
Revert lazy TAIT PR Revert rust-lang#92306 (sorry `@Aaron1011,` will include your changes in the fix PR) Revert rust-lang#93783 Revert rust-lang#92007fixesrust-lang#93788fixesrust-lang#93794fixesrust-lang#93821fixesrust-lang#93831fixesrust-lang#93841
flip1995 pushed a commit
to flip1995/rust-clippy
that referenced
this pull request
Feb 24, 2022
Revert lazy TAIT PR Revert rust-lang/rust#92306 (sorry `@Aaron1011,` will include your changes in the fix PR) Revert rust-lang/rust#93783 Revert rust-lang/rust#92007fixesrust-lang/rust#93788fixesrust-lang/rust#93794fixesrust-lang/rust#93821fixesrust-lang/rust#93831fixesrust-lang/rust#93841
bors added a commit
to rust-lang-ci/rust
that referenced
this pull request
Mar 30, 2022
…sakis
Lazy type-alias-impl-trait take two
### user visible change 1: RPIT inference from recursive call sites
Lazy TAIT has an insta-stable change. The following snippet now compiles, because opaque types can now have their hidden type set from wherever the opaque type is mentioned.
```rust
fn bar(b: bool) -> impl std::fmt::Debug {
if b {
return 42
}
let x: u32 = bar(false); // this errors on stable
99
}
```
The return type of `bar` stays opaque, you can't do `bar(false) + 42`, you need to actually mention the hidden type.
### user visible change 2: divergence between RPIT and TAIT in return statements
Note that `return` statements and the trailing return expression are special with RPIT (but not TAIT). So
```rust
#![feature(type_alias_impl_trait)]
type Foo = impl std::fmt::Debug;
fn foo(b: bool) -> Foo {
if b {
return vec![42];
}
std::iter::empty().collect() //~ ERROR `Foo` cannot be built from an iterator
}
fn bar(b: bool) -> impl std::fmt::Debug {
if b {
return vec![42]
}
std::iter::empty().collect() // Works, magic (accidentally stabilized, not intended)
}
```
But when we are working with the return value of a recursive call, the behavior of RPIT and TAIT is the same:
```rust
type Foo = impl std::fmt::Debug;
fn foo(b: bool) -> Foo {
if b {
return vec![];
}
let mut x = foo(false);
x = std::iter::empty().collect(); //~ ERROR `Foo` cannot be built from an iterator
vec![]
}
fn bar(b: bool) -> impl std::fmt::Debug {
if b {
return vec![];
}
let mut x = bar(false);
x = std::iter::empty().collect(); //~ ERROR `impl Debug` cannot be built from an iterator
vec![]
}
```
### user visible change 3: TAIT does not merge types across branches
In contrast to RPIT, TAIT does not merge types across branches, so the following does not compile.
```rust
type Foo = impl std::fmt::Debug;
fn foo(b: bool) -> Foo {
if b {
vec![42_i32]
} else {
std::iter::empty().collect()
//~^ ERROR `Foo` cannot be built from an iterator over elements of type `_`
}
}
```
It is easy to support, but we should make an explicit decision to include the additional complexity in the implementation (it's not much, see a721052457cf513487fb4266e3ade65c29b272d2 which needs to be reverted to enable this).
### PR formalities
previous attempt: rust-lang#92007
This PR also includes rust-lang#92306 and rust-lang#93783, as they were reverted along with rust-lang#92007 in rust-lang#93893fixesrust-lang#93411fixesrust-lang#88236fixesrust-lang#89312fixesrust-lang#87340fixesrust-lang#86800fixesrust-lang#86719fixesrust-lang#84073fixesrust-lang#83919fixesrust-lang#82139fixesrust-lang#77987fixesrust-lang#74282fixesrust-lang#67830fixesrust-lang#62742fixesrust-lang#54895
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
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
8 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Currently, any higher-ranked region errors involving opaque types
fall back to a generic "higher-ranked subtype error" message when
run under NLL. This PR adds better error message handling for this
case, giving us the same kinds of error messages that we currently
get without NLL:
To accomplish this, several different refactoring needed to be made:
InstantiateOpaqueTypestruct whichimplements
TypeOp. This is used to invokeinstantiate_opaque_typesduring MIR type checking.
TypeOpis refactored to pass around aMirBorrowckCtxt, which isneeded to report opaque type region errors.
TypeOps correspond to canonicalizedqueries. This allows us to properly handle opaque type instantiation
(which does not occur in a query) as a
TypeOp.A new
ErrorInfoassociated type is used to determine whatadditional information is used during higher-ranked region error
handling.
try_extract_error_from_fulfill_cxhas been moved out to a new function
try_extract_error_from_region_constraints.This allows us to re-use the same error reporting code between
canonicalized queries (which can extract region constraints directly
from a fresh
InferCtxt) and opaque type handling (which needs to takeregion constraints from the pre-existing
InferCtxtthat we usethroughout MIR borrow checking).