Precise bounds #19
Merged
Precise bounds #19
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
We'll want this in a moment. No functional chnage.
* Make the internal wrapper type have *all* the parent's generics, and contain a PhantomData, so we can: * Put the bounds on the Debug impl for the internal wrapper type. * Rename the internal wrapper type to avoid name clashes with users' types. * Rename the formatter variable to avoid name clashes with users's values.
Rather than trying to filter the generic parameters for ones used in types, state the bounds in terms of the actual field types. This is precisely correct. It avoids lossage in situations where the field type's implementation of the trait has bounds which depend in a less-than-straightforward way on its generics. There is still an issue with implementing traits which have supertraits. We ought to apply a `Self: Super` bound, but we don't yet.
When we are implementing traits which have supertraits, add bounds of the form `where Self: Super` for each supertrait. While we're touching every call site, abolish the unused parameters. (I have future plans that will want `params` in into_where_predicates_by_generic_parameters_check_types, but not in create_where_predicates_from_generic_parameters_check_types, so leave that one there for now.)
Use the new precise bounds function.
Use the new precise bounds function, rather than the ident-hunter. Apply the Clone supertrait explicitly. This disposes of the oddity mentioned in the comment, as will be demonstrated in a test.
Use the new precise bounds function.
This is no longer used, now that everything uses the precise bounds function.
Nothing calls this. And, nothing should, because it would give wrong answers. Rename create_where_predicates_from_generic_parameters to create_where_predicates_from_all_generic_parameters. This will be useful if we implement a `bound(...)` syntax that allows requesting the "apply a bound for each generic parameter, regardless" mode. If we don't want to implement such a mode, we should delete this function, but let's not entangle this already large bugfix MR with questions about future features and UI.
This was referenced Feb 20, 2024
Merged
|
Thank you so much for your PR! Your contribution is greatly appreciated. I truly value your help and effort! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fix the bounds for all generated trait impls.
Here, we change the default behaviour (without
...(bound(...))) to say something like:where FieldType: Clone, for each field.For example, consider this:
Before, we would get just
where T: Clone, which is not sufficient, because the generated impl needs to cloneboxedtoo.With this MR, we get
where T: Clone, Box<U>: Clone, PhantomData<V>: Clone. The latter is always satisfied sincePhantomDatais alwaysClone. The middle one is equivalent toU: Clone.That the previous ident-hunting approach cannot be made to work can be seen by considering
DebugAsDisplay(in the newdebug_struct::bound_5test case in this MR).struct S<T>(DebugAsDisplay<T>)can only beDebugifT: Display. There's no reasonable way to determine this condition and express it in those terms. But instead, if we writewhere DebugAsDisplay<T>: Debug, the compiler does the work for us.Fixes #17, choosing option (3). I think this is not a semver break: the bounds we now require are precisely correct, so the effect is to make code compile (and work) which would previously not compile.
I have retained the existing behaviour for fields with
method =. Those fields' bounds are not included. In some cases this means that the user needs to restate all of the bounds manually. That's not great but it's a thing we can improve later. Likewise my changes only affect autocomputed bounds.I've added test cases, each of which fails when attempted without the functional changes.
I've left open the possibility of providing a syntax for #17 option 2. I'm hoping to make a followup MR for that; I think that's important to help migration from educe 0.4.x.
Also addresses the
MyDebugpart of #18, since I was reworking that part anyway.