breaking changes policies

[Araq]

Note: Constructive critique is most welcome but if there continues to be no agreement, I might use my BDFL powers and push for a solution...

[timotheecour]

Raising a Defect should be considered an acceptable breaking change when the defect is called for

[saem]

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.

[Araq]

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

[Araq]

Raising a Defect should be considered an acceptable breaking change when the defect is called for

Hardly, as it's a subtle runtime change.

[saem]

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.

[timotheecour]

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.

[Araq]

"in a new code path" is not clear enough and is better left out.

[timotheecour]

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)

[Araq]

Sounds reasonable, but this rule can added later.

[timotheecour]

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.

[Araq]

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

[timotheecour]

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.

[Araq]

"first opt-in, then opt-out, then removed" is not that unattractive and doesn't need yet another compiler feature. Complexity breeds complexity.

[dom96]

Well written and makes sense. Looks good :)

[timotheecour]

A fair test for whether this is a realistic policy would be to assess what percentage of PRs since 1.0 would be acceptable under these guidelines.

[Araq]

A fair test for whether this is a realistic policy would be to assess what percentage of PRs since 1.0 would be acceptable under these guidelines.

I don't see how that would be the case given that we already know plenty of changes were done not following these policies. Which is not surprising since the policies are new...

[dom96]

I think we should try these, we can always discuss changse later if we run into problems.

[timotheecour]

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.

[Araq]

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

[timotheecour]

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)

[Araq]

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.

[timotheecour]

I don't see how that would be the case given that we already know plenty of changes were done not following these policies. Which is not surprising since the policies are new...

and yet we've managed to exercise common sense each time a breaking change was knowingly introduced, relying on practical matters (how much impact on important_packages sample, how difficult is it to make code fwd/backward compatible) over other considerations. nim in devel is considerably better than 1.4, and likewise for each prior release, thanks to, not in spite of, the breaking changes/bug fixes introduced.

Claiming "we finally nailed it now" just ignores history and the same promises made in 1.0 (no breaking change apart from security related + experimental parts of language) will have to be broken again. All I'm arguing for is exercising common sense:

• breaking changes are bad but sometimes this is the lesser evil
• use important_packages (and grow it) as proxy to assess damage, along with difficulty of patching broken code
• improve tooling to help mitigate breaking changes:
  • identify impacted lines of code (add {.migrated(ident, msg):... .} to issue callsite migration warnings for changed APIs #18513)
  • offer localizable migration (--moduleoverride:mod1:mod2:prefix: generalized patchFile to allow non-global effect; works on cmdline / config #18496 + followup improvements)
  • -d:nimTransitionX should not have to wait for a major version bump to become default on

Lots of people also want bugfixes even when it implies breaking changes, see reactions to #18468 (comment) or to #18480

[Araq]

breaking changes are bad but sometimes this is the lesser evil

Subtle changes in run-time behavior are not the "lesser evil". Even os2 (which I didn't bring up this time btw) would be the lesser evil than that as we could design a solid API for it first.

use important_packages (and grow it) as proxy to assess damage, along with difficulty of patching broken code

Difficulty of patching production code that relies on runtime edge cases: High.

How do I know? I still patch old code that uses s[i] without a s.len check as that used to be valid Nim code.

Claiming "we finally nailed it now" just ignores history...

I don't claim that. I claim that these policies will work better than what we use today, yes. How will we know for sure? By trying it.

[Araq]

I'm merging this but follow-up PRs can further refine the rules.