Make 'echo' raise IOErrors when appropriate

[iffy]

Attempt to address #16366 and to see what breaks with this change.

[iffy]

Is there any other real-world testing we could subject this change to before merging? Changing the behavior of something as commonly-used as echo (especially adding a new failure mode) seems like it could cause users problems. It seems like a breaking change, at least.

On the other hand, I can't imagine anyone depends on or intentionally exploits the current no-raise behavior.

[timotheecour]

Is there any other real-world testing we could subject this change to before merging

how about add -d:nimLegacyEchoNoRaise to control this; analog to the other ones:

changelog.md:52:23:  see #16034. Use `-d:nimLegacyReprWithNewline` for previous behavior.
changelog.md:85:55:- Type mismatch errors now show more context, use `-d:nimLegacyTypeMismatch` for previous
compiler/types.nim:1494:24:    if conf.isDefined("nimLegacyTypeMismatch"):
lib/system/repr.nim:328:18:    when defined(nimLegacyReprWithNewline): # see PR #16034
lib/system/reprjs.nim:240:16:  when defined(nimLegacyReprWithNewline): # see PR #16034
lib/system.nim:1338:16:    ## Use `-d:nimLegacyReprWithNewline` to revert to old behavior where newlines
tests/config.nims:16:19:switch("define", "nimLegacyTypeMismatch")

(and mention in changelog)

IMO that's good enough.

[juancarlospaco]

debugEcho would still crash?
if not, then tests should test debugEcho too, so no one breaks it in the future.

[timotheecour]

debugEcho would still crash?

I think it should also raise; it's easy enough to wrap it inside try/except in the off-chance it does raise in your code; the main point is to evade noSideEffects; silently hiding errors shouldn't be the goal

[timotheecour]

this seems unfortunately subject to the classic EINTR bug, see https://github.com/angrave/SystemProgramming/wiki/POSIX%2C-Part-1%3A-Error-handling#what-is-eintr-what-does-it-mean-for-sem_wait-read-write and how I fixed it here #13232 via a while loop which is IIUC the correct way to handle this

the question is whether to fix this in this PR or merge this PR as is and fix it in future PR; is there an easy repro?

[iffy]

I think do it as a separate PR, abstracting out a common template in the process (as mentioned in your fix for the linked issue).

[timotheecour]

LGTM but https://github.com/nim-lang/Nim/pull/16367/files#r545569618 should be addressed now or after this PR

[Araq]

It seems better than before but if writing to stdout fails, there is little reason to assume we can report an exception in some other way. Maybe it would be wiser to quit 1 instead.

[timotheecour]

if writing to stdout fails, there is little reason to assume we can report an exception in some other way

there is, user code can handle this however they want for their requirements:

• with writing to stderr (stdout may fail doesn't mean stderr would)
• writing to a file
• doing cleanups before quitting

quit 1 should always be up to user code, not library code

[iffy]

It seems better than before but if writing to stdout fails, there is little reason to assume we can report an exception in some other way.

In my case (the reason I found this issue), I was able to write to stderr even when stdout writing failed. And after a sleep, I was also able to write to stdout again, too.

[timotheecour]

indeed, you got EAGAIN and that's what it's about:

35 EAGAIN Resource temporarily unavailable. This is a temporary condition and later calls to the same routine may complete normally.

there are other temporary failure error codes (EINTER etc) that io needs to be aware of

[timotheecour]

This was (wrongly IMO) reverted in #18460

[iffy]

Any discussion on why it was reverted? @narimiran (I'm just seeing this)

This change fixed a really difficult-to-debug problem I was having with some code.

[Araq]

It was reverted because the policy is "what used not to raise, cannot start to raise". Which is a pretty bad policy but we need consensus of how to deal with it.

[arnetheduck]

Any discussion on why it was reverted?

when you have a function like:

proc f() {.raises: [].} = echo "hello"

raising a new exception breaks that code at compile time - if instead you have:

proc f() =
  var handle = open_database()
  echo "hello"

  try:
    if record_missing(handle):
     raise (ref CatchableError)(msg: "")
  finally:
    close_database(handle)

try:
  f()
except:
 ...

you're breaking runtime code without any way to detect it at compile time, really - not only have you caused a leak - this code also changes the semantics of the whole application, because it introduces a new control flow - that's not acceptable for many applications.

you could argue that if you're using two ways to write to the same output that have different semantics (write and echo), the problem lies with the code that does that, and not with echo itself.

[iffy]

Would the addition of an echo2 be welcome instead? Or, could I reintroduce the change but instead of altering the default behavior, require something like -d:nimEchoCanRaise to enable it?

[ghost]

Since breaking changes are allowed in devel now for Nim 2.0, maybe this PR can be re-merged again? Or maybe we need an RFC to discuss if echo should raise on IO errors?

[Araq]

echo should raise, it should be annotated to raises: [IOError], maybe add a switch to bring back the old non-raising echo or maybe add a rawEcho which doesn't raise.