From a7eaac7f57ac1b0c4d1897a1beae0cafd3811790 Mon Sep 17 00:00:00 2001 From: Jacek Sieka Date: Mon, 12 Jul 2021 18:00:18 +0200 Subject: [PATCH 01/11] document some std library evolution policies In wanting to improve the standard library, it's helpful to have a set of principles and guidelines to lean back on, that show how to introduce such improvements while at the same time considering existing users of the language. A key idea behind documentation of this sort is that it should highlight a path forwards in making changes rather than trying to prevent them, and although the current snippet does include some language for what one shouldn't do, it also shows a few options for what one can do. This is a riff on https://github.com/nim-lang/Nim/pull/18468 mainly based on what helps enable to the use of Nim productively in environments and teams where the priority and focus is not always on the tool (Nim in this case) but rather on the codebase itself, and its use by end users. We use similar guidance documentation to help coordinate the code of the teams using Nim in https://status-im.github.io/nim-style-guide/ where it is applied not as law, but rather as recommendations for the default approach that should be considered first. --- doc/contributing.rst | 45 ++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 45 insertions(+) diff --git a/doc/contributing.rst b/doc/contributing.rst index 5a31d85bdec80..64a168c834240 100644 --- a/doc/contributing.rst +++ b/doc/contributing.rst @@ -671,3 +671,48 @@ 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. + +Introducing change +------------------ + +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. Above all else, additive +approaches that don't change existing behaviors should be preferred. + +Examples of breaking changes include (but are not limited to): + +* renaming functions and modules, or moving things +* raising exceptions of a new type, compared to what's currently being raised +* changing behavior of existing functions + * this includes inputs around edge cases or undocumented behaviors +* adding overloads or generic `proc`:s, in particular when an unconstrained generic implementation exists already + * this may cause code to behave differently depending only on which modules are imported - common examples include `==` and `hash` +* hiding the old behavior behind a `-d:nimLegacy` flag + * legacy flags are suitable for use between major, breaking release of the langauge or non-breaking changes that nontheless are deemed sensitive + +Examples of changes that are considered non-breaking include: + +* creating a new module, or adding a function with a new name + * this may create compile-time issues due to name conflicts +* changing the behavior of a function where previously it was crashing, raising `Defect` or giving an incorrect result +* addressing issues whose invalid or undefined behavior can be made a compile-time error + +Some behaviors in the standard library are inconsistent or unfortunate. Even so, code may have come to depend on the quirks, and it's often not possible to tell. When encountering such behaviors, consider the following approaches instead: + +* document the quirk in manual and tutorials to raise awareness +* create a separate, stand-alone Nimble package and deprecate the existing function or module + * this allows the API to evolve at its own pace - in stand-alone packages, it's also easier to introduce breaking changes since they are not bundled with other changes + * the deprecation message should explain why the existing behavior is problematic +* deprectate the existing function or module, and create a new implementation in a separate module + * this approach is suitable for functionality closely tied to the language +* introduce the new behavior behind a `-d:nimEnable` flag + * this approach is suitable for experimental changes and changes which will become default in a future breaking release of Nim, allowing early adopters to experiment with the new behavior + +Introducing breaking changes in the standard library, no matter how well intentioned, creates long-term problems for the community, in particular those looking to protomote 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 + +These guidelines apply to released versions of the language - `devel` can always be expected to change. From 482bb59cb84bfd5092cd9e84802529f71da884c1 Mon Sep 17 00:00:00 2001 From: Araq Date: Tue, 20 Jul 2021 09:58:00 +0200 Subject: [PATCH 02/11] document the new policies --- doc/contributing.rst | 134 ++++++++++++++++++++++++++++++++----------- 1 file changed, 102 insertions(+), 32 deletions(-) diff --git a/doc/contributing.rst b/doc/contributing.rst index 64a168c834240..44ec9ccd79be5 100644 --- a/doc/contributing.rst +++ b/doc/contributing.rst @@ -672,47 +672,117 @@ Conventions letters, which cause issues when going from an OS without case sensitivity to an OS with it. -Introducing change ------------------- -Nim is used for mission critical applications which depend on behaviours that +Breaking Changes +================ + +Introducing breaking changes, no matter how well intentioned, +creates long-term problems for the community, in particular those looking to protomote +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. Above all else, additive -approaches that don't change existing behaviors should be preferred. +behaviors, even when the test suite continues to pass. -Examples of breaking changes include (but are not limited to): +Examples of run-time breaking changes: -* renaming functions and modules, or moving things -* raising exceptions of a new type, compared to what's currently being raised -* changing behavior of existing functions - * this includes inputs around edge cases or undocumented behaviors -* adding overloads or generic `proc`:s, in particular when an unconstrained generic implementation exists already - * this may cause code to behave differently depending only on which modules are imported - common examples include `==` and `hash` -* hiding the old behavior behind a `-d:nimLegacy` flag - * legacy flags are suitable for use between major, breaking release of the langauge or non-breaking changes that nontheless are deemed sensitive +- Raising exceptions of a new type, compared to what's currently being raised. + +- Adding overloads or generic `proc`:s when an unconstrained + 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 +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 +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. -Examples of changes that are considered non-breaking include: -* creating a new module, or adding a function with a new name - * this may create compile-time issues due to name conflicts -* changing the behavior of a function where previously it was crashing, raising `Defect` or giving an incorrect result -* addressing issues whose invalid or undefined behavior can be made a compile-time error +Compile-time breaking changes +----------------------------- -Some behaviors in the standard library are inconsistent or unfortunate. Even so, code may have come to depend on the quirks, and it's often not possible to tell. When encountering such behaviors, consider the following approaches instead: +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. + +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. + + +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 include: -* document the quirk in manual and tutorials to raise awareness -* create a separate, stand-alone Nimble package and deprecate the existing function or module - * this allows the API to evolve at its own pace - in stand-alone packages, it's also easier to introduce breaking changes since they are not bundled with other changes - * the deprecation message should explain why the existing behavior is problematic -* deprectate the existing function or module, and create a new implementation in a separate module - * this approach is suitable for functionality closely tied to the language -* introduce the new behavior behind a `-d:nimEnable` flag - * this approach is suitable for experimental changes and changes which will become default in a future breaking release of Nim, allowing early adopters to experiment with the new behavior +* Creating a new module. +* Adding an overload to an already overloaded proc. +* 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 convetion 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). -Introducing breaking changes in the standard library, no matter how well intentioned, creates long-term problems for the community, in particular those looking to protomote reusable Nim code in libraries: +* Adding new fields to an existing object. -* 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 +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. -These guidelines apply to released versions of the language - `devel` can always be expected to change. From 689f819b1af81e398614fe39add611f1e7de6faa Mon Sep 17 00:00:00 2001 From: Araq Date: Tue, 20 Jul 2021 10:02:58 +0200 Subject: [PATCH 03/11] typo --- doc/contributing.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/doc/contributing.rst b/doc/contributing.rst index 44ec9ccd79be5..ff1dd6c09ffc7 100644 --- a/doc/contributing.rst +++ b/doc/contributing.rst @@ -775,7 +775,7 @@ Examples of changes that are considered non-breaking include: * Adding an overload to an already overloaded proc. * 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 convetion from `nimcall` to `inline` +* 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). From c441694bc953677d584f01ff6573cabc81b96048 Mon Sep 17 00:00:00 2001 From: Andreas Rumpf Date: Tue, 20 Jul 2021 10:47:10 +0200 Subject: [PATCH 04/11] Update doc/contributing.rst Co-authored-by: Timothee Cour --- doc/contributing.rst | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/doc/contributing.rst b/doc/contributing.rst index ff1dd6c09ffc7..eb61361304593 100644 --- a/doc/contributing.rst +++ b/doc/contributing.rst @@ -769,7 +769,7 @@ consider the following solution: Non-breaking changes -------------------- -Examples of changes that are considered non-breaking include: +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. @@ -785,4 +785,3 @@ Examples of changes that are considered non-breaking include: 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. - From 8509fb1e7ebdd09a0716c8679084600bcd993496 Mon Sep 17 00:00:00 2001 From: Andreas Rumpf Date: Tue, 20 Jul 2021 11:04:36 +0200 Subject: [PATCH 05/11] Update doc/contributing.rst --- doc/contributing.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/doc/contributing.rst b/doc/contributing.rst index eb61361304593..696e91f521cd9 100644 --- a/doc/contributing.rst +++ b/doc/contributing.rst @@ -719,7 +719,7 @@ 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 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 +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. (And no, "git bisect" is not an acceptable alternative.) From 76f038650a1f253bab3ba6850744358b92fbbe3f Mon Sep 17 00:00:00 2001 From: Andreas Rumpf Date: Tue, 20 Jul 2021 11:08:25 +0200 Subject: [PATCH 06/11] Update doc/contributing.rst Co-authored-by: konsumlamm <44230978+konsumlamm@users.noreply.github.com> --- doc/contributing.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/doc/contributing.rst b/doc/contributing.rst index 696e91f521cd9..ddeb1c6a4c9d1 100644 --- a/doc/contributing.rst +++ b/doc/contributing.rst @@ -677,7 +677,7 @@ Breaking Changes ================ Introducing breaking changes, no matter how well intentioned, -creates long-term problems for the community, in particular those looking to protomote +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 From c642f6be24443a8f2b63d22daa4a4ef6922374bc Mon Sep 17 00:00:00 2001 From: Andreas Rumpf Date: Tue, 20 Jul 2021 11:08:34 +0200 Subject: [PATCH 07/11] Update doc/contributing.rst Co-authored-by: konsumlamm <44230978+konsumlamm@users.noreply.github.com> --- doc/contributing.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/doc/contributing.rst b/doc/contributing.rst index ddeb1c6a4c9d1..377503f6e5fe0 100644 --- a/doc/contributing.rst +++ b/doc/contributing.rst @@ -732,7 +732,7 @@ 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. +Additive approaches are to be preferred here as well. Examples of compile-time breaking changes include (but are not limited to): From d9dae9914ee427b51924b67cc54edc6851b69174 Mon Sep 17 00:00:00 2001 From: Andreas Rumpf Date: Tue, 20 Jul 2021 11:11:07 +0200 Subject: [PATCH 08/11] Update doc/contributing.rst --- doc/contributing.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/doc/contributing.rst b/doc/contributing.rst index 377503f6e5fe0..7064b728508e4 100644 --- a/doc/contributing.rst +++ b/doc/contributing.rst @@ -750,7 +750,7 @@ Examples of compile-time breaking changes include (but are not limited to): 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. + be annotated with a `.since` annotation. Compiler bugfixes From 017cc98526ac1967578f6ab162ae926c8fd1c564 Mon Sep 17 00:00:00 2001 From: Andreas Rumpf Date: Tue, 20 Jul 2021 11:13:42 +0200 Subject: [PATCH 09/11] Update doc/contributing.rst Co-authored-by: konsumlamm <44230978+konsumlamm@users.noreply.github.com> --- doc/contributing.rst | 1 - 1 file changed, 1 deletion(-) diff --git a/doc/contributing.rst b/doc/contributing.rst index 7064b728508e4..9ab8bfc69a75c 100644 --- a/doc/contributing.rst +++ b/doc/contributing.rst @@ -779,7 +779,6 @@ Examples of changes that are considered non-breaking (or acceptable breaking cha (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 From 1d38202a85a731e1cc1d09c9107cfeca2c3320d6 Mon Sep 17 00:00:00 2001 From: Araq Date: Tue, 20 Jul 2021 15:18:42 +0200 Subject: [PATCH 10/11] clarify some things --- doc/contributing.rst | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/doc/contributing.rst b/doc/contributing.rst index 9ab8bfc69a75c..bc83c6591f172 100644 --- a/doc/contributing.rst +++ b/doc/contributing.rst @@ -702,8 +702,8 @@ Examples of run-time breaking changes: - Raising exceptions of a new type, compared to what's currently being raised. -- Adding overloads or generic `proc`:s when an unconstrained - generic implementation exists already: This may cause code to behave differently +- 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: @@ -721,7 +721,8 @@ represenation of a floating point number that "roundtrips" is much better than 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. (And no, "git bisect" is not an acceptable alternative.) +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. @@ -753,8 +754,8 @@ Examples of compile-time breaking changes include (but are not limited to): be annotated with a `.since` annotation. -Compiler bugfixes ------------------ +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", From a4fb06dd4f79b6aeccf5b9f83f7d0a51d47c1ed9 Mon Sep 17 00:00:00 2001 From: Andreas Rumpf Date: Tue, 20 Jul 2021 17:49:54 +0200 Subject: [PATCH 11/11] Update doc/contributing.rst Co-authored-by: Dominik Picheta --- doc/contributing.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/doc/contributing.rst b/doc/contributing.rst index bc83c6591f172..07fa017140219 100644 --- a/doc/contributing.rst +++ b/doc/contributing.rst @@ -717,7 +717,7 @@ says "inspired by Python's f-strings". This new code can be submitted to the std 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 +representation of a floating point number that "roundtrips" is much better than 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