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

Instance::resolve doesn't have the exact same checks as associated type projection. #70419

Closed
eddyb opened this issue Mar 26, 2020 · 0 comments · Fixed by #70535
Closed

Instance::resolve doesn't have the exact same checks as associated type projection. #70419

eddyb opened this issue Mar 26, 2020 · 0 comments · Fixed by #70535
Assignees
Labels
A-associated-items Area: Associated items (types, constants & functions) A-specialization Area: Trait impl specialization A-trait-system Area: Trait system A-type-system Area: Type system C-bug Category: This is a bug. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@eddyb
Copy link
Member

eddyb commented Mar 26, 2020

Specifically, this part of resolving associated fn/consts:

// Since this is a trait item, we need to see if the item is either a trait default item
// or a specialization because we can't resolve those unless we can `Reveal::All`.
// NOTE: This should be kept in sync with the similar code in
// `rustc::traits::project::assemble_candidates_from_impls()`.
let eligible = if !resolved_item.defaultness.is_default() {
true
} else if param_env.reveal == traits::Reveal::All {
!trait_ref.needs_subst()
} else {
false
};

Is missing this logic deciding whether an associated item is default:

let is_default = if node_item.node.is_from_trait() {
// If true, the impl inherited a `type Foo = Bar`
// given in the trait, which is implicitly default.
// Otherwise, the impl did not specify `type` and
// neither did the trait:
//
// ```rust
// trait Foo { type T; }
// impl Foo for Bar { }
// ```
//
// This is an error, but it will be
// reported in `check_impl_items_against_trait`.
// We accept it here but will flag it as
// an error when we confirm the candidate
// (which will ultimately lead to `normalize_to_error`
// being invoked).
false
} else {
// If we're looking at a trait *impl*, the item is
// specializable if the impl or the item are marked
// `default`.
node_item.item.defaultness.is_default()
|| super::util::impl_is_default(selcx.tcx(), node_item.node.def_id())
};

Just from looking at the code (as I can't get the MIR of this example to constant-fold that associated const into the function, so I can't test any of this), this would likely mishandle default impl (instead of default on the associated item inside the impl).

cc @wesleywiser @nikomatsakis

@Centril Centril added A-type-system Area: Type system A-trait-system Area: Trait system T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Mar 26, 2020
@jonas-schievink jonas-schievink added A-associated-items Area: Associated items (types, constants & functions) A-specialization Area: Trait impl specialization C-bug Category: This is a bug. labels Mar 26, 2020
@jonas-schievink jonas-schievink self-assigned this Mar 29, 2020
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this issue Apr 1, 2020
…ikomatsakis

Track the finalizing node in the specialization graph

Fixes rust-lang#70419
Fixes rust-lang#70442

r? @eddyb
@bors bors closed this as completed in 0e0d84c Apr 1, 2020
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 (types, constants & functions) A-specialization Area: Trait impl specialization A-trait-system Area: Trait system A-type-system Area: Type system C-bug Category: This is a bug. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants