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.

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

stdlib policy: document what we should do #18468

Closed
wants to merge 2 commits into from
Closed

stdlib policy: document what we should do #18468

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
84d4ac9
stdlib policy: document what we should do
Araq Jul 9, 2021
b09ed34
changelog typo
Araq Jul 9, 2021
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
2 changes: 1 addition & 1 deletion changelog.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -152,7 +152,7 @@

- `jsonutils.toJson` now supports customization via `ToJsonOptions`.

- Added an overload for the `collect` macro that inferes the container type based
- Added an overload for the `collect` macro that infers the container type based
on the syntax of the last expression. Works with std seqs, tables and sets.

- Added `randState` template that exposes the default random number generator.
Expand Down
17 changes: 17 additions & 0 deletions doc/contributing.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -671,3 +671,20 @@ 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.


Stdlib bugfixes
===============

Nim is used for mission critical applications where details like
"can this proc raise or not" do matter. A "bugfix" that changes the behavior
from "error is silently ignored" to "error now produces an exception" is
not acceptable -- **it is not a bug if somebody's code relies on it**.
Copy link
Contributor

Choose a reason for hiding this comment

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

I understand that changing the exception that a proc raises is a breaking change and should be avoided. But saying "it is not a bug if somebody's code relies on it" is a bad idea IMO (obligatory xkcd: https://xkcd.com/1172/). What if somebodys code relies on undocumented bugs or implementation details? What about something like #16689? What about changing the implementation of a proc to use FFI, so that it can't be used in the VM anymore? Someone always might depend on some bug, but that doesn't mean it shouldn't be considered a bug.

Copy link
Contributor

Choose a reason for hiding this comment

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

Another example: what if someone relies on something producing a compiler error (then most additions to the language would be breaking changes)? Or what if someone wrote their own proc and a module adds a proc with the same name, so suddenly the program breaks because of a name conflict, or even worse, there is no name conflict because the exported proc is more specialized and the behaviour suddenly changes (so the code relies on the module not exporting a proc with the same name)?

Copy link
Member Author

Choose a reason for hiding this comment

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

What if somebodys code relies on undocumented bugs or implementation details?

Well that's the point. We try harder not to break his code.

What about changing the implementation of a proc to use FFI, so that it can't be used in the VM anymore?

What about it? It's a breaking change and if it was covered by a CT test we would have detected it.

Someone always might depend on some bug, but that doesn't mean it shouldn't be considered a bug.

Actually, that's pretty much what I think nowadays. Bugfixes are for "previously it crashed" behaviors, many of what are currently bugfixes can be dealt with more effectively by deprecating the respective API and moving it to somewhere else.

Copy link
Member Author

Choose a reason for hiding this comment

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

Or what if someone wrote their own proc and a module adds a proc with the same name, so suddenly the program breaks because of a name conflict, or even worse, there is no name conflict because the exported proc is more specialized and the behaviour suddenly changes (so the code relies on the module not exporting a proc with the same name)?

Since the proc prototypes would be the same, I fail to see how this could happen and either way, if you end up calling a deprecated proc (with overloads or not) you get a deprecation warning.

Copy link
Contributor

Choose a reason for hiding this comment

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

Before bringing this into effect can a run through of the standard library be done marking things as experimental or weaker levels of support?

Then this level of assurance is only maintained for key parts? Heuristic being something like age + key nim feature.

Like web APIs, some changes are more breaking than others. So if we could classify these it would let people know what's allowed. Types of things:

  • object field to property, or vice versa
  • as a field to an object, tuple, enum, ...
  • func to proc downgrade
  • additional exception to existing non-empty list
  • newly raises exception
  • remove support for a platform: compile time, JS
  • add a routine parameter with default value
  • can the breakage be fixed with precise auto-migration?

It doesn't have to be exhaustive but this will start providing a way to compare and contrast; moreover, constructively debate. It should also help guide API design. For example, we should be using opaque types more often, especially for returns in order to allow adding properties or other changes in the future.

Copy link
Contributor

Choose a reason for hiding this comment

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

@Varriount brought a perfect example of what I mean:

Say there is a string replacement procedure in the standard library that has a bug, where an empty string is always returned when an input string starts with the letter "b".

This behaviour is clearly a bug and not intended, yet someone might actually rely on that. So according to "it is not a bug if somebody's code relies on it", this should not be considered a bug, which imo is plain wrong. Thus, I think this part should be left out. I assume your preferred way of handling this would be to deprecate it and make a new version? This would have a few problems though: You can't name the new routine the same as the old one when the parameters don't change and people are less likely to use the fixed version (simply because with just changing the old version, everyone would be forced to use the fixed version).

I also agree with @saem, not every std module should be considered stable (some even explicitly say they're unstable) and a comprehensive list of what is and what isn't considered to be a breaking change would be very useful, especially for new contributors.

Copy link
Contributor

Choose a reason for hiding this comment

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

Before bringing this into effect can a run through of the standard library be done marking things as experimental or weaker levels of support?

+1 on this point in general - the liberty to make breaking changes has de facto been the policy for a long time which in a way contributes to why much of the library looks like it does: why bother when you can fix it later? Some code is likely not ready to have a more strict policy applied to it without a cleanup period - one way to introduce a policy like this is to apply it to a few core modules and expand from there, also to tune the policy itself.

Copy link
Contributor

Choose a reason for hiding this comment

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

Like web APIs, some changes are more breaking than others.

most likely though, these are candidates for a "deprecate-and-move-to-package" policy where they can develop at a faster pace, disconnected from the Nim development process in general (ie why does the release cycle of a web API be tied to that of a compiler?)


Instead, deprecate the proc with the questionable edge case behavior and
introduce a new proc in a new module. It is much easier to handle a compiler
update that merely produces more deprecation warnings.

This principle also generalizes to full modules: Deprecate the questionable
module, delegate to a new Nimble package where the API can evolve in its
own pace.