Skip to content

Document process for breaking changes #69

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.

Sign up for GitHub

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

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Document process for breaking changes #69

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
f8eb710
Document process for breaking changes
workingjubilee Aug 20, 2024
7388b6d
Document a trivial fix example for breaking changes
workingjubilee Aug 25, 2024
5322b01
Admonish to wait on maintainers landing stuff
workingjubilee Aug 27, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions src/breaking-changes/new-trait-impls.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,12 @@ let b = Arc::from("a");
will no longer compile, because we've previously been relying on inference to figure out the `T` in `Box<T>`.

This kind of breakage can be ok, but a [crater](https://github.com/rust-lang/crater/blob/master/docs/bot-usage.md) run should estimate the scope.
When implementing traits known to have this problem, crater should be run before initiating FCP,
so information on the scope of the breakage is available before deciding to accept the change.
This can include, but is not limited to,

- From
- FromIterator

## Deref coercion breaks when a new impl is introduced

Expand Down
27 changes: 26 additions & 1 deletion src/breaking-changes/summary.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,5 +4,30 @@ Breaking changes should be avoided when possible.
[RFC 1105](https://rust-lang.github.io/rfcs/1105-api-evolution.html) lays the foundations for what constitutes a breaking change.
Breakage may be deemed acceptable or not based on its actual impact,
which can be approximated with a [crater](https://github.com/rust-lang/crater/blob/master/docs/bot-usage.md) run.
Running crater should be done if nontrivial breakage is expected, so the information is
available during the final comment period.

If the impact isn't too high, looping in maintainers of broken crates and submitting PRs to fix them can be a valid strategy.
If the impact isn't too high, looping in maintainers of to-be-broken crates and submitting PRs
to fix them can be a valid strategy. However, this can only affect the crates in question, and
it does not automatically affect their dependents. Binary dependents may have already locked-in
a different, older version.

## Breaking and the trains
If a PR is merged and it turns out to have caused code to not compile during the nightly or beta release cycle,
unless there is a trivial fix, the PR should be reverted and a crater run should assess the impact.

### Model: A Trivial Fix
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AFAICT, this example is specifically about a nightly-only feature. It seems like it'd be a better example if we used a change that was visible to stable Rust code but that we mitigated, and how we mitigated it?

(Not trying to add substantial additional work here. And this example may still make sense as a guide for changes that only affect nightly.)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@joshtriplett Actually the issue here is precisely that the change was visible to stable Rust code, i.e. the regression was heading to stable, but the feature was nightly. I can clarify that in the remarks if you like?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I see. That does seem different from the case of a change where the change itself intentionally adds new stable API surface area, but I appreciate the clarification.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Uh, so, I don't actually know what I'm supposed to do here?


On 2024, March 9th, [`Context::ext`] was added in [#123203]. It adds a `&mut dyn Any` field to Context.
On 2024, May 16th regression [#125193] appeared in beta crater: `Context` was no longer `UnwindSafe`.
Some code depended on it being so, but `&mut T` is `!UnwindSafe`, so Context also became `!UnwindSafe`.

The PR to add `Context::ext` could have been reverted, but as the function is a nightly feature,
its implications for unwind safety are limited to those actually using that nightly feature.
Since nightly features are, by definition, permitted to have effects we may not want to stabilize,
the question of whether the unwind safety regression should be accepted was deferred by [#125392]
wrapping the field in `AssertUnwindSafe`. This allowed continued experimentation with the API.

[#123203]: https://github.com/rust-lang/rust/pull/123203
[#125193]: https://github.com/rust-lang/rust/pull/125193
[#125392]: https://github.com/rust-lang/rust/pull/125392
Loading