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

orphan check incorrectly handles projections #99554

Open
lcnr opened this issue Jul 21, 2022 · 6 comments · Fixed by #117164
Open

orphan check incorrectly handles projections #99554

lcnr opened this issue Jul 21, 2022 · 6 comments · Fixed by #117164
Assignees
Labels
A-associated-items Area: Associated items such as associated types and consts. A-coherence Area: Coherence A-traits Area: Trait system C-bug Category: This is a bug. E-medium Call for participation: Medium difficulty. Experience needed to fix: Intermediate. E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion. I-unsound Issue: A soundness hole (worst kind of bug), see: https://en.wikipedia.org/wiki/Soundness P-high High priority S-triggers-future-incompat-lint Status: This bug triggers a future-incompatibility lint T-types Relevant to the types team, which will review and decide on the PR/issue.

Comments

@lcnr
Copy link
Contributor

lcnr commented Jul 21, 2022

We consider projections to be foreign types during orphan check

| ty::Alias(ty::Projection | ty::Inherent | ty::Weak, ..) => {
self.found_non_local_ty(ty)
}

but do not consider them to be parameters when checking for uncovered types

} else if let ty::Param(_) = input_ty.kind() {

This is more obvious after #99552

this means that the following impl passes the orphan check even though it shouldn't

// crate a
pub trait Foreign<T, U> {
    type Assoc;
}

// crate b
use a::Foreign;

trait Id {
    type Assoc;
}

impl<T> Id for T {
    type Assoc = T;
}

pub struct B;
impl<T> Foreign<B, T> for <T as Id>::Assoc {
    type Assoc = usize;
}

The impl in b overlaps with an impl impl<U> Foreign<T, LocalTy> for LocalTy in another crate c which passes the orphan check.

While I wasn't able to cause runtime UB with this, I was able to get an ICE during codegen_fulfill_obligation: https://github.com/lcnr/orphan-check-ub

cc @rust-lang/types

@lcnr lcnr added A-traits Area: Trait system I-unsound Issue: A soundness hole (worst kind of bug), see: https://en.wikipedia.org/wiki/Soundness C-bug Category: This is a bug. T-types Relevant to the types team, which will review and decide on the PR/issue. labels Jul 21, 2022
@rustbot rustbot added the I-prioritize Issue: Indicates that prioritization has been requested for this issue. label Jul 21, 2022
@lcnr lcnr added E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion. E-medium Call for participation: Medium difficulty. Experience needed to fix: Intermediate. labels Jul 21, 2022
@lcnr
Copy link
Contributor Author

lcnr commented Jul 21, 2022

after #99552 has landed, we should be able to fix this by:

  • rename found_param_ty to found_uncovered_ty
  • also use that function for ty::Projection
  • add tests + update the error message to also make sense for projections
  • run crater

@apiraino
Copy link
Contributor

WG-prioritization assigning priority (Zulip discussion).

@rustbot label -I-prioritize +P-high

@rustbot rustbot added P-high High priority and removed I-prioritize Issue: Indicates that prioritization has been requested for this issue. labels Jul 21, 2022
@atsuzaki
Copy link
Contributor

atsuzaki commented Aug 3, 2022

@rustbot claim

@nikomatsakis
Copy link
Contributor

Discussed in the types team on-site meetup thing:

Concluded that we should explore having the orphan check normalize first but having the "is knowable" check remain syntactic (not normalize).

@steffahn
Copy link
Member

Adding some labels from #107377@rustbot label A-associated-items, A-coherence

bors added a commit to rust-lang-ci/rust that referenced this issue Jan 18, 2024
Normalize trait ref before orphan check & consider ty params in alias types to be uncovered

Fixes rust-lang#99554.

r? lcnr
bors added a commit to rust-lang-ci/rust that referenced this issue Feb 26, 2024
Normalize trait ref before orphan check & consider ty params in alias types to be uncovered

Fixes rust-lang#99554, fixes rust-lang/types-team#104.

Supersedes rust-lang#100555.

r? lcnr
@bors bors closed this as completed in f705de5 Apr 30, 2024
@fmease
Copy link
Member

fmease commented Apr 30, 2024

Reopening because the soundness issue still exists. However, since #117164 we now emit a future-incompat dont-report-in-deps warn-by-default lint for it. Tracked separately in #124559.

@fmease fmease reopened this Apr 30, 2024
@fmease fmease added the S-triggers-future-incompat-lint Status: This bug triggers a future-incompatibility lint label Sep 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-associated-items Area: Associated items such as associated types and consts. A-coherence Area: Coherence A-traits Area: Trait system C-bug Category: This is a bug. E-medium Call for participation: Medium difficulty. Experience needed to fix: Intermediate. E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion. I-unsound Issue: A soundness hole (worst kind of bug), see: https://en.wikipedia.org/wiki/Soundness P-high High priority S-triggers-future-incompat-lint Status: This bug triggers a future-incompatibility lint T-types Relevant to the types team, which will review and decide on the PR/issue.
Projects
Status: future compat lint
7 participants