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

Jump to bottom

WIP #781

Closed
timotheecour opened this issue Jul 10, 2021 · 6 comments
Closed

WIP #781

timotheecour opened this issue Jul 10, 2021 · 6 comments

Comments

@timotheecour
Copy link
Owner

timotheecour commented Jul 10, 2021 •
edited
Loading

example PRs that were needed despite being breaking changes, demonstrating that the "no breaking change promise" made in 1.0 was broken many times, will continue to be broken, and is untenable in the current state of nim compiler and stdlib maturity level.

Indeed, it breaks weave mratsim/weave#173

this compiles before PR and doesn't after PR

rev

see also:

We have this tool already, it's called .deprecated: "use X instead". Old code keeps working, new code avoids the quirky proc since there is an alternative provided.

Great, so if there's a bug (performance, logic, etc) in hash, we just need to deprecate hash and introduce hash2 or std/hashes2 which produces different runtime values to avoid collisions. Since we can't silently change behavior of tables in stdlib by using the new std/hashes because someone might depend on old behavior tied to the old hash, we need to introduce std/tables2, std/sets2, and in turn std/json2 since it depends on tables, and transitively every module needs to be duplicated. Any client of any of those dependencies will have to be updated to use the new modules and in the process cut support for prior nim versions, creating a mess of incompatible duplicate modules that need to be maintained and break nimble ecosystem beyond repair.

This problem is pervasive, with your logic we have to deprecate and find a new symbol name for sugar.=>, os./, we have to deprecate std/json, std/macros

Nowadays I avoid jsonutils.nim and parts of the stdlib because of this cowboy style attitude to breaking changes

I also recommend you should avoid std/[json, tables, strutils, sequtils, os, macros] and indeed, just about any stdlib module because of the same cowboy attitude which views bugs as things that should be fixed, not worked around, if nim is to be taken seriously.

nim 1.0 release made a promise to offer stability guarantee (with an exception for "security vulnerability" and experimental features to a good approximation); I was against making that promise because the immaturity of the language and standard library would require breaking that promise in many occasions in order to allow evolving the language and free stdlib from design bugs. The reality is that the stability guarantee has been broken on many occasions, for good reasons (unrelated to security or experimental features or modules).

Here's a tiny sample of silently changed runtime behavior (aka what your call "cowboy attitude") that would require introducing duplicate modules, renaming $, /, => etc:

# `=>` would need to be renamed or sugar2 would need to be introduced:
import std/sugar
proc bar(a: float, fn: proc(x: float, y: int): string): auto = 1
proc bar[T](a: T, fn: proc(x: int, y: int): string): auto = 2
echo bar(5.0, (x, y: int) => $(x, y)) # since 1.4: prints 2, until 1.2: prints 1

# `$` would need to be renamed because of changed behavior:
echo $(int,).type # 1.2: (int); 1.4: (int,)

# type conversion would have to be deprecated:
let a = not 0'u64
echo int64(a) # 1.2: prints -1; 1.4: RangeDefect

# json would have to be deprecated:
import std/json
let a = 18446744073709551615'u64
echo %(a) # until 1.2: prints -1; 1.5.1: prints 18446744073709551615

let a = [NaN, Inf, -Inf, 0.0, -0.0, 1e20, 1.5]
let j = %a
echo $j # prints json that is invalid according to spec (fixed in 1.5.1 https://github.com/nim-lang/Nim/pull/18026)

# macros would have to be deprecated because of newLit:
import std/[macros,typetraits]
macro fn(): untyped =
  result = newLit((1, 2))
echo fn().type.isNamedTuple # 1.2: true; 1.4: false (refs 408518c9fed9757fd1cc246e3ced05ecbb3bcaab)

# std/os and `/` would have to be deprecated
import std/os
echo isAbsolute("/foo") # 1.2 (nim js): false; 1.4: true

echo "abc" / "/" # 1.0: abc; 1.2: abc/ (fixed in https://github.com/nim-lang/Nim/pull/13467)
echo "abc" / "" # 1.0: abc/; 1.2: abc

# relativePath(rel, abs) and relativePath(abs, rel) used to silently give wrong results (see #13222); instead they now use `getCurrentDir` to resolve those cases, and can now throw in edge cases

# plus lots of other examples of (silent) changed behavior in each release in std/os

How do you know that old code in a foreign Nimble package needs inspection? You compile it as your main project and look at the deprecation warnings.

and what good does it do when you have no control over it and your dependencies generate thousands of warnings due to this policy which would cause most of stdlib APIs/modules to be deprecated?

The fallacy here is that you can somehow secretly "fix" code that used the stdlib by changing the stdlib's behavior. This is only important for security related issues.

The fallacy here is because of transitive dependencies, anything can be considered a "security related issue". Mission critical projects should have proper tests in place when updating dependencies including upgrading to a new compiler release.
@juancarlospaco
Copy link
Collaborator

I think the only solution is to hurry up Nim 2.0.
so all Deprecated can be removed, all Experimental can be default,
all breaking Fixes can be implemented...

@timotheecour
Copy link
Owner Author

timotheecour commented Jul 12, 2021 •
edited
Loading

I think the only solution is to hurry up Nim 2.0.

this won't solve the underlying problem, if a bug is discovered in 2.0, I don't want to wait 10 years until 3.0 comes out. Hence the position I'm articulating in the "stdlib policy" PR in favor of fixing bugs, not working around it.

@juancarlospaco
Copy link
Collaborator

I agree, but I think about the way that fixes/improvements gets actually accepted and merged... 🤷

@ringabout
Copy link
Collaborator

ringabout commented Jul 13, 2021 •
edited
Loading

Hence the position I'm articulating in the "stdlib policy" PR in favor of fixing bugs, not working around it.

Make sense. Alas, I wish no PR would be reverted(Or at least providing the alternative way right after reverting) even if the new policy were to be merged.

@juancarlospaco
Copy link
Collaborator

This only shows people using the language wrong, they are never asserting inputs, nor outputs,
thats basic primitive type assertiong checking, with or without a revert of any fix their code is still very fragile,
but the language currently has nothing to offer to improve on that.

DrNim would be perfect, but it wont work currently, and no one knows when it will work realistically.

Testament/unittest is awesome, but tests the whole thing,
so maybe test pass even with wrong inputs and outputs, and very often input is hardcoded in the test itself.

DBC module on stdlib can catch that, theres no way it dont,
if this were Ada the DBC would catch it, theres literally an ISO standard and books about this, this has been fixed decades ago.

If you dont add DBC people keep inventing random "aliases" into unittesting tools,
like err, oks, fail, skip, ignore, pass, disallow, falsify, checkNot, etc.

Documentation then strongly recommends using DBC for all core/serious/mission critical code.
DBC can be enabled/disabled with a define flag -d:nimNoDbc or -d:nimDbc or similar, to get performance back.
The DBC module can be Deprecated when DrNim works out-of-the-box for all OS/Arch.

See

@ringabout
Copy link
Collaborator

ringabout commented Aug 23, 2021 •
edited
Loading

Not needed anymore IMO :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@juancarlospaco @timotheecour @ringabout