Driver diagnostics (such as remarks and errors) don't obey -parseable-output

[abertelrud]

Diagnostics emitted by the driver itself don't obey -parseable-output. This makes it a bit difficult for clients to handle the output and leads to things like swiftlang/swift-package-manager#5968.

Consider this command line:

❯ swiftc -parseable-output -whole-module-optimization -incremental -c foo.swift

(assuming there is a file foo.swift). It results in:

remark: Incremental compilation has been disabled: it is not compatible with whole module optimization
1201
{
  "command_arguments" : [
    "-frontend",
    "-c",
    "foo.swift",
    "-target",
    "arm64-apple-macosx12.0",
    "-Xllvm",
    "-aarch64-use-tbi",
    "-enable-objc-interop",
    "-stack-check",
. . .

which is a mixture of JSON output and plain-text output. Making matters worse, both the JSON output and the diagnostics are emitted to stderr. I would have expected that output would go to stdout when -parseable-output is passed, while the human-readable diagnostics go to stderr.

Alternatively, when -parseable-output is in effect, the diagnostics from the driver could be wrapped in JSON packets reflecting the type of diagnostic so that clients don't have to handle a mixture.

[artemcm]

This is not great.

Re: emitting parseable output to stderr, this is a behavior that SwiftDriver inherited from the legacy driver, and we need to be careful we don't break clients that rely on that... I agree that in principle it should go to stdout.

I'm not sure how we get interleaving in the output that swiftlang/swift-package-manager#5968 sees, since both the plain driver diagnostics, and parseable output, are emitted synchronously on the same dispatch queue:
https://github.com/apple/swift-driver/blob/main/Sources/SwiftDriver/Driver/Driver.swift#L413
https://github.com/apple/swift-driver/blob/main/Sources/SwiftDriver/Driver/ToolExecutionDelegate.swift#L169

I agree with the options we have, we could come up with a JSON format that will be a part of parseable-output for driver diagnostics only, or actually just change current parseable-output to go to stdout.

[abertelrud]

Thanks Artem. There are certainly multiple things going on here. The missing newline looks like a simple problem in SwiftPM's handling of unparseable JSON, so that can easily be fixed and I'm preparing a fix now.

But I do think that the SwiftDriver should probably emit its own diagnostics in JSON (and this is actually related, probably, to how SwiftDriver's diagnostics don't show up in the .dia file either, when that option is passed — I know the underlying implementation reasons for that, but from the outside it's still unfortunate).

[abertelrud]

As for stdout vs stderr, perhaps a new option is warranted — although I think it's probably better to emit them as JSON so that the order is preserved among the messages.

[abertelrud]

Also, I didn't figure out why the original problem had that unfortunate truncation, but figured we can deal with that separately from the newline. Perhaps there's some kind of undercounting in UTF-8 to String conversion going on — I haven't looked closer at what the output parsing in SwiftPM is actually doing, but messing up byte counts and character counts is a classic kind of bug.

[abertelrud]

@artemcm For swiftlang/swift-package-manager#5987, is there a good way to reliably generate a remark from the Swift Driver that I can use in my unit test in that PR? I tried with the incremental one and the whole module optimization, but it seems that in the self-hosted test I get the remark but in the smoke test I don't. Is that an unreliable remark to rely on?

[artemcm]

This remark is reliable but I believe smoke tests actually do not utilize the new swift-driver, relying on the legacy driver instead.

@neonichu investigated this in swiftlang/swift-package-manager#5885.

[abertelrud]

Ah! Thank you for clarifying! Maybe the best is to make the test conditional so that if the message is there then we make sure it's newline-terminated. If there's a better one that we can use, I could switch to that, but if the driver isn't even involved in the smoke tests, then there doesn't seem much we can do. And also seems like a problem that would go away over time, when swift-driver is used unconditionally.