Refactor "diagnostics" into "findings". #270
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This change addresses swiftlang/swift-syntax#309 and swiftlang/swift-syntax#325, which split `SwiftSyntaxParser` into its own module and then removed the `DiagnosticEngine` APIs (so only the `Diagnostic` type remains as the way diagnostics from the parser are sent back to the caller). Rather than remain coupled to swift-syntax's `Diagnostic` type, this change creates a similar `Finding` type (and related types) to describe the "lint findings" that tree-based rules, the pretty-printer, and the whitespace linter encounter during formatting/linting. This `Finding` type is the currency type for these kinds of findings/diagnostics emitted by the formatter API layer. The `swift-format` frontend tool now adopts `DiagnosticsEngine` from https://github.com/apple/swift-tools-support-core to manage and print these, but other clients using the API could use something different entirely. Since the usage of `TSC` is limited to the executable, API users don't have to take on a large and mostly unrelated dependency. There are still some opportunities for renaming here -- mainly replacing instances of `diagnose` in the internal code with `emitFinding` or something similar -- but I'll leave that as future cleanup.
|
Tested against cc @dylansturg |
dylansturg
approved these changes
Nov 9, 2021
|
|
||
| /// Prints a diagnostic to standard error. | ||
| func printDiagnosticToStderr(_ diagnostic: TSCBasic.Diagnostic) { | ||
| printQueue.sync { |
There was a problem hiding this comment.
I wonder if there would be any appreciable performance difference using async here, especially in parallel mode. In the end, we would need to flush the print queue though (so all diagnostics get printed before exiting), which I guess would have to be handled in the Linter and Format frontends. Might not be worth the added complexity.
There was a problem hiding this comment.
It's certainly worth trying out as follow-up work to see if there's a meaningful difference.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This change addresses swiftlang/swift-syntax#309 and swiftlang/swift-syntax#325, which split
SwiftSyntaxParserinto its own module and then removed theDiagnosticEngineAPIs (so only theDiagnostictype remains as the way diagnostics from the parser are sent back to the caller).Rather than remain coupled to swift-syntax's
Diagnostictype, this change creates a similarFindingtype (and related types) to describe the "lint findings" that tree-based rules, the pretty-printer, and the whitespace linter encounter duringformatting/linting.
This
Findingtype is the currency type for these kinds of findings/diagnostics emitted by the formatter API layer. Theswift-formatfrontend tool now adoptsDiagnosticsEnginefrom https://github.com/apple/swift-tools-support-core to manage and print these, but other clients using the API could use something different entirely. Since the usage ofTSCis limited to the executable, API users don't have to take on a large and mostly unrelated dependency.There are still some opportunities for renaming here -- mainly replacing instances of
diagnosein the internal code withemitFindingor something similar -- but I'll leave that as future cleanup.