Fix soundness bug with type alias impl trait by oli-obk · Pull Request #82558 · rust-lang/rust

oli-obk · 2021-02-26T17:36:53Z

fixes #73481 (only the first defining for a specific TAIT use within a function is handled at all, all others are not looked at at all)

r? @matthewjasper

I am fully aware the diagnostics aren't great, but they are ungreat in general around type alias impl trait, which is tracked in #63205

compiler/rustc_trait_selection/src/opaque_types.rs

src/test/ui/type-alias-impl-trait/multiple-def-uses-in-one-fn.stderr

oli-obk · 2021-02-26T17:41:25Z

src/test/ui/type-alias-impl-trait/multiple-def-uses-in-one-fn.stderr

This duplication is really odd.. we go through the TAIT logic (where I emit this diagnostic) twice. Once with the function as the body, once with the crate root as the body. I haven't been able to figure out the reason for that yet, but will investigate further.

estebank

I'd be ok with landing this either way (with or without the err ty) as long as we leave a ticket open to sometime fix this.

estebank · 2021-02-26T19:36:58Z

src/test/ui/type-alias-impl-trait/multiple-def-uses-in-one-fn.stderr

This is quite confusing to me :(

I think the spans could be better here: the error should have a span of X<B, A> and the note should have a span of X<A, B>

src/test/ui/type-alias-impl-trait/multiple-def-uses-in-one-fn.stderr

estebank · 2021-02-26T19:39:02Z

compiler/rustc_trait_selection/src/opaque_types.rs

Can we check whether this and self.value_span are the same, and if they are, emit a different note or span_label?

I made a note of this, but it may become obsolete once I tackle all these spans in a more general manner.

compiler/rustc_mir/src/borrow_check/type_check/mod.rs

jackh726

I think this approach makes sense to me, but I don't know much about this code, so someone else (@matthewjasper) can do the "real" review.

compiler/rustc_trait_selection/src/opaque_types.rs

jackh726 · 2021-03-02T21:04:33Z

r? @nikomatsakis

bors · 2021-03-16T07:03:32Z

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

…rait

…hin a single function

…up diagnostics

…ror must have been emitted already

Dylan-DPC-zz · 2021-04-23T10:26:14Z

@nikomatsakis any updates on this?

nikomatsakis

So, I think that this fix is sound, but it doesn't seem correct -- I think we can accommodate this sort of case without a real problem.

nikomatsakis · 2021-04-27T09:23:56Z

compiler/rustc_trait_selection/src/opaque_types.rs

-            debug!("instantiate_opaque_types: returning concrete ty {:?}", opaque_defn.concrete_ty);
-            return opaque_defn.concrete_ty;
+        // than once in a function (e.g., if it's passed to a type alias).
+        if let Some(opaque_defn) = self.opaque_types.get_mut(&def_id) {


I feel like the right behavior here is to key by def-id+substs. We know that the substs are always universally quantified placeholders, right?

The substs here are concrete types from the function. I don't understand what universally quantified placeholders mean here

Keying by DefId+substs makes sense though. We'll get two type variables, and then we can figure out whether they are compatible in the same pass that does it cross-function

"Universally quantified placeholders" is jargon for the "generic parameters on the fn that we are type-checking". i.e., when we treat a parameter T as a kind of placeholder for "some type". "universally quantified' is because there is a "forall" quantifier on the function.

nikomatsakis · 2021-04-27T09:29:48Z

compiler/rustc_trait_selection/src/opaque_types.rs

+                opaque_defn.substs.push(substs);
+                opaque_defn.concrete_ty = tcx.ty_error_with_message(
+                    self.value_span,
+                    "defining use generics differ from previous defining use",


(Just a note that I think we could say this in a clearer way; I doubt users know what a defining use is, etc)

bors · 2021-05-07T19:07:29Z

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

nikomatsakis · 2021-05-13T17:13:56Z

@spastorino is going to pick up this PR -- @spastorino do you plan to open a separate PR? Should we close this one?

spastorino · 2021-05-13T18:24:33Z

Yes, I can open a separate PR.

rust-highfive assigned matthewjasper Feb 26, 2021

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Feb 26, 2021

oli-obk commented Feb 26, 2021

View reviewed changes

This comment has been minimized.

Sign in to view

estebank reviewed Feb 26, 2021

View reviewed changes

jackh726 reviewed Feb 27, 2021

View reviewed changes

compiler/rustc_trait_selection/src/opaque_types.rs Outdated Show resolved Hide resolved

jackh726 added F-type_alias_impl_trait `#[feature(type_alias_impl_trait)]` WG-compiler-traits labels Mar 2, 2021

rust-highfive assigned nikomatsakis and unassigned matthewjasper Mar 2, 2021

oli-obk added 6 commits March 18, 2021 08:58

Use tracing::instrument for better analzyabilty of logs around impl t…

af5f158

…rait

Stop OpaqueTypeDecl from being Copy

1625296

Require all type variables to satisfy the impl trait bounds

549f545

Emit an error when there are two differently inferred impl traits wit…

d9d4a52

…hin a single function

Defining use errors yield the error type to reduce irrelevant follow-…

0382c00

…up diagnostics

Don't emit errors containing type error as another (more useful) er…

93f3d08

…ror must have been emitted already

oli-obk force-pushed the type_alias_impl_TAIT branch from dcd3dd2 to 93f3d08 Compare March 18, 2021 10:50

JohnCSimon added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 4, 2021

camelid added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 23, 2021

nikomatsakis reviewed Apr 27, 2021

View reviewed changes

nikomatsakis 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 May 12, 2021

spastorino closed this May 13, 2021

Uh oh!

Conversation

oli-obk commented Feb 26, 2021

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This comment has been minimized.

estebank left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jackh726 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jackh726 commented Mar 2, 2021

Uh oh!

bors commented Mar 16, 2021

Uh oh!

Dylan-DPC-zz commented Apr 23, 2021

Uh oh!

nikomatsakis left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bors commented May 7, 2021

Uh oh!

nikomatsakis commented May 13, 2021

Uh oh!

spastorino commented May 13, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

12 participants