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

Extend push_auto_trait_impl to built-in types #612

Merged
merged 14 commits into from
Sep 19, 2020

Conversation

memoryleak47
Copy link
Contributor

cc #604.

@basil-cow
Copy link
Contributor

basil-cow commented Sep 18, 2020

I am not entirely sure, but it seems like we can do the same thing in chalk as we do in rustc: get a list of constituent types for a type and push a clause T: Auto :- (∀x Constituent(x, T) -> x: Auto) (but expanded), I think that will make for a cleaner implementation?

@basil-cow
Copy link
Contributor

otherwise looks more or less what i've expected

Copy link
Member

@jackh726 jackh726 left a comment

Choose a reason for hiding this comment

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

Decent start

chalk-solve/src/clauses.rs Outdated Show resolved Hide resolved
chalk-solve/src/clauses.rs Outdated Show resolved Hide resolved
chalk-solve/src/clauses.rs Outdated Show resolved Hide resolved
chalk-solve/src/clauses.rs Outdated Show resolved Hide resolved
@basil-cow
Copy link
Contributor

oh I missed it at first, but thats not how you spell "constituent" :P

@memoryleak47
Copy link
Contributor Author

Whoops ^^'

@memoryleak47 memoryleak47 changed the title [WIP] Extend push_auto_trait_impl to built-in types Extend push_auto_trait_impl to built-in types Sep 19, 2020
enum ExtEnum { GoodVariant, BadVariant(Ext) }
}

goal {
Copy link
Member

Choose a reason for hiding this comment

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

Comments on these would be nice.

Can we also get a test of a tuple that does impl the auto trait.

Copy link
Member

Choose a reason for hiding this comment

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

Oh, Haha, that's below.

Clever, but I would feel better about splitting it up a bit.

}

goal {
forall<'a> { (fn(), [(); 1], [()], u32, *const (), str, !, Struct, Enum, func, good_closure, &'a ()): AutoTrait }
Copy link
Member

Choose a reason for hiding this comment

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

Mutable ref and raw please

Comment on lines 29 to 37
TypeName::Adt(adt_id) => {
let adt_datum = &db.adt_datum(adt_id);
let adt_datum_bound = adt_datum.binders.substitute(interner, &app_ty.substitution);
adt_datum_bound
.variants
.into_iter()
.flat_map(|variant| variant.fields.into_iter())
.collect()
}
Copy link
Contributor

Choose a reason for hiding this comment

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

PhantomData<T> should have T as a constituent type

Copy link
Member

@jackh726 jackh726 left a comment

Choose a reason for hiding this comment

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

A few comments but only one small actually suggestion.

}
});

// we only compare the `TypeName`s as
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if it's worth passing the substitution to this function, even if unused? Theoretically, if we wanted to match rustc's behavior in the integration, we could.

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 the answer is no. Interesting thought, but let's not overcomplicate.

assert!(builder.placeholders_in_scope().is_empty());

// auto traits are not implemented for foreign types
if let TypeName::Foreign(_) = app_ty.name {
Copy link
Member

Choose a reason for hiding this comment

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

Hmm, I feel like it would be simpler to make constituent_types return an Option and then have it return None for this.

);
});
// closures require binders, while the other types do not
if let TypeName::Closure(closure_id) = app_ty.name {
Copy link
Member

Choose a reason for hiding this comment

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

Alternatively, just move the Foreign check here.

chalk-solve/src/clauses.rs Show resolved Hide resolved
@jackh726
Copy link
Member

@bors r+

@bors
Copy link
Contributor

bors commented Sep 19, 2020

📌 Commit c35689c has been approved by jackh726

@bors
Copy link
Contributor

bors commented Sep 19, 2020

⌛ Testing commit c35689c with merge 9a3d72d...

@bors
Copy link
Contributor

bors commented Sep 19, 2020

☀️ Test successful - checks-actions
Approved by: jackh726
Pushing 9a3d72d to master...

@bors bors merged commit 9a3d72d into rust-lang:master Sep 19, 2020
@memoryleak47 memoryleak47 deleted the fix-auto-traits branch September 25, 2020 12:39
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