-
Notifications
You must be signed in to change notification settings - Fork 181
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
Conversation
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 |
otherwise looks more or less what i've expected |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Decent start
oh I missed it at first, but thats not how you spell "constituent" :P |
Whoops ^^' |
tests/test/auto_traits.rs
Outdated
enum ExtEnum { GoodVariant, BadVariant(Ext) } | ||
} | ||
|
||
goal { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
tests/test/auto_traits.rs
Outdated
} | ||
|
||
goal { | ||
forall<'a> { (fn(), [(); 1], [()], u32, *const (), str, !, Struct, Enum, func, good_closure, &'a ()): AutoTrait } |
There was a problem hiding this comment.
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
chalk-solve/src/clauses.rs
Outdated
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() | ||
} |
There was a problem hiding this comment.
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
There was a problem hiding this 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 |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
chalk-solve/src/clauses.rs
Outdated
assert!(builder.placeholders_in_scope().is_empty()); | ||
|
||
// auto traits are not implemented for foreign types | ||
if let TypeName::Foreign(_) = app_ty.name { |
There was a problem hiding this comment.
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.
chalk-solve/src/clauses.rs
Outdated
); | ||
}); | ||
// closures require binders, while the other types do not | ||
if let TypeName::Closure(closure_id) = app_ty.name { |
There was a problem hiding this comment.
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.
@bors r+ |
📌 Commit c35689c has been approved by |
☀️ Test successful - checks-actions |
cc #604.