Skip to content

Implement support for automatically coloring output in DiagnosticsFormatter #1280

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 8 additions & 3 deletions Sources/SwiftDiagnostics/DiagnosticsFormatter.swift
Original file line number Diff line number Diff line change
Expand Up @@ -139,10 +139,11 @@ public struct DiagnosticsFormatter {
switch message.severity {
case .error:
let annotation = ANSIAnnotation(color: .red, trait: .bold)
return annotation.applied(to: message.message)
return annotation.applied(to: "error: \(message.message)")
case .warning:
let annotation = ANSIAnnotation(color: .yellow)
return annotation.applied(to: message.message)
let color = ANSIAnnotation(color: .yellow)
let prefix = color.withTrait(.bold).applied(to: "warning: ")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just to be clear: It’s intentional that errors have both the prefix and the the message in bold but warnings only have a bold prefix? I’m fine with either way but just want to make sure the current design is a conscious decision.

return prefix + color.applied(to: message.message)
case .note:
return message.message
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we also add a prefix for notes?

}
Expand Down Expand Up @@ -181,6 +182,10 @@ struct ANSIAnnotation {
self.trait = trait
}

func withTrait(_ trait: Trait) -> Self {
return ANSIAnnotation(color: self.color, trait: trait)
}

func applied(to message: String) -> String {
// Resetting after the message ensures that we don't color unintended lines in the output
return "\(code)\(message)\(ANSIAnnotation.normal.code)"
Expand Down
51 changes: 51 additions & 0 deletions Sources/swift-parser-cli/TerminalUtils.swift
Original file line number Diff line number Diff line change
@@ -0,0 +1,51 @@
//===----------------------------------------------------------------------===//
//
// This source file is part of the Swift.org open source project
//
// Copyright (c) 2014 - 2022 Apple Inc. and the Swift project authors
// Licensed under Apache License v2.0 with Runtime Library Exception
//
// See https://swift.org/LICENSE.txt for license information
// See https://swift.org/CONTRIBUTORS.txt for the list of Swift project authors
//
//===----------------------------------------------------------------------===//

#if canImport(Glibc)
import Glibc
#elseif os(Windows)
import CRT
#else
import Darwin.C
#endif

#if os(Android)
typealias FILEPointer = OpaquePointer
#else
typealias FILEPointer = UnsafeMutablePointer<FILE>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be named FilePointer to look a little more swift? It’s bad enough to have FILE scream at you in all caps.

#endif

enum TerminalHelper {
static var isConnectedToTerminal: Bool {
return isTTY(stderr)
}
Comment on lines +28 to +30
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since we are printing the errors to stdout (instead of stderr like swift-format), we should be checking whether stdout is connected to a terminal here. For example, stdout could be connected to a terminal while stderr is not if you do swift-parser-cli … 2> /tmp/errors.txt.

And, to be clear, we should probably name the method to mention that is checking stdout instead of stderr.

Suggested change
static var isConnectedToTerminal: Bool {
return isTTY(stderr)
}
static var isStdoutConnectedToTerminal: Bool {
return isTTY(stdout)
}


/// Checks if passed file pointer is a tty.
static func isTTY(_ filePointer: FILEPointer) -> Bool {
return terminalType(filePointer) == .tty
}

/// The type of terminal.
enum TerminalType {
/// The terminal is a TTY.
case tty

/// The terminal is a file stream.
case file
}

/// Computes the terminal type of the stream.
static func terminalType(_ filePointer: FILEPointer) -> TerminalType {
let isTTY = isatty(fileno(filePointer)) != 0
return isTTY ? .tty : .file
}
}
11 changes: 8 additions & 3 deletions Sources/swift-parser-cli/swift-parser-cli.swift
Original file line number Diff line number Diff line change
Expand Up @@ -198,17 +198,22 @@ class PrintDiags: ParsableCommand {
@Flag(name: .long, help: "Perform sequence folding with the standard operators")
var foldSequences: Bool = false

@Flag(name: .long, help: "Colorize output with ANSI color codes")
@Flag(name: .long, help: "Force output coloring with ANSI color codes")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You could make this a flag with inversion: .prefixedNo like in https://github.com/apple/swift-format/blob/072768f1af50989996e137919f9f6be71e8294e6/Sources/swift-format/Subcommands/LintFormatOptions.swift#L62-L69 so you can also force coloring off if you want to.


And now that I think about it, we should name the flag color-diagnostics (just rename the variable to colorDiagnostics) to match the name of the flag in the Swift Compiler.

Suggested change
@Flag(name: .long, help: "Force output coloring with ANSI color codes")
@Flag(name: .long, inversion: .prefixedLo, help: "Enable or disable coloring with ANSI color codes")
let colorDiagnostics: Bool?

var colorize: Bool = false

func run() throws {
let source = try getContentsOfSourceFile(at: sourceFile)

source.withUnsafeBufferPointer { sourceBuffer in
let tree = Parser.parse(source: sourceBuffer)

var diags = ParseDiagnosticsGenerator.diagnostics(for: tree)
print(DiagnosticsFormatter.annotatedSource(tree: tree, diags: diags, colorize: colorize))
let annotatedSource = DiagnosticsFormatter.annotatedSource(
tree: tree,
diags: diags,
colorize: colorize || TerminalHelper.isConnectedToTerminal
)

print(annotatedSource)

if foldSequences {
diags += foldAllSequences(tree).1
Expand Down
8 changes: 4 additions & 4 deletions Tests/SwiftDiagnosticsTest/DiagnosticsFormatterTests.swift
Original file line number Diff line number Diff line change
Expand Up @@ -101,7 +101,7 @@ final class DiagnosticsFormatterTests: XCTestCase {

let expectedOutput = """
1 │ var foo = bar +
∣ ╰─ \u{001B}[1;31mexpected expression in variable\u{001B}[0;0m
∣ ╰─ \u{001B}[1;31merror: expected expression in variable\u{001B}[0;0m

"""
AssertStringsEqualWithDiff(expectedOutput, annotate(source: source, colorize: true))
Expand All @@ -113,9 +113,9 @@ final class DiagnosticsFormatterTests: XCTestCase {
"""
let expectedOutput = """
1 │ foo.[].[].[]
∣ │ │ ╰─ \u{001B}[1;31mexpected name in member access\u{001B}[0;0m
∣ │ ╰─ \u{001B}[1;31mexpected name in member access\u{001B}[0;0m
∣ ╰─ \u{001B}[1;31mexpected name in member access\u{001B}[0;0m
∣ │ │ ╰─ \u{001B}[1;31merror: expected name in member access\u{001B}[0;0m
∣ │ ╰─ \u{001B}[1;31merror: expected name in member access\u{001B}[0;0m
∣ ╰─ \u{001B}[1;31merror: expected name in member access\u{001B}[0;0m

"""
AssertStringsEqualWithDiff(expectedOutput, annotate(source: source, colorize: true))
Expand Down