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

Loop over all opaque types instead of looking at just the first one with the same DefId #87107

Merged
merged 3 commits into from
Jul 16, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 6 additions & 4 deletions compiler/rustc_data_structures/src/vec_map.rs
Original file line number Diff line number Diff line change
Expand Up @@ -127,13 +127,15 @@ impl<K, V> IntoIterator for VecMap<K, V> {
}
}

impl<K, V> Extend<(K, V)> for VecMap<K, V> {
impl<K: PartialEq, V> Extend<(K, V)> for VecMap<K, V> {
fn extend<I: IntoIterator<Item = (K, V)>>(&mut self, iter: I) {
self.0.extend(iter);
for (k, v) in iter {
self.insert(k, v);
}
}

fn extend_one(&mut self, item: (K, V)) {
self.0.extend_one(item);
fn extend_one(&mut self, (k, v): (K, V)) {
self.insert(k, v);
}

fn extend_reserve(&mut self, additional: usize) {
Expand Down
36 changes: 13 additions & 23 deletions compiler/rustc_typeck/src/collect/type_of.rs
Original file line number Diff line number Diff line change
Expand Up @@ -509,11 +509,10 @@ pub(super) fn type_of(tcx: TyCtxt<'_>, def_id: DefId) -> Ty<'_> {
}
}

#[instrument(skip(tcx), level = "debug")]
fn find_opaque_ty_constraints(tcx: TyCtxt<'_>, def_id: LocalDefId) -> Ty<'_> {
use rustc_hir::{Expr, ImplItem, Item, TraitItem};

debug!("find_opaque_ty_constraints({:?})", def_id);

struct ConstraintLocator<'tcx> {
tcx: TyCtxt<'tcx>,
def_id: DefId,
Expand All @@ -522,13 +521,11 @@ fn find_opaque_ty_constraints(tcx: TyCtxt<'_>, def_id: LocalDefId) -> Ty<'_> {
}

impl ConstraintLocator<'_> {
#[instrument(skip(self), level = "debug")]
fn check(&mut self, def_id: LocalDefId) {
// Don't try to check items that cannot possibly constrain the type.
if !self.tcx.has_typeck_results(def_id) {
debug!(
"find_opaque_ty_constraints: no constraint for `{:?}` at `{:?}`: no typeck results",
self.def_id, def_id,
);
debug!("no constraint: no typeck results");
return;
}
// Calling `mir_borrowck` can lead to cycle errors through
Expand All @@ -540,21 +537,19 @@ fn find_opaque_ty_constraints(tcx: TyCtxt<'_>, def_id: LocalDefId) -> Ty<'_> {
.get_by(|(key, _)| key.def_id == self.def_id)
.is_none()
{
debug!(
"find_opaque_ty_constraints: no constraint for `{:?}` at `{:?}`",
self.def_id, def_id,
);
debug!("no constraints in typeck results");
return;
}
// Use borrowck to get the type with unerased regions.
let concrete_opaque_types = &self.tcx.mir_borrowck(def_id).concrete_opaque_types;
if let Some((opaque_type_key, concrete_type)) =
concrete_opaque_types.iter().find(|(key, _)| key.def_id == self.def_id)
{
debug!(
"find_opaque_ty_constraints: found constraint for `{:?}` at `{:?}`: {:?}",
self.def_id, def_id, concrete_type,
);
debug!(?concrete_opaque_types);
for (opaque_type_key, concrete_type) in concrete_opaque_types {
if opaque_type_key.def_id != self.def_id {
// Ignore constraints for other opaque types.
continue;
}

debug!(?concrete_type, ?opaque_type_key.substs, "found constraint");

// FIXME(oli-obk): trace the actual span from inference to improve errors.
let span = self.tcx.def_span(def_id);
Expand Down Expand Up @@ -603,7 +598,7 @@ fn find_opaque_ty_constraints(tcx: TyCtxt<'_>, def_id: LocalDefId) -> Ty<'_> {

if let Some((prev_span, prev_ty)) = self.found {
if *concrete_type != prev_ty {
debug!("find_opaque_ty_constraints: span={:?}", span);
debug!(?span);
// Found different concrete types for the opaque type.
let mut err = self.tcx.sess.struct_span_err(
span,
Expand All @@ -619,11 +614,6 @@ fn find_opaque_ty_constraints(tcx: TyCtxt<'_>, def_id: LocalDefId) -> Ty<'_> {
} else {
self.found = Some((span, concrete_type));
}
} else {
debug!(
"find_opaque_ty_constraints: no constraint for `{:?}` at `{:?}`",
self.def_id, def_id,
);
}
}
}
Expand Down
1 change: 1 addition & 0 deletions src/test/ui/type-alias-impl-trait/issue-85113.rs
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ trait Output<'a> {}
impl<'a> Output<'a> for &'a str {}

fn cool_fn<'a>(arg: &'a str) -> OpaqueOutputImpl<'a> {
Copy link
Contributor

Choose a reason for hiding this comment

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

This test is kind of weird. Does it pass if the let out: ... is removed?

I expect it to pass, because we stabilized member constraints in #84701, but it seems like the let out: ... is probably caused it not to work.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This test is kind of weird. Does it pass if the let out: ... is removed?

yes, it does. Note that this test has impl_trait_in_bindings enabled, so it's not really relevant for min_tait. The problem is that the let binding gives us

expected &'<empty> str, got &'a str

which is probably due to the odd way that impl_trait_in_bindings works and that should hopefully all go away once we do not treat them special anymore and just put them into the inference list just like we do with the return type.

Copy link
Member

Choose a reason for hiding this comment

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

This test is really weird, we were talking about it with @lqd some minutes ago.

Note the following https://play.rust-lang.org/?version=nightly&mode=debug&edition=2018&gist=3c4de7d1bfdfee59d8e2f3f5c0d39453

Copy link
Member

Choose a reason for hiding this comment

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

I think I'd merge this as is, in my remove let binding PR this test is going away anyway so ...

//~^ ERROR: concrete type differs from previous defining opaque type use
let out: OpaqueOutputImpl<'a> = arg;
arg
}
Expand Down
14 changes: 13 additions & 1 deletion src/test/ui/type-alias-impl-trait/issue-85113.stderr
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,18 @@ note: hidden type `&'<empty> str` captures lifetime smaller than the function bo
LL | type OpaqueOutputImpl<'a> = impl Output<'a> + 'a;
| ^^^^^^^^^^^^^^^^^^^^

error: concrete type differs from previous defining opaque type use
--> $DIR/issue-85113.rs:14:1
|
LL | fn cool_fn<'a>(arg: &'a str) -> OpaqueOutputImpl<'a> {
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ expected `&'<empty> str`, got `&'a str`
|
note: previous use here
--> $DIR/issue-85113.rs:14:1
|
LL | fn cool_fn<'a>(arg: &'a str) -> OpaqueOutputImpl<'a> {
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

Copy link
Member

Choose a reason for hiding this comment

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

The PR is great to me, we've discussed this with Oli already. I'm not strong enough to approve in particular this part of the PR. cc @nikomatsakis

Copy link
Member

Choose a reason for hiding this comment

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

The span here is kind of confusing. I suppose we want to be pointing to the binding?

error[E0477]: the type `&'<empty> str` does not fulfill the required lifetime
--> $DIR/issue-85113.rs:5:29
|
Expand Down Expand Up @@ -42,7 +54,7 @@ LL | type OpaqueOutputImpl<'a> = impl Output<'a> + 'a;
= note: expected `Output<'a>`
found `Output<'_>`

error: aborting due to 3 previous errors
error: aborting due to 4 previous errors

Some errors have detailed explanations: E0477, E0495, E0700.
For more information about an error, try `rustc --explain E0477`.