--nilseqs:on should show warning when nil is used; or add --nilseqs:warning option

[timotheecour]

latest devel makes nil string an error instead of a deprecation warning; it breaks some packages, eg nitely/nim-regex#17
using --nilseqs:on is too blunt of a workaround as it doesn't show where the code needs to be fixed, so there's no way currently to list all places where a nil is wrongly used in a string/seq context. This is especially bad in case of third party (eg nimble) dependencies: right now I dont' have easy way to tell which packages will fail because of nil unless I fix errors in all these 3rd party packages 1 by one, which is impractical

proposal

• either make --nilseqs:on show a warning when nil is used or add an option --nilseqs:warning to do just that

• EDIT (minor point): Error: usage of '==' is a user-defined error is confusing (/cc @Araq)
  bugs/compiler/t18_isNil.nim:

block:
  var s:string
  # Error: usage of '==' is a user-defined error
  echo s == nil

=> could we make it either a regular mismatch error (showing overloads of ==) or Error: <nil> invalid value for a string

regressions caused by nil

• on latest devel, Error: type mismatch: got <nil> but expected 'string' nitely/nim-regex#17
• /Users/timothee/.nimble/pkgs/httpbeast-0.2.0/httpbeast.nim(58, 44) Error: type mismatch: got <nil> but expected 'string' dom96/httpbeast#7

[Araq]

It's not clear what to do. if x == nil can produce a warning and be mapped to false as strings cannot be nil anymore but at the same time the default also changed to "", so we can map it to if x.len == 0. It seemed wiser to force people into biting the bullet and do a decent analysis on their own how the code needs to be patched.

[nitely]

well, in nim-regex case, compiling the code did show where the error is, I had proc initGroupStart(name: string = nil, flags: seq[Flag] = nil) and I guess the alternative now is proc initGroupStart(name: string = "", flags: seq[Flag] = @[]). I'll miss using nil for uninitialized checks, but I won't miss the bugs it caused.

[timotheecour]

@nitely

well, in nim-regex case, compiling the code did show where the error is

it only shows either 0 or 1 error depending on --nilseqs and stops at 1st location where nil is wrongly used. If --nilseqs:on showed a warning, you'd get all locations at once. (IIRC there are more such locations, eg not ls.s.isNil where s is a seq)

@Araq

It's not clear what to do

I'm not talking about what should be the value of if x == nil (true or false) when --nilseqs:on is used , I'm talking about making it produce a warning.

The code owner will have to fix his code on a case by case basis (sometimes it requires adding a field in some data structure as @Araq did in dae5450 : cIsEmpty: bool)

It seemed wiser to force people into biting the bullet and do a decent analysis on their own how the code needs to be patched.

with my suggestion of a warning, i'll be much more likely the code will end up being patched soon. Otherwise I currently HAVE to use --nilseqs:on in my nim.cfg which hides this elephant completely.
Let's expose this elephant!

[Araq]

it only shows either 0 or 1 error depending on --nilseqs and stops at 1st location where nil is wrongly used. If --nilseqs:on showed a warning, you'd get all locations at once. (IIRC there are more such locations, eg not ls.s.isNil where s is a seq)

nim check doesn't stop after the first error.

I'm not talking about what should be the value of if x == nil (true or false) when --nilseqs:on is used , I'm talking about making it produce a warning.

But if we warn about this, we still need to decide how to compile it.

Otherwise I currently HAVE to use --nilseqs:on in my nim.cfg which hides this elephant completely.
Let's expose this elephant!

Well --nilseqs:on could also produce warnings automatically, this doesn't require yet another switch.

[timotheecour]

But if we warn about this, we still need to decide how to compile it.

Ok, the snippet below show's there's just no good default behavior, it'll be a source of bugs (worst case: potentially subtle ones that only trigger in rare conditions, or only in production etc).

var s:string
if s is nil:
  initialize1()

for i in 0..10:
  if s is nil:
    s = ""
    initialize2()
  restOfCode()

* behavior 1: s is nil: false for ""
=> initialize1 never called (previous behavior: once) => BUG

* behavior 2: s is nil: true for ""
=> initialize2 called more than once (previous behavior: once) => BUG

how about this:

--nilseqs:off: default => compile error
--nilseqs:assert => show warning and throw runtime assert when `s == nil` is called
--nilseqs:on: => show warning and default to behavior 2 (in my very subjective opinion, least evil option) => here be dragons!!

rationale

--nilseqs:off is the safest, but also prevents valid code from compiling just because some 3rd party dependency hasn't been updated to new behavior, even if it's not called
--nilseqs:assert is also safe from above kind of error, and allows code that depends on 3rd party dependency that hasn't been updated to compile and work just as it did before; but it'll trigger runtime assert if a code path leads to s == nil (which is much better than causing potential other harm)
--nilseqs:on is the real danger here, which could do all sorts of evil things, data corruption etc.

[Araq]

One year later and people had enough time to update their code -- with or without this possible improvement.