Skip to content
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

Propagate lifetime resolution errors into tcx.type_of #69178

Closed

Conversation

Aaron1011
Copy link
Member

Fixes #69136

Previously, lifetime resolution errors would cause an error to be
emitted, but would not mark the parent type as 'tainted' in any way.
We usually abort compilation before this becomes an issue - however,
opaque types can cause us to type-check function bodies before such an
abort occurs. Ths can result in trying to instantiate opaque types that
have invalid computed generics. Currently, this only causes issues for
nested opaque types, but there's no reason to expect the computed
generics to be sane when we had unresolved lifetimes (which can result
in extra lifetime parameters getting added to the generics).

This commit tracks 'unresolved lifetime' errors that occur during
lifetime resolution. When we type-check an item, we bail out and return
tcx.types.err if a lifetime error was reported for that type. This
causes us to skip type-checking of types affected by the lifetime error,
while still checking unrelated types.

Additionally, we now check for errors in 'parent' opaque types (if such
a 'parent' exists) when collecting constraints for opaque types. This
reflects the fact that opaque types inherit generics from 'parent'
opaque types - if an error ocurred while type-checking the parent,
we don't attempt to type-check the child.

@rust-highfive
Copy link
Collaborator

r? @matthewjasper

(rust_highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Feb 15, 2020
@bors
Copy link
Contributor

bors commented Feb 15, 2020

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

@Aaron1011 Aaron1011 force-pushed the feature/lifetime-error-tracking branch 2 times, most recently from 0ab312d to bdb0794 Compare February 15, 2020 18:55
Copy link
Contributor

@matthewjasper matthewjasper left a comment

Choose a reason for hiding this comment

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

So, I'm not convinced that this should be needed, lifetime resolution isn't handling OpaqueTy correctly because AST to HIR lowering is treating top level and nested impl Trait differeny. For example this will still incorrectly error:

#![feature(type_alias_impl_trait)]

trait SomeTrait {}

trait WithAssoc {
    type AssocType;
}

impl<T: ?Sized> WithAssoc for T {
    type AssocType = ();
}
impl<T: ?Sized> SomeTrait for T {}

type Return<'a> = impl WithAssoc<AssocType = impl Sized + 'a>;

fn my_fun<'a>() -> Return<'a> {}

fn main() {}

src/librustc_typeck/collect.rs Outdated Show resolved Hide resolved
@Aaron1011
Copy link
Member Author

@matthewjasper: You're right - there are still issues with lifetimes in nested opaque types (see #69137). This PR doesn't fix those errors - it just ensures that we properly bail out when errors do occur, since opaque type inference may implicitly assume that the opaque type has sane generics.

@bors
Copy link
Contributor

bors commented Feb 17, 2020

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

Fixes rust-lang#69136

Previously, lifetime resolution errors would cause an error to be
emitted, but would not mark the parent type as 'tainted' in any way.
We usually abort compilation before this becomes an issue - however,
opaque types can cause us to type-check function bodies before such an
abort occurs. Ths can result in trying to instantiate opaque types that
have invalid computed generics. Currently, this only causes issues for
nested opaque types, but there's no reason to expect the computed
generics to be sane when we had unresolved lifetimes (which can result
in extra lifetime parameters getting added to the generics).

This commit tracks 'unresolved lifetime' errors that occur during
lifetime resolution. When we type-check an item, we bail out and return
`tcx.types.err` if a lifetime error was reported for that type. This
causes us to skip type-checking of types affected by the lifetime error,
while still checking unrelated types.

Additionally, we now check for errors in 'parent' opaque types (if such
a 'parent' exists) when collecting constraints for opaque types. This
reflects the fact that opaque types inherit generics from 'parent'
opaque types - if an error ocurred while type-checking the parent,
we don't attempt to type-check the child.
@Aaron1011 Aaron1011 force-pushed the feature/lifetime-error-tracking branch from e940496 to 50a3c38 Compare February 17, 2020 12:56
@bors
Copy link
Contributor

bors commented Feb 27, 2020

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

@matthewjasper
Copy link
Contributor

It should be OK to assume that lifetime resolution is producing sane results, if a lifetime cannot be resolved then not recording it will result in AST conv lowering it as 'static.

The problem here is that lifetime resolution is recording 'a as EarlyBound with index 0, when it should have index 1 (A has index 0).

@JohnCSimon JohnCSimon 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 Mar 28, 2020
@joelpalmer joelpalmer 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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Apr 6, 2020
@Dylan-DPC-zz
Copy link

@Aaron1011 waiting for you to resolve the conflicts

@joelpalmer joelpalmer 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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Apr 14, 2020
@Dylan-DPC-zz
Copy link

Closing this due to inactivity

@Dylan-DPC-zz Dylan-DPC-zz added S-inactive Status: Inactive and waiting on the author. This is often applied to closed PRs. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Apr 14, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-inactive Status: Inactive and waiting on the author. This is often applied to closed PRs.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ICE: src\librustc\ty\subst.rs:496: Region parameter out of range when substituting in region 'a
7 participants