Fix base/System.IO.withFile to only annotate own exceptions

[ch1bo]

Why

withFile annotating any exception raised within the wrapped action is misleading when debugging. It makes users think that something is wrong with the file opened, although another IOException was raised. This is particularly confusing if "not found" errors of other files are raised, e.g. trying to start a process which is not found on the PATH.

What

withFile should not annotate / change unrelated exceptions.

For example, given the file test.hs:

import System.IO
main = withFile "test.txt" WriteMode $ \_ -> fail "test

when run with runhaskell test.hs, produces

test.hs: test.txt: withFile: user error (test)

while it should rather exit and print:

test.hs: user error (test)

How

• Upstream issue opened by @ndmitchell https://gitlab.haskell.org/ghc/ghc/-/issues/20886
• A merge request fixing this https://gitlab.haskell.org/ghc/ghc/-/merge_requests/11866

[hasufell]

Since no API is changed, I guess this doesn't need an impact assessment? Although I expect that some tests examining error messages could break. I'm not sure we require running tests on clc-stackage (and afair that's problematic anyway).

[hasufell]

I backported the change to 9.6, so if anyone wants to try to run some selective tests:

ghcup install ghc -u "https://gitlab.haskell.org/api/v4/projects/933/jobs/1746054/artifacts/ghc-x86_64-linux-fedora33-release.tar.xz" clc237

[Bodigrim]

Thanks @ch1bo! Could you please copy the proposal here and update the top post? It's easier to discuss things when it's in the same place.

[ch1bo]

@Bodigrim Done and also updated the upstream MR based on your comments.

[hasufell]

What is the behavior of withFile fp WriteMode hGetContents?

I think it will now show filedescriptor information in ioe_filename: hasufell/file-io#18 (comment)

Is that less or more confusing?

[hasufell]

$ ghci-clc237
GHCi, version 9.6.2.20240106: https://www.haskell.org/ghc/  :? for help
ghci> import System.IO
ghci> withFile "test" WriteMode hGetContents
*** Exception: test: hGetContents: illegal operation (handle is not open for reading)

It seems the base version of withFile is not affected since it doesn't use fdToHandle as described here.

[Bodigrim]

This looks simple enough and impactful enough to proceed with a vote sooner than usual.

Dear CLC members, let's vote on the proposal to amend withFile and withFileBlocking so that they annotate only their own exceptions. Current behaviour is confusing and prone to mask the actual cause of the exception or even overwrite the relevant filename, which makes debugging unnecessary painful. The original issue is available at https://gitlab.haskell.org/ghc/ghc/-/issues/20886 and the MR is at https://gitlab.haskell.org/ghc/ghc/-/issues/20886. This is not a breaking change, just a bug fix for incorrect behaviour.

@hasufell @mixphix @angerman @parsonsmatt @tomjaguarpaw @velveteer

---

+1 from me. This dovetails nicely into our recent efforts to improve UX of IO exceptions.

[hasufell]

+1

[velveteer]

+1

[tomjaguarpaw]

+1

---

This seems like an improvement because the old message was rather confusing, that is, it's more confusing to see

test.hs: test.txt: withFile: user error (test)

than

test.hs: user error (test)

because the connection to the withFile block is not explained. On the other hand the former contains more information. I wonder if it would be better than both to have something like

test.hs: Whilst in the withFile block for test.txt: user error (test)

I'd would be in support of that, if anyone wanted to make a follow-up proposal.

[hasufell]

@tomjaguarpaw IO actions that use the file-handle inside the withFile block and fail should already print the proper filepath the handle is related to. I'm not sure it's relevant how the handle was obtained.

For IO actions unrelated to the file handle, I don't think we need any more information.

[tomjaguarpaw]

Possibly. My suggestion would be to "piggyback" on the withFile block as a source of information about the origin of the error. Perhaps that's too "cute" or perhaps it's helpful. I tend to think the latter, but there are good arguments for the former.

[tomjaguarpaw]

I'm also confused why this change isn't also made for withBinaryFile (and to confuse the matter further, I think withBinaryFiles Haddock is wrong: https://gitlab.haskell.org/ghc/ghc/-/issues/24350).

[hasufell]

I'm also confused why this change isn't also made for withBinaryFile (and to confuse the matter further, I think withBinaryFiles Haddock is wrong: https://gitlab.haskell.org/ghc/ghc/-/issues/24350).

That seems to be an oversight. Please comment on the MR.

[Bodigrim]

This seems like an improvement because the old message was rather confusing

Not only confusing, but also misleading.

main = withFile "test.txt" WriteMode $ \h -> do 
  cnt <- readFile "foo.txt"
  hPut h cnt

There could have been an exception in readFile "foo.txt", but its provenance will be obscured by withFile overwriting ioe_filename.

[Bodigrim]

I wonder if it would be better than both to have something like

test.hs: Whilst in the withFile block for test.txt: user error (test)

I'm not sure of potential consequences of accumulating error messages in user land (could they get too long? could their concatenation trigger more exceptions?). Backtraces for exceptions look like a more robust solution.

[tomjaguarpaw]

Fair enough. I don't feel strongly. Yes, my suggestion is a hacky replication of some of the work of backtraces. Maybe best to ignore it.

[parsonsmatt]

I'd feel a lot better about this if we had the backtraces already. Exception provenance information is already hard enough to come by (unless you're using annotated-exception), and removing some now before we have a better system in place feels bad.

So, weak -1 since we're in a pre-backtraces world.

In a backtraces world, I'd like to additionally see a HasCallStack constraint here so we can put the source location of the offending withFile call, and the filepath attached as an annotation to the exception thrown. So simply removing the annotation from other exceptions would also be a weak -1 since we have a much better possible solution ahead of us.

[hasufell]

@parsonsmatt what information does withFile in the error message give you when the encapsulated error has nothing to do with the file Handle? If an operation using the file handle fails, it'll show the filepath regardless. And as demonstrated above, the current code can actually show misleading error messages.

[ch1bo]

@tomjaguarpaw @hasufell I have also fixed withBinaryFile now and all squashed into this single commit: https://gitlab.haskell.org/ghc/ghc/-/merge_requests/11866/diffs?commit_id=910bb38821599ab070ea026c60a65bca9107299c

[Bodigrim]

Thanks @ch1bo. Since the MR was updated, strictly speaking, we have to vote anew, but I do not really expect people to revoke their votes since you just fixed an inadverent omission of withBinaryFile. Moreover, the discussion above suggests that me, @hasufell and @tomjaguarpaw are still in favor. @velveteer unless you revoke your voice explicitly, I'll assume that you are still in favor; it would be helpful if you re-confirm that you are.

@angerman @mixphix this is a gentle reminder to vote.

[mixphix]

+1

[velveteer]

Still +1

[Bodigrim]

Thanks all, that's enough votes to approve.