Skip to content

orphan check incorrectly handles projections #99554

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

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 (types, constants & functions) A-coherence Area: Coherence A-trait-system 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-trait-system 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

@lcnr lcnr moved this to unblocked in T-types unsound issues Nov 15, 2023
bors added a commit to rust-lang-ci/rust that referenced this issue Dec 7, 2023
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 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
@github-project-automation github-project-automation bot moved this from unblocked to new solver everywhere in T-types unsound issues 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
@lcnr lcnr moved this from new solver everywhere to unblocked in T-types unsound issues May 17, 2024
@lcnr lcnr moved this from unblocked to future compat lint in T-types unsound issues May 17, 2024
@fmease fmease added the S-triggers-future-incompat-lint Status: This bug triggers a future-incompatibility lint label Sep 14, 2024
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
A-associated-items Area: Associated items (types, constants & functions) A-coherence Area: Coherence A-trait-system 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