-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
Add "unit struct to normal struct" case to semver.md #10871
Conversation
Changing a public unit struct to a normal struct is a breaking change that should require a major version bump. Adding an entry to the semver page in the reference to document this.
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @ehuss (or someone else) soon. Please see the contribution instructions for more information. |
I think I would prefer to add a more general rule. I'm pretty sure it is not allowed to change a struct from any of the forms (unit, tuple, or brace). They each have subtle differences. I'm also fairly sure you can't change between other ADTs (union, enum). https://internals.rust-lang.org/t/pre-rfc-stable-rustdoc-urls/13099 contains a long discussion about the possibility of semver compatibility between types. It's been a long while since I've read it, so I don't remember all the details there, but it would be good to review. However, my instinct is that there is no compatibility between changes (although the context of rustdoc URLs may be a little different from Cargo's SemVer guidelines). I've also been contemplating changing the approach here to only list what is explicitly allowed (at least in terms of signatures), and then all other changes are assumed to be not allowed. I suspect the list of what is not allowed is going to be much longer than what is allowed. I haven't really thought this through much, but it is something that seems like a possibly better direction. Otherwise we'll just keep adding an endless list of rules of what you can't do. Also, I'm not sure if you've seen it, but I've been keeping a list of changes that haven't been added in #8736. A problem with managing these rules is that they can be very subtle and can require a lot of thought and consideration. I would like to address as many rules as possible, but most of those rules are tricky. |
Currently, I believe it does not appear that changing a unit struct to a tuple struct must be a breaking change: Of course, we could decide that, despite the conversion being technically allowed by the compiler, a unit -> tuple struct change requires a major version bump regardless. I just wanted to raise this edge case for your consideration. If the struct has no private fields, any other struct form change is clearly breaking because of differing struct literals rules. However, if the struct is
Great link — thanks! It's a bit of a long read and I'm about to go on a trip, so I'll need a bit of time to digest it.
I understand the hesitation around the very long list of rules. That said, I think having the "explicitly not allowed" approach is overall better for several reasons:
It's also much easier to create a good end-user experience when building semver-checkers (again, like my own The allow-list approach requires much more "understanding" of what the user's change is in order to generate good error messages, and comes with a bigger UX risk: "Is this really a breaking change or did the checker misunderstand my change?" With simple deny-list rules, it's easier to look at the query that captures the rule and verify that it faithfully encodes the rule because of the relative simplicity. Any violations that query finds can also be easily explained to the user. Since the tool might not cover all rules (since there are many and adding them all to the tool will take time), this approach will make it easier to explain to users which rules are checked by the tool and which are not: one can just show a table of all the semver rules in the reference and show checkmarks next to the ones checked by the tool. As @oli-obk said on Twitter the other day, we should strongly consider writing the rules in a form amenable to being checked automatically since they are complex and difficult to check by hand. I hope I've convinced you that the deny-list approach is better for checking automatically than the allow-list approach. I understand that the deny-list approach means more work to write and maintain the semver reference docs. As I'm advocating for that approach, I'm also happy to share in that burden by writing and reviewing new additions to the semver reference docs.
That's a fantastic list! I hadn't seen it, thanks for sharing — it will be very useful to me. |
This isn't quite right. // but both of these ways to create a Bar work fine
let _a = Bar;
let _b = Bar();
The subtleties are also around which entities exist in various namespaces. All structs place an entity in the type namespace. Tuple structs also place a constructor function in the value namespace. A unit-struct places a Thanks for the comments on the negative rules. That does sound like it'll be useful. Hopefully we can wrangle the rules so that they don't become too endless. |
Ah, my bad — thanks. Do you know if |
I believe the tuple constructor becomes private (it removes the EDIT: (I have an issue open to clarify that.) |
Once again, great link — thanks :) Removing the Here's my reasoning:
What do you think? Happy to amend the PR with what you think the rule should be. |
I think only if they have zero public fields. I can't think of a specific case otherwise where that would be a problem. So, in summary, I think the only safe conversion (aside from If there are any public fields, then it could cause problems with pattern matching. |
☔ The latest upstream changes (presumably #12339) made this pull request unmergeable. Please resolve the merge conflicts. |
## 🤖 New release * `logging`: 0.1.3 -> 0.2.0 (⚠️ API breaking changes) ###⚠️ `logging` breaking changes ``` --- failure derive_trait_impl_removed: built-in derived trait no longer implemented --- Description: A public type has stopped deriving one or more traits. This can break downstream code that depends on those types implementing those traits. ref: https://doc.rust-lang.org/reference/attributes/derive.html#derive impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.27.0/src/lints/derive_trait_impl_removed.ron Failed in: type MakeSpanWithId no longer derives Copy, in /tmp/.tmpL0GyxN/logging/src/http.rs:13 --- failure unit_struct_changed_kind: unit struct changed kind --- Description: A public unit struct has been changed to a normal (curly-braces) struct, which cannot be constructed using the same struct literal syntax. ref: rust-lang/cargo#10871 impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.27.0/src/lints/unit_struct_changed_kind.ron Failed in: struct MakeSpanWithId in /tmp/.tmpL0GyxN/logging/src/http.rs:13 ``` <details><summary><i><b>Changelog</b></i></summary><p> </p></details> --- This PR was generated with [release-plz](https://github.com/MarcoIeni/release-plz/). Signed-off-by: the-hacker-app-releases[bot] <150499272+the-hacker-app-releases[bot]@users.noreply.github.com> Co-authored-by: the-hacker-app-releases[bot] <150499272+the-hacker-app-releases[bot]@users.noreply.github.com>
At this has been waiting-on-author for a couple of years, I'm going to go ahead and close this. We can always pick this up again later. |
Ah, sorry — I did drop the ball on this. Closing is the right call. Thanks, Ed! I'm not sure I have the right set of skills to write reference-quality docs, and I don't want to take up more of the cargo team's time with reviewing docs PRs and mentoring me toward getting better at it. As I work on |
What does this PR try to resolve?
Changing a public unit struct to a normal struct appears to be a breaking change that should require a major version bump: unit structs are always constructible as
Foo
but normal (i.e. plain) structs are only constructible asFoo {}
. The curly braces are required by the compiler, and produce a compilation error and suggestion to add them if they are missing.The semver page in the reference does not mention anything about unit structs, and as far as I could tell, this case does not seem to be covered in any of the existing entries in the
Structs
section of the semver page.This PR adds a new entry in the
Structs
section of the semver page in the reference, describing this major breaking change and providing an example.