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

breaking changes policies #18541

Merged
merged 12 commits into from
Jul 21, 2021
114 changes: 114 additions & 0 deletions doc/contributing.rst
Original file line number Diff line number Diff line change
Expand Up @@ -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 protomote
Araq marked this conversation as resolved.
Show resolved Hide resolved
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,
Copy link
Member

@timotheecour timotheecour Jul 20, 2021

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:

  • override a whole module (that's what's implemented in the PR)
  • override a symbol (eg so the old behavior is used within a scope; eg for system.delete)
  • override whether a symbol can raise a new exception (very useful in case original implementation forgot to handle an edge case)

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.

Copy link
Member Author

@Araq Araq Jul 20, 2021

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.

Copy link
Member

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.

Copy link
Member Author

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.

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.
Copy link
Member

Choose a reason for hiding this comment

The 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

Copy link
Contributor

Choose a reason for hiding this comment

The 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.

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't understand. So what do you propose?

Copy link
Member Author

Choose a reason for hiding this comment

The 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

Hardly, as it's a subtle runtime change.

Copy link
Contributor

Choose a reason for hiding this comment

The 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.

Copy link
Member

Choose a reason for hiding this comment

The 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

✔️ ALLOWED: Throwing an exception that is considered unrecoverable

✔️ ALLOWED: Throwing a new exception in a new code path

The exception must apply only to a new code-path that's executed with new parameter values or state and that can't be executed by existing code that targets the previous version.

Copy link
Member Author

Choose a reason for hiding this comment

The 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.

Copy link
Member

@timotheecour timotheecour Jul 20, 2021

Choose a reason for hiding this comment

The 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 raises: [] on a caller of fn (falls under category of CT breaking change, not RT breaking change)

Copy link
Member Author

Choose a reason for hiding this comment

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

Sounds reasonable, but this rule can added later.


- Adding overloads or generic `proc`:s when an unconstrained
Araq marked this conversation as resolved.
Show resolved Hide resolved
generic implementation exists already: 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:nimX` or command line flag and then should become
Araq marked this conversation as resolved.
Show resolved Hide resolved
the new default later, in follow-up versions. This way users can track
regressions more easily. (And no, "git bisect" is not an acceptable alternative.)

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 too.
Araq marked this conversation as resolved.
Show resolved Hide resolved

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 annotate with a `.since` annotation.
Araq marked this conversation as resolved.
Show resolved Hide resolved


Compiler 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.
Copy link
Member

@timotheecour timotheecour Jul 20, 2021

Choose a reason for hiding this comment

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

Suggested change
* Adding an overload to an already overloaded proc.
* Adding an overload to an already overloaded proc.
* Fixing a bug (implementation is wrong according to spec or doc comment)

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 std/os2 whose drawbacks have been extensively discussed in previous attempts at defining a policy.

Copy link
Member Author

Choose a reason for hiding this comment

The 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.

Copy link
Member

@timotheecour timotheecour Jul 20, 2021

Choose a reason for hiding this comment

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

well, i wrote wrote according to spec or doc comment. If a module implements a json API (eg std/json), producing invalid json is a bug according to an external spec, so #18026 should remain an acceptable breaking change

path handling is another typical case. if C:foo\bar is currently incorrectly treated as absolute; fixing this should be considered an acceptable breaking change; ditto with C:\ which is currently normalized as C: instead of C:\ which is incorrect (becomes relative)
(refs https://docs.microsoft.com/en-us/dotnet/standard/io/file-path-formats)

Copy link
Member Author

@Araq Araq Jul 21, 2021

Choose a reason for hiding this comment

The 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.
Araq marked this conversation as resolved.
Show resolved Hide resolved

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.