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

Implement Impl Trait #324

Merged
merged 42 commits into from
Apr 16, 2020
Merged

Conversation

detrumi
Copy link
Member

@detrumi detrumi commented Jan 20, 2020

Part of #335

@jackh726
Copy link
Member

I'll definitely look over this later, but can you add the tests removed in 6252238?

@detrumi
Copy link
Member Author

detrumi commented Jan 21, 2020

I'll definitely look over this later, but can you add the tests removed in 6252238?

Thanks, that's really helpful! (wasn't aware that we had those tests)
Note that it's a bit early for a review, there's still a lot of todo's and some parts that need to change (I misunderstood how the goals would look like for example, which is why those tests are so useful)

@detrumi detrumi force-pushed the impl-trait-alias branch 2 times, most recently from 1351751 to df2444a Compare January 22, 2020 14:38
@detrumi detrumi force-pushed the impl-trait-alias branch 3 times, most recently from 795607b to 65d118a Compare March 2, 2020 18:29
@detrumi detrumi marked this pull request as ready for review March 2, 2020 18:40
@detrumi detrumi force-pushed the impl-trait-alias branch from 62e0ae4 to 123e0a8 Compare March 2, 2020 18:59
@nikomatsakis
Copy link
Contributor

Needs rebase now, @detrumi =)

@detrumi detrumi force-pushed the impl-trait-alias branch from 123e0a8 to 9fcbc21 Compare March 2, 2020 21:27
@detrumi detrumi force-pushed the impl-trait-alias branch 2 times, most recently from 43fcfef to 32223e4 Compare March 11, 2020 14:33
Copy link
Contributor

@nikomatsakis nikomatsakis left a comment

Choose a reason for hiding this comment

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

Left some comments, some of which probably require a bit more discussion to really act on. I am wondering also if we should carve out some slices of this PR to land (e.g., some of the parsing, maybe creating some core data structures, or generalizing AliasEq to have an enum -- perhaps initially with just one variant for the existing placeholders)

@detrumi detrumi force-pushed the impl-trait-alias branch 2 times, most recently from b90a788 to 9e8d974 Compare March 20, 2020 15:27
@detrumi detrumi force-pushed the impl-trait-alias branch 3 times, most recently from c01fe20 to 3b368bb Compare April 6, 2020 20:45
@detrumi
Copy link
Member Author

detrumi commented Apr 14, 2020

@nikomatsakis Could you make some time to do the review you were planning to do?

@nikomatsakis
Copy link
Contributor

Gah @detrumi sorry I forgot! Will try to do so now.

@nikomatsakis
Copy link
Contributor

I think the remaining test failures are because you have to modify the code that generates 'which program clauses might implement a given goal'.

In particular, when you run the test, you are asked to prove;

goal { T : Clone }
using solver: SLG { max_size: 10, expected_answers: None }
| creating new table TableIndex(0) and goal UCanonical {
:     canonical: Canonical {
:         value: InEnvironment {
:             environment: Env([]),
:             goal: Implemented(impl for<> OpaqueTyDatumBound { hidden_ty: Ty, bounds: for<type> [for<> Implemented(^1.0: Clone)] }: Clone),
:         },
:         binders: [],
:     },
:     universes: 1,
: } {

that winds up in this match arm:

DomainGoal::Holds(WhereClause::Implemented(trait_ref)) => {

this has logic for "things derived from traits and impls" and "things where dyn Trait is the self type" but we need some logic for "cases where an alias placeholder is the self type".

Something like:

if let /* AliasPlaceholder */ = self_ty.data(interner) {
    db.opaque_type(...).to_program_clauses(...);
}

(BTW, I find the debug output for impl trait types (OpaqueTyDatumBound { hidden_ty: Ty, bounds: for<type> [for<> Implemented(^1.0: Clone)] }) rather confusing -- I guess it's looking up the definition? I think I'd prefer to see just the name of the impl trait, like Foo.)

@nikomatsakis
Copy link
Contributor

@detrumi I guess this is waiting on a rebase and then perhaps some more tests?

It'd be good to create an updated list of what's left to do.

I forget if we decided to defer handling of things generic opaque types like type Foo<X> = impl Iterator<Item = X>? It seems like a lot of the code is there for that already.

@detrumi
Copy link
Member Author

detrumi commented Apr 15, 2020

@detrumi I guess this is waiting on a rebase and then perhaps some more tests?

It'd be good to create an updated list of what's left to do.

I forget if we decided to defer handling of things generic opaque types like type Foo<X> = impl Iterator<Item = X>? It seems like a lot of the code is there for that already.

Yes, I started on the rebase but quite a lot of things changed last week, so that takes some work 😅

For what's left, the list in #335 is still fairly up-to-date, the only additional points are adding documentation and more tests, which I'd like to do in separate PRs.

@detrumi
Copy link
Member Author

detrumi commented Apr 16, 2020

@detrumi heh, sorry, one more rebase? heart

@nikomatsakis Done!

Copy link
Contributor

@nikomatsakis nikomatsakis left a comment

Choose a reason for hiding this comment

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

Sorry, @detrumi, in doing the final review, I noted one thing I really do think we should fix before landing.

});
}

for auto_trait_id in builder.db.auto_traits() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Since we need to get rid of this method, what I think we should be doing instead is to look and check:

If we are generate clauses for a goal like

Implemented(!T: Foo)

where !T is an alias placeholder and Foo is an auto-trait, then we generate these clauses, and otherwise we don't.

The to_program_clauses trait doesn't work for this, but we can just a free function or another extension trait.

Copy link
Member Author

@detrumi detrumi Apr 16, 2020

Choose a reason for hiding this comment

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

Right, so we generate such clauses only when there's such a goal. Somewhere around push_auto_trait_impls probably?

Anyways, I'll just remove this part for now, I'll add it to the list for next steps. With the auto_traits function removed, it should be ready to merge, right?

pub opaque_ty_ids: BTreeMap<Identifier, OpaqueTyId<ChalkIr>>,

/// For each opaque type:
pub opaque_ty_data: BTreeMap<OpaqueTyId<ChalkIr>, Arc<OpaqueTyDatum<ChalkIr>>>,
Copy link
Contributor

Choose a reason for hiding this comment

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

This can be done in a follow-up PR, but I think we should add a opaque_names: BTreeMap<TraitId<ChalkIr>, Identifier> or something like that, created during lowering...

opaque_ty: &OpaqueTy<ChalkIr>,
fmt: &mut fmt::Formatter<'_>,
) -> Result<(), fmt::Error> {
write!(fmt, "impl {:?}", opaque_ty.opaque_ty_id)
Copy link
Contributor

Choose a reason for hiding this comment

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

...and then reference the opaque_type_names here to print the name of the opaque type, and not "impl"

fmt: &mut fmt::Formatter<'_>,
) -> Result<(), fmt::Error> {
if let Some(d) = self.opaque_ty_data.get(&opaque_ty_id) {
write!(fmt, "{:?}", d.bound.skip_binders().hidden_ty)
Copy link
Contributor

Choose a reason for hiding this comment

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

...or maybe here we want to print the name? anyway, somewhere. It'd be nice for opaque types to print out with their name and for the placeholders to print with !Foo.

@nikomatsakis nikomatsakis merged commit f438d7e into rust-lang:master Apr 16, 2020
@detrumi detrumi deleted the impl-trait-alias branch April 16, 2020 17:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants