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

Test case: replace type with a more generic one without major change. #337

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

obi1kenobi
Copy link
Owner

@obi1kenobi obi1kenobi commented Jan 31, 2023

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:

  • the same name
  • the same fields/variants
  • the same constructibility and pattern-matching behavior.

@obi1kenobi
Copy link
Owner Author

@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 type here is not just a rename and re-export — it's a new type that isn't semantically identical to the underlying.

Comment on lines +35 to +39
pub struct NewStructConstructible<T> {
pub x: T,
}

pub type OriginalStructConstructible = NewStructConstructible<i64>;
Copy link
Collaborator

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.

Copy link
Owner Author

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.

Copy link
Owner Author

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.

Copy link

@QuineDot QuineDot Feb 2, 2023

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.

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.

3 participants