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

better errors when resolving bad Self in impl block #93971

Closed

Conversation

compiler-errors
Copy link
Member

@compiler-errors compiler-errors commented Feb 13, 2022

  1. Fix the ICE when we see things like impl Self by resolving an impl's self type outside of its own scope.
  2. Fix a cycle error when we have something like impl Tr<Self::A> for Ty {} by distinguishing when we're resolving an impl's trait ref, and marking any associated types in that trait ref as ambiguous.

Fixes #93952

@rustbot rustbot added T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. labels Feb 13, 2022
@rust-highfive
Copy link
Collaborator

r? @petrochenkov

(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 13, 2022
Copy link
Contributor

@cjgillot cjgillot left a comment

Choose a reason for hiding this comment

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

I'm not really convinced by this implementation. If having Self in the type reference is illegal, we should report it right away.
Could we use a new RibKind::InImplBlockType binding Self to Res::Err, and have validate_res_from_ribs emit the error in the resolver.

this.with_self_rib(Res::SelfTy(None, None), |this| {
// FIXME(compiler-errors): `Self` may still exist in the value namespace,
// so we might be able to resolve an outer `Self` in, e.g., a const generic default.
this.with_no_self_type(|this| {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this required? IIUC, current_self_type is only used for diagnostics.

Copy link
Member Author

@compiler-errors compiler-errors Feb 13, 2022

Choose a reason for hiding this comment

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

it's probably not useful in that case.

pub struct ResImpl {
pub def_id: DefId,
pub generics_allowed: bool,
pub in_trait_ref: bool,
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure I understand where this flag is used.

Copy link
Member Author

@compiler-errors compiler-errors Feb 13, 2022

Choose a reason for hiding this comment

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

We specifically need to deny this:

impl Trait<Self::A> for () {}

But we should not deny this:

impl Trait<Self> for () {}

So we use this flag during astconv to catch cases (like the former) where we have an associated type with a Self in the path, which without this flag would try to elaborate the supertraits of the trait ref, looking for one with the associated type.

Since resolving the trait ref would itself require resolving an associated type in the trait ref, we get a cycle error.

@compiler-errors
Copy link
Member Author

compiler-errors commented Feb 13, 2022

Could we use a new RibKind::InImplBlockType binding Self to Res::Err, and have validate_res_from_ribs emit the error in the resolver.

@cjgillot: I could try this! Do you think that splitting out all of the combinations of Some and None in the Res::SelfTy is worthwhile?

Still not sure if this fixes the cycle error with a associated type on Self in an impl's trait ref, see the comment above for more detail. I'll do some thinking about that.

@rustbot author

@rustbot rustbot 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 Feb 13, 2022
@petrochenkov petrochenkov added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Feb 14, 2022
@petrochenkov
Copy link
Contributor

I'd personally close this and close #93952 as well as not an issue.
The error is quite clear, the erroreous case is fringe, and I don't want to add extra data to HIR only to report a different message for it.

@petrochenkov petrochenkov 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 Feb 14, 2022
@bors
Copy link
Contributor

bors commented Feb 14, 2022

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

@compiler-errors
Copy link
Member Author

compiler-errors commented Feb 22, 2022

Welp, don't think I'll get around to fixing this. I may re-attempt this with that rib idea suggested above later.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

"cycle detected" with impl Self {}
6 participants