-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
breaking changes policies #18541
breaking changes policies #18541
Changes from 11 commits
a7eaac7
1fdc465
482bb59
689f819
c441694
8509fb1
76f0386
c642f6b
d9dae99
017cc98
1d38202
a4fb06d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||
---|---|---|---|---|---|---|---|---|
|
@@ -671,3 +671,117 @@ Conventions | |||||||
Furthermore, module names should use `snake_case` and not use capital | ||||||||
letters, which cause issues when going from an OS without case | ||||||||
sensitivity to an OS with it. | ||||||||
|
||||||||
|
||||||||
Breaking Changes | ||||||||
================ | ||||||||
|
||||||||
Introducing breaking changes, no matter how well intentioned, | ||||||||
creates long-term problems for the community, in particular those looking to promote | ||||||||
reusable Nim code in libraries: In the Nim distribution, critical security and bugfixes, | ||||||||
language changes and community improvements are bundled in a single distribution - it is | ||||||||
difficult to make partial upgrades with only benign changes. When one library depends on | ||||||||
a legacy behavior, it can no longer be used together with another library that does not, | ||||||||
breaking all downstream applications - the standard library is unique in that it sits at | ||||||||
the root of **all** dependency trees. | ||||||||
|
||||||||
There is a big difference between compile-time breaking changes and run-time breaking | ||||||||
changes. | ||||||||
|
||||||||
|
||||||||
Run-time breaking changes | ||||||||
------------------------- | ||||||||
|
||||||||
Run-time breaking changes are to be avoided at almost all costs: Nim is used for | ||||||||
mission critical applications which depend on behaviours that | ||||||||
are not covered by the test suite. As such, it's important that changes to the | ||||||||
*stable* parts of the standard library are made avoiding changing the existing | ||||||||
behaviors, even when the test suite continues to pass. | ||||||||
|
||||||||
Examples of run-time breaking changes: | ||||||||
|
||||||||
- Raising exceptions of a new type, compared to what's currently being raised. | ||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Raising a Defect should be considered an acceptable breaking change when the defect is called for There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Defect is an annoying things about Nim. The last thing I need is some library author thinking they know better than I do in regards to what is recoverable. Which is rarely if ever possible for the author to know as per the end to end argument in system design, the consumer of the library almost always has more options at their disposal and an uncatchable exception really stymes that. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't understand. So what do you propose? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Hardly, as it's a subtle runtime change. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Raising an exception of a new type is a lesser breaking change than raising a defect, the latter should be scrutinized further. Anyhow, as it's written is better than what @timotheecour is proposing. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. just an example, but see what dotnet allows/disallows: https://docs.microsoft.com/en-us/dotnet/core/compatibility/ agrees
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. "in a new code path" is not clear enough and is better left out. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. the meaning is: if the new exception can only be raised when new optional params are explicitly provided, it's an acceptable breaking change: # before:
proc fn*(a: int) = discard
# after v1:
proc fn*(a: int, dir = "") =
assert dir.len == 0 or dir.isAbsolute # with a Defect existing code that uses fn will not break # after v2:
proc fn*(a: int, dir = "") =
if dir != "" and not dir.isAbsolute: raise newException(ValueError, dir) # with a ValueError existing code that uses fn will not raise, at worse you get a CT error if user has There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sounds reasonable, but this rule can added later. |
||||||||
|
||||||||
- Adding unconstrained or poorly constrained generic procs or macros | ||||||||
("hash now works for all `ref T`"): This may cause code to behave differently | ||||||||
depending only on which modules are imported - common examples include `==` and `hash`. | ||||||||
|
||||||||
- Changing behavior of existing functions like: | ||||||||
|
||||||||
* "Nim's path handling procs like `getXDir` now consistently lack the trailing slash" | ||||||||
* "Nim's strformat implementation is now more consistent with Python" | ||||||||
|
||||||||
Instead write new code that explicitly announces the feature you think we announced but | ||||||||
didn't. For example, `strformat` does not say "it's compatible with Python", it | ||||||||
says "inspired by Python's f-strings". This new code can be submitted to the stdlib | ||||||||
and the old code can be deprecated or it can be published as a Nimble package. | ||||||||
|
||||||||
Sometimes, a run-time breaking change is most desirable: For example, a string | ||||||||
represenation of a floating point number that "roundtrips" is much better than | ||||||||
Araq marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||
a string represenation that doesn't. These run-time breaking changes must start in the | ||||||||
state "opt-in" via a new `-d:nimPreviewX` or command line flag and then should become | ||||||||
the new default later, in follow-up versions. This way users can track | ||||||||
regressions more easily. ("git bisect" is not an acceptable alternative, that's for | ||||||||
Nim compiler developers, not for Nim users.) | ||||||||
|
||||||||
Above all else, additive approaches that don't change existing behaviors | ||||||||
should be preferred. | ||||||||
|
||||||||
|
||||||||
Compile-time breaking changes | ||||||||
----------------------------- | ||||||||
|
||||||||
Compile-time breaking changes are usually easier to handle, but for large code bases | ||||||||
it can also be much work and it can hinder the adoption of a new Nim release. | ||||||||
Additive approaches are to be preferred here as well. | ||||||||
|
||||||||
Examples of compile-time breaking changes include (but are not limited to): | ||||||||
|
||||||||
* Renaming functions and modules, or moving things. Instead of a direct rename, | ||||||||
deprecate the old name and introduce a new one. | ||||||||
* Renaming the parameter names: Thanks to Nim's "named parameter" calling syntax | ||||||||
like `f(x = 0, y = 1)` this is a breaking change. Instead live with the existing | ||||||||
parameter names. | ||||||||
* Adding an enum value to an existing enum. Nim's exhaustive case statements stop | ||||||||
compiling after such a change. Instead consider to introduce new `bool` | ||||||||
fields/parameters. This can be impractical though, so we use good judgement | ||||||||
and our list of "important packages" to see if it doesn't break too much code | ||||||||
out there in practice. | ||||||||
* Adding a new proc to an existing stdlib module. However, for aesthetic reasons | ||||||||
this is often preferred over introducing a new module with just a single proc | ||||||||
inside. We use good judgement and our list of "important packages" to see if | ||||||||
it doesn't break too much code out there in practice. The new procs need to | ||||||||
be annotated with a `.since` annotation. | ||||||||
|
||||||||
|
||||||||
Compiler/language spec bugfixes | ||||||||
------------------------------- | ||||||||
|
||||||||
This can even be applied to compiler "bugfixes": If the compiler should have been | ||||||||
"pickier" in its handling of `typedesc`, instead of "fixing typedesc handling bugs", | ||||||||
consider the following solution: | ||||||||
|
||||||||
- Spec out how `typedesc` should really work and also spec out the cases where it | ||||||||
should not be allowed! | ||||||||
- Deprecate `typedesc` and name the new metatype something new like `typeArg`. | ||||||||
- Implement the spec. | ||||||||
|
||||||||
|
||||||||
Non-breaking changes | ||||||||
-------------------- | ||||||||
|
||||||||
Examples of changes that are considered non-breaking (or acceptable breaking changes) include: | ||||||||
|
||||||||
* Creating a new module. | ||||||||
* Adding an overload to an already overloaded proc. | ||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
stdlib is full of them, cementing bugs for sake of stability at the stage of maturity where nim is a bad trade-off. The alternative is introducing things like There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. "according to spec or doc comment" is a real bug, "doesn't work like in Bash/Python/D" is not a bug, it's just something that annoys you. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. well, i wrote wrote path handling is another typical case. if There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't accept your proposed wording: Code out there is not written against the spec, it's written against the existing implementation. The bugfixes you describe are all about changing the runtime behavior in subtle ways. These fixes must be opt-in first, then opt-out and then we can eventually remove the code that keeps opt-out from working. It's more work on our part but it helps our users and doesn't imply an os2.nim module. |
||||||||
* Adding new default parameters to an existing proc. It is assumed that you do not | ||||||||
use Nim's stdlib procs's addresses (that you don't use them as first class entities). | ||||||||
* Changing the calling convention from `nimcall` to `inline` | ||||||||
(but first RFC https://github.com/nim-lang/RFCs/issues/396 needs to be implemented). | ||||||||
* Changing the behavior from "crashing" into some other, well documented result (including | ||||||||
raising a Defect, but not raising an exception that does not inherit from Defect). | ||||||||
* Adding new fields to an existing object. | ||||||||
|
||||||||
Nim's introspection facilities imply that strictly speaking almost every addition can | ||||||||
break somebody's code. It is impractical to care about these cases, a change that only | ||||||||
affects introspection is not considered to be a breaking change. |
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.
but that's the whole point of #18496; it would allow legacy behaviors localized to a scope (eg nimble package); as noted in PR+RFC, this can be generalized to other behaviors:
system.delete
)These strategies can work, and be less painful than alternatives.
Bad initial design can also create long term problems, which is why we should consider approaches based on tooling to ease transitions (solutions based on
std/os2
have serious drawbacks that can be worse than the breaking change in consideration).#18513 also helps transition, by allowing to show which lines of code need to be migrated.
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.
#18496 is completely unproven at this point. I don't like the idea but even if I liked it, it's impossible to tell the consequences. For example, maybe #18496 triggers dozens of compiler bugs that even though they always already existed, make the feature unusable in practice for the next months and years.
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.
given how unattractive the other options are, we should at least explore this before saying it might trigger compiler bugs. A feature can be useful even if it has limitations.
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.
"first opt-in, then opt-out, then removed" is not that unattractive and doesn't need yet another compiler feature. Complexity breeds complexity.