-
-
Notifications
You must be signed in to change notification settings - Fork 78
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
Test case: replace type with a more generic one without major change. #337
base: main
Are you sure you want to change the base?
Conversation
@epage could you check if you think this test crate contains any breaking changes, if you have a second? To me, it seems like the answer is "no" but I'd love to check if there's some edge case I've missed. If "no" is the right answer, then this is the remaining class of false-positives vaguely related to the import system. This isn't resolved by the changes in 0.17 since the |
pub struct NewStructConstructible<T> { | ||
pub x: T, | ||
} | ||
|
||
pub type OriginalStructConstructible = NewStructConstructible<i64>; |
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 this is the case you need to look into of whether it is semver compatible. I have a vague recollection of someone warning about replacing a type with type
and I think it was with {}
construction syntax.
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.
This would be another great use of witness crates (#223), except to demonstrate that some syntax does not break, instead of demonstrating that it breaks.
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 either found the issue you had in mind or something related: #338
TL;DR: pub type
replacement of a constructible unit or tuple struct is breaking, since the unit/tuple constructor is not usable on the type alias.
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.
Trait bounds on type aliases aren't checked. This is linted (sometimes an error, sometimes a warning), but the lint has false positives.
Additionally -- and as far as I know this is not documented anywhere -- while lifetime constraints aren't enforced per se, they do have semantic impact. Therefore, not having the same lifetime bounds (or lack of bounds) on the type
as are on the original struct is a breaking change. But if it doesn't cause local breakage, people who get the lint warning are likely to apply the suggestion and remove the semantically relevant bounds.
I suspect most people don't know the dyn Trait
lifetime default mechanism in the first place, and RFC 2093 made the potential for breakage a lot more nuanced to boot, so that may be an area to look into independently of type
aliases as well.
We're going to replace the original type (named like
OriginalType
) with two types:pub struct NewType<A, B = ()>
pub type OriginalType<A> = NewType<A>
This looks like it wouldn't be breaking, since the type alias has: