Extended side effect error messages

[quantimnot]

Fixes #4210
Fixes #10712

Is this the right approach?
Any desired changes to make the messages clearer?
All feedback is welcome.

[quantimnot]

I just found nim-lang/RFCs#323 and nim-lang/RFCs#324.

I'm not sure what to apply from those discussions.

[quantimnot]

tests/effects/tdiagnostic_messages.nim:

var globalVar = 0
proc indirectCallViaVarParam(call: var proc(): int {.nimcall.}): int =
  call()
proc indirectCallViaPointer(call: pointer): int =
  cast[ptr proc(): int](call)[]()
proc callWithSideEffects(): int =
  var p = proc (): int {.nimcall.} = 0
  discard indirectCallViaVarParam(p)
  discard indirectCallViaPointer(addr p)
  echo ""
  globalVar

func a: int =
  discard callWithSideEffects()

Here is the current state of rendering:

Terminal:

tdiagnostic_messages.nim(31, 6) Error: 'a' has side effects
  tdiagnostic_messages.nim(32, 30) Hint: 'a' calls 'callWithSideEffects' which has side effects
    tdiagnostic_messages.nim(24, 6) Hint: 'callWithSideEffects' called by a `noSideEffect` routine, but has side effects
      tdiagnostic_messages.nim(26, 34) Hint: 'callWithSideEffects' calls 'indirectCallViaVarParam' which has side effects
        tdiagnostic_messages.nim(20, 6) Hint: 'indirectCallViaVarParam' called by a `noSideEffect` routine, but has side effects
          tdiagnostic_messages.nim(21, 7) Hint: 'indirectCallViaVarParam' calls a routine with side effects via hidden pointer indirection
      tdiagnostic_messages.nim(27, 33) Hint: 'callWithSideEffects' calls 'indirectCallViaPointer' which has side effects
        tdiagnostic_messages.nim(22, 6) Hint: 'indirectCallViaPointer' called by a `noSideEffect` routine, but has side effects
          tdiagnostic_messages.nim(23, 32) Hint: 'indirectCallViaPointer' calls a routine with side effects via pointer indirection
      tdiagnostic_messages.nim(28, 8) Hint: 'callWithSideEffects' calls 'echo' which has side effects
        ../../lib/system.nim(2004, 6) Hint: 'echo' called by a `noSideEffect` routine, but has side effects
      tdiagnostic_messages.nim(29, 3) Hint: 'callWithSideEffects' accesses global state 'globalVar'
        tdiagnostic_messages.nim(19, 5) Hint: 'globalVar' accessed by a `noSideEffect` routine

VSCode:

[quantimnot]

The terminal rendering's structure gives context to the hints about what noSideEffect routine is trying to use routines with side effects.
The VSCode hints don't provide that context. Should I change them to someting like:
Hint: 'indirectCallViaVarParam' called by `a`

[timotheecour]

LGTM, excellent addition

[RSDuck]

please rather look at https://github.com/saem/vscode-nim as it's error parsing code has gotten some fixes (and also feature additions). https://github.com/pragmagic/vscode-nim is somewhat dead.

[quantimnot]

Don't merge. I just notice a small bug. Give me a minute.

[quantimnot]

@timotheecour
Please take another look at the message changes I just made. I'm exhausted and my mind isn't working.
Ready to merge if it's ok.

@RSDuck
I just submitted a PR to support the leading '>' in the diagnostic messages.

[quantimnot]

Current rendering with saem/vscode-nim:

[quantimnot]

I don't know why the checks failed:

cmd: git -C csources_v1 checkout a8a5241f9475099c823cfe1a5e0ca4022ac201ff
fatal: reference is not a tree: a8a5241f9475099c823cfe1a5e0ca4022ac201ff

[Araq]

I don't know why the checks failed

Me neither, I'm investigating...

[timotheecour]

I don't know why the checks failed

retry now that #18427 was merged

[quantimnot]

Don't merge. I getting an exception while using nimsuggest:

/Volumes/origin/ws/ports/rdn/org.nim-lang.nim/origin.src/compiler/sempass2.nim(292) listSideEffects
/Volumes/origin/ws/ports/rdn/org.nim-lang.nim/origin.src/lib/pure/strutils.nim(2698) addf
/Volumes/origin/ws/ports/rdn/org.nim-lang.nim/origin.src/lib/pure/strutils.nim(2638) invalidFormatString
Error: unhandled exception: invalid format string [ValueError]

[timotheecour]

@Araq is there anything else blocking this PR to be merged? there has been no pending review comment since 8 days ago

Leaving PRs open for a long time guarantees bitrot and eventual demotivation from PR author, eventually joining the train of the 200+ open abandoned PRs where original author gave up, despite being in a mergeable state at some point.

the concern regarding "how costly is this" was answered in #18418 (comment)

[Araq]

Sorry, it said

Don't merge. I getting an exception while using nimsuggest:

Will test it on my machine wrt memory consumption and then merge it.

[timotheecour]

Sorry, it said [...]

that was fixed in the "fixed double string formatting bug" commit;
if anything, this proves that the "PTAL" label is a good idea (author can add it when ready for next round/merge, anyone can remove it when comments are added / some action is needed on the PR author part)

[Araq]

Can confirm that the memory and CPU consumption is acceptable.

[Araq]

Btw this needs a changelog entry.