Emit compiler diagnostics #3867

krzyzanowskim · 2021-11-13T15:12:34Z

Check for error,warning,note and remark in a format compatible with clang and swiftc, see: https://github.com/apple/swift/blob/main/docs/Diagnostics.md

Motivation:

Emit compiler diagnostics, normally redirected to output, now consumable by libSwiftPM users. https://bugs.swift.org/browse/SR-15479

Check for error,warning,note and remark in a format compatible with clang and swiftc, see: https://github.com/apple/swift/blob/main/docs/Diagnostics.md

tomerd · 2021-11-15T22:27:01Z

@swift-ci please smoke test

neonichu · 2021-11-15T22:27:41Z

Could you please add a test for this?

tomerd · 2021-11-15T22:27:55Z

Sources/Build/BuildOperationBuildSystemDelegateHandler.swift

                    for match in regex.matchGroups(in: output) {
                        self.errorMessagesByTarget[parser.targetName] = (self.errorMessagesByTarget[parser.targetName] ?? []) + [match[0]]
+


remove deal line

tomerd · 2021-11-15T22:28:10Z

Sources/Build/BuildOperationBuildSystemDelegateHandler.swift

+                        }
+                    }
+
+                    let regex = try! RegEx(pattern: #"(error:[^\n]*)\n.*"#, options: .dotMatchesLineSeparators)


try! safe?

safe. not my line though, I just moved it

Agreed, with a literal string to a RegEx, try! is fine — it's unfortunate but if we get support for compiler-checked RegEx literals in Swift some day, it will go away.

tomerd · 2021-11-16T03:40:40Z

Sources/Build/BuildOperationBuildSystemDelegateHandler.swift

+                    do {
+                        let regex = try! RegEx(pattern: #"^(.*):(\d+):(\d+):\s*(error|warning|note|remark)\S?\s*(.*)$"#, options: .anchorsMatchLines)
+                        for match in regex.matchGroups(in: output) {
+                            if let filePath = try? AbsolutePath(validating: match[0]) {


should we guard for length of match? seems safer?

I'm not sure it's necessary in this case, since it comes from a RegEx created from a string literal. There will always be five matches unless there's a bug in matchGroups(in:). Perhaps we can assert that, but it doesn't seem as if there is any runtime condition here that could cause this to fail.

Hopefully at some point Swift will have support for regular expressions where the matches can be returned as tuples so the type system knows how many there are.

tomerd · 2021-11-16T03:41:34Z

Sources/SPMBuildCore/PluginInvocation.swift

+            desc += ":\(line)"
+        }
+
+        if let column = column {


if let column = self.column reads better

tomerd · 2021-11-16T03:41:51Z

Sources/SPMBuildCore/PluginInvocation.swift

    }

    public var description: String {
-        "\(self.file)\(self.line?.description.appending(" ") ?? "")"
+        var desc = self.file.description
+        if let line = line {


if let line = self.line reads better

tomerd · 2021-11-16T03:44:18Z

this is a nice improvement, thanks for putting the PR together @krzyzanowskim. we need a couple of tests, and one thing to confirm is this won't print errors/warnings multiple times now since I think we are printing the compiler output in L609?

cc @abertelrud

abertelrud · 2021-11-16T18:13:32Z

Sources/SPMBuildCore/PluginInvocation.swift

@@ -859,13 +859,24 @@ extension ObservabilityMetadata {
 public struct FileLocation: Equatable, CustomStringConvertible {
    public let file: AbsolutePath
    public let line: Int?
+    public let column: Int?


Eventually I think we will want line/column ranges here (as the .dia format supports) for that's for a later PR. Just a note about the direction I think we'll end up going here.

abertelrud · 2021-11-16T18:18:59Z

this is a nice improvement, thanks for putting the PR together @krzyzanowskim. we need a couple of tests, and one thing to confirm is this won't print errors/warnings multiple times now since I think we are printing the compiler output in L609?

cc @abertelrud

That's a really good point — I hadn't noticed that. It does look as if the message would be emitted twice. For other IDEs I've worked on the pattern is usually to try to recognize it as a diagnostics and if that fails to just print it.

Either way, a unit test would show for sure whether it's printed once or twice now.

krzyzanowskim · 2021-11-16T18:41:38Z

I think we are printing the compiler output in L609?

writing to output is parallel to diagnostics. emitting diagnostics is derived from the output as we parse the output. I use it as library and it's handy to have it in diagnostics and in the output stream. Does the SPM CLI print diagnostics too?

abertelrud · 2021-11-17T05:12:44Z

I think we are printing the compiler output in L609?

writing to output is parallel to diagnostics. emitting diagnostics is derived from the output as we parse the output. I use it as library and it's handy to have it in diagnostics and in the output stream. Does the SPM CLI print diagnostics too?

Yes, it looks like the SwiftToolObservability struct prints any emitted diagnostics (and I think all observability events emitted during the build reach up to that level).

krzyzanowskim · 2021-11-17T11:25:55Z

Yes, it looks like the SwiftToolObservability struct prints any emitted diagnostics (and I think all observability events emitted during the build reach up to that level).

An alternative is to either emit diagnostic or write to the output stream, but not both? while it would work for SwiftToolObservability, not so much for everyone else, am I right? If I'd like to get a build output and save it to eg. a file, I need to merge output stream and diagnostics into a single stream to get it all. Thoughts?

abertelrud · 2021-11-17T17:03:46Z

Yes, it looks like the SwiftToolObservability struct prints any emitted diagnostics (and I think all observability events emitted during the build reach up to that level).

An alternative is to either emit diagnostic or write to the output stream, but not both? while it would work for SwiftToolObservability, not so much for everyone else, am I right? If I'd like to get a build output and save it to eg. a file, I need to merge output stream and diagnostics into a single stream to get it all. Thoughts?

Right, that doesn't seem like a great solution.

Another option would be to use the new ability to associate metadata with diagnostics (this is a new feature of ObservabilityScope) to indicate that it was derived by parsing the output text. Then an observer who is printing text anyway can ignore those diagnostics, while an observer who isn't printing the text can emit them.

abertelrud · 2021-11-17T17:05:17Z

Yes, it looks like the SwiftToolObservability struct prints any emitted diagnostics (and I think all observability events emitted during the build reach up to that level).

An alternative is to either emit diagnostic or write to the output stream, but not both? while it would work for SwiftToolObservability, not so much for everyone else, am I right? If I'd like to get a build output and save it to eg. a file, I need to merge output stream and diagnostics into a single stream to get it all. Thoughts?

Right, that doesn't seem like a great solution.

Another option would be to use the new ability to associate metadata with diagnostics (this is a new feature of ObservabilityScope) to indicate that it was derived by parsing the output text. Then an observer who is printing text anyway can ignore those diagnostics, while an observer who isn't printing the text can emit them.

It would be clearer in this case if it were the observer that printed regular output text too — I guess in this case the SwiftTool observer will have to rely no line 609 to have already emitted the output, and just not print diagnostics that are tagged as having been parsed from output text.

tomerd · 2021-11-19T00:50:29Z

Yes, it looks like the SwiftToolObservability struct prints any emitted diagnostics (and I think all observability events emitted during the build reach up to that level).

An alternative is to either emit diagnostic or write to the output stream, but not both? while it would work for SwiftToolObservability, not so much for everyone else, am I right? If I'd like to get a build output and save it to eg. a file, I need to merge output stream and diagnostics into a single stream to get it all. Thoughts?

Right, that doesn't seem like a great solution.
Another option would be to use the new ability to associate metadata with diagnostics (this is a new feature of ObservabilityScope) to indicate that it was derived by parsing the output text. Then an observer who is printing text anyway can ignore those diagnostics, while an observer who isn't printing the text can emit them.

It would be clearer in this case if it were the observer that printed regular output text too — I guess in this case the SwiftTool observer will have to rely no line 609 to have already emitted the output, and just not print diagnostics that are tagged as having been parsed from output text.

lets take a step back here. @krzyzanowskim I assume in your use case are setting a BuildOperationBuildSystemDelegateHandler and it has an outputStream set? if so, where is that pointing?

krzyzanowskim · 2021-11-22T15:53:20Z

@tomerd I use outputStream as a "raw" build log, and in my case, it goes to the file or to the console, depends on what I need. I treat OutputByteStream as raw data without semantic meaning whatsoever, in contrary to Diagnostics that provide useful information.

tomerd · 2021-12-01T22:30:00Z

thanks for the additional info @krzyzanowskim, so in your case it sounds the console output is treated as optional and the diagnostics are the critical part.

@abertelrud @neonichu iiuc the delegate here was designed to pipe the build-system output to the provided stream, to give users information about the build status, etc. is there any important information expected to be included in this beyond what we consider diagnostics? iow, should the entire thing be parsed(*) into diagnostics instead of being printed to the stream as-is?

* with some failure mode protection in which we fall back to dumping it into the stream

abertelrud · 2021-12-02T17:09:13Z

Yes, it looks to me as if it would be better to just use a single sequence of delegate calls here (and no text stream). Each line of text could really be broken into an info or debug diagnostic if needed — I don't think a fallback is necessary. I don't know how this evolved but I suspect that it started as just a text stream and then some delegate callbacks were added on top to show the animated progress, but that the text stream wasn't removed.

I think the key would be to get rid of the parallel streams, either by making non-issues become info diagnostics or having separate didSeePlainText and didSeeDiagnostic calls (as in the recent plugin changes).

abertelrud · 2021-12-02T17:56:26Z

I guess I didn't really answer the key question there. Yes, I think the delegate calls about starting and ending certain steps are important because it affects what clients show in terms of status. We can also model those as debug or info diagnostics (perhaps carrying specific metadata) but it seems cleaner to keep a delegate for such build events.

abertelrud · 2021-12-02T18:33:33Z

So the other approach would then be to have a delegate call to handle the diagnostic in-order with the other calls, but that seems at odds with using ObservabilityScope.emit for diagnostics.

abertelrud · 2021-12-14T17:46:45Z

Having looked at this a bit more, my specific suggestion for this PR is to:

remove the separate outputStream from BuildOperation
instead send each line of output through the observabilityScope
- those that are recognized as errors or warnings have that severity
- the others get sent as info

That keeps all the output in order, and everything only gets reported once.

I think the only change compared with what you have here would be to send each line of non-matching output as info.

@krzyzanowskim is that something you'd be willing to take on as part of this?

krzyzanowskim · 2021-12-14T19:05:02Z

Having looked at this a bit more, my specific suggestion for this PR

thank you for that. I wasn't sure what's the output of the discussion here was.

@krzyzanowskim is that something you'd be willing to take on as part of this?

yes. I put it on my todo

abertelrud · 2021-12-14T20:00:41Z

Having looked at this a bit more, my specific suggestion for this PR

thank you for that. I wasn't sure what's the output of the discussion here was.

Yes, sorry about that — I was exploring ideas out loud, but of course that's not very helpful for the PR author.

@krzyzanowskim is that something you'd be willing to take on as part of this?

yes. I put it on my todo

Great, thanks! Please do ping me here if there are problems or ambiguities and I'll try to be more specific with any recommendations.

krzyzanowskim requested review from abertelrud, elsh, neonichu and tomerd as code owners November 13, 2021 15:12

Emit compiler diagnostics

4fbbeef

Check for error,warning,note and remark in a format compatible with clang and swiftc, see: https://github.com/apple/swift/blob/main/docs/Diagnostics.md

krzyzanowskim force-pushed the marcin/emit-output-diagnostics branch from 411c1c6 to 4fbbeef Compare November 13, 2021 15:20

tomerd reviewed Nov 15, 2021

View reviewed changes

krzyzanowskim added 2 commits November 16, 2021 00:52

Cleanup. Remove line.

ae95c58

Now! remove empty line

c8b776b

tomerd reviewed Nov 16, 2021

View reviewed changes

tomerd self-assigned this Nov 16, 2021

abertelrud reviewed Nov 16, 2021

View reviewed changes

neonichu added the next waiting for next merge window label Apr 11, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Emit compiler diagnostics #3867

Emit compiler diagnostics #3867

krzyzanowskim commented Nov 13, 2021

tomerd commented Nov 15, 2021

neonichu commented Nov 15, 2021

tomerd Nov 15, 2021

tomerd Nov 15, 2021

krzyzanowskim Nov 15, 2021

abertelrud Nov 16, 2021

tomerd Nov 16, 2021

abertelrud Nov 16, 2021

abertelrud Nov 16, 2021

tomerd Nov 16, 2021

tomerd Nov 16, 2021

tomerd commented Nov 16, 2021 •

edited

Loading

abertelrud Nov 16, 2021

abertelrud commented Nov 16, 2021

krzyzanowskim commented Nov 16, 2021

abertelrud commented Nov 17, 2021

krzyzanowskim commented Nov 17, 2021

abertelrud commented Nov 17, 2021

abertelrud commented Nov 17, 2021

tomerd commented Nov 19, 2021

krzyzanowskim commented Nov 22, 2021

tomerd commented Dec 1, 2021 •

edited

Loading

abertelrud commented Dec 2, 2021

abertelrud commented Dec 2, 2021

abertelrud commented Dec 2, 2021

abertelrud commented Dec 14, 2021

krzyzanowskim commented Dec 14, 2021

abertelrud commented Dec 14, 2021

		for match in regex.matchGroups(in: output) {
		self.errorMessagesByTarget[parser.targetName] = (self.errorMessagesByTarget[parser.targetName] ?? []) + [match[0]]

Emit compiler diagnostics #3867

Are you sure you want to change the base?

Emit compiler diagnostics #3867

Conversation

krzyzanowskim commented Nov 13, 2021

Motivation:

tomerd commented Nov 15, 2021

neonichu commented Nov 15, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tomerd commented Nov 16, 2021 • edited Loading

Choose a reason for hiding this comment

abertelrud commented Nov 16, 2021

krzyzanowskim commented Nov 16, 2021

abertelrud commented Nov 17, 2021

krzyzanowskim commented Nov 17, 2021

abertelrud commented Nov 17, 2021

abertelrud commented Nov 17, 2021

tomerd commented Nov 19, 2021

krzyzanowskim commented Nov 22, 2021

tomerd commented Dec 1, 2021 • edited Loading

abertelrud commented Dec 2, 2021

abertelrud commented Dec 2, 2021

abertelrud commented Dec 2, 2021

abertelrud commented Dec 14, 2021

krzyzanowskim commented Dec 14, 2021

abertelrud commented Dec 14, 2021

tomerd commented Nov 16, 2021 •

edited

Loading

tomerd commented Dec 1, 2021 •

edited

Loading