Merge pull request #242 from cashapp/bradfol/comment-error

Fix error messages for comment commands
cashapp · Feb 6, 2025 · cf86daa · cf86daa
2 parents 0d8a1b5 + bdf93fc
commit cf86daa
Show file tree

Hide file tree

Showing 4 changed files with 95 additions and 7 deletions.
diff --git a/Sources/KnitCodeGen/AssemblyParsing.swift b/Sources/KnitCodeGen/AssemblyParsing.swift
@@ -343,6 +343,10 @@ enum ReplacesParsingError: LocalizedError, SyntaxError {
             return syntax
         }
     }
+
+    var positionAboveNode: Bool {
+        return false
+    }
 }
 
 // Output any errors that occurred during parsing
@@ -354,11 +358,8 @@ func printErrors(_ errors: [Error], filePath: String, syntaxTree: SyntaxProtocol
 
     for error in errors {
         if let syntaxError = error as? SyntaxError {
-            let position = syntaxError.syntax.startLocation(converter: lineConverter, afterLeadingTrivia: true)
-            let line = position.line
-            let column = position.column
             FileHandle.standardError.write(Data(
-                "\(filePath):\(line):\(column): error: \(error.localizedDescription)\n".utf8
+                syntaxError.standardErrorDescription(lineConverter: lineConverter).utf8
             ))
         } else {
             FileHandle.standardError.write(Data(
@@ -368,6 +369,22 @@ func printErrors(_ errors: [Error], filePath: String, syntaxTree: SyntaxProtocol
     }
 }
 
+extension SyntaxError {
+
+    func standardErrorDescription(lineConverter: SourceLocationConverter) -> String {
+        let position = self.syntax.startLocation(
+            converter: lineConverter,
+            afterLeadingTrivia: true
+        )
+        let filePath = position.file
+        let line = positionAboveNode ? position.line - 1 : position.line
+        let column = position.column
+
+        return "\(filePath):\(line):\(column): error: \(self.localizedDescription)\n"
+    }
+
+}
+
 // MARK: - IfConfigClauseSyntax
 
 /// Visitor that is able to wrap children inside an #if block

diff --git a/Sources/KnitCodeGen/FunctionCallRegistrationParsing.swift b/Sources/KnitCodeGen/FunctionCallRegistrationParsing.swift
@@ -113,7 +113,7 @@ extension FunctionCallExprSyntax {
                 registrationArguments: registrationArguments,
                 leadingTrivia: leadingTrivia,
                 functionName: .implements,
-                syntax: implementsCalledMethod.arguments
+                syntax: implementsCalledMethod.calledExpression.period
             ) {
                 if forwardedRegistration.hasRedundantGetter {
                     throw RegistrationParsingError.redundantGetter(
@@ -400,4 +400,15 @@ enum RegistrationParsingError: LocalizedError, SyntaxError {
         }
     }
 
+    var positionAboveNode: Bool {
+        switch self {
+        case .redundantGetter,
+                .redundantAccessControl:
+            // These cases are errors regarding the comment command in the trivia above the syntax
+            return true
+        default:
+            return false
+        }
+    }
+
 }
diff --git a/Sources/KnitCodeGen/SyntaxError.swift b/Sources/KnitCodeGen/SyntaxError.swift
@@ -6,6 +6,9 @@ import Foundation
 import SwiftSyntax
 
 // An error that is related to a piece of syntax
-protocol SyntaxError {
+protocol SyntaxError: Error {
     var syntax: SyntaxProtocol { get }
+
+    /// Report the error position on the line above the syntax node.
+    var positionAboveNode: Bool { get }
 }
diff --git a/Tests/KnitCodeGenTests/AssemblyParsingTests.swift b/Tests/KnitCodeGenTests/AssemblyParsingTests.swift
@@ -892,7 +892,14 @@ final class AssemblyParsingTests: XCTestCase {
         _ = try assertParsesSyntaxTree(sourceFile, assertErrorsToPrint: { errors in
             XCTAssertEqual(errors.count, 1)
             if case RegistrationParsingError.redundantAccessControl = errors[0] {
-                // Correct
+                // Correct. Check the printed standard error
+                assertStandardErrorMessage(
+                    sourceFile: sourceFile,
+                    error: errors.first,
+                    expectedLineNumber: 6,
+                    expectedColumnNumber: 13,
+                    expectedMessage: "Access control matches the default and can be removed"
+                )
             } else {
                 XCTFail("Incorrect error case")
             }
@@ -921,6 +928,37 @@ final class AssemblyParsingTests: XCTestCase {
         })
     }
 
+    func testRedundantAccessControlImplements() throws {
+        let sourceFile: SourceFileSyntax = """
+            // @knit public
+            class MyAssembly: ModuleAssembly {
+                typealias TargetResolver = TestResolver
+                
+                func assemble(container: Container) {
+                    container.register(PublicType.self) { _ in PublicType() }
+                        // @knit public
+                        .implements(OtherType.self)
+                }
+            }
+        """
+
+        _ = try assertParsesSyntaxTree(sourceFile, assertErrorsToPrint: { errors in
+            XCTAssertEqual(errors.count, 1)
+            if case RegistrationParsingError.redundantAccessControl = errors[0] {
+                // Correct. Check the printed standard error
+                assertStandardErrorMessage(
+                    sourceFile: sourceFile,
+                    error: errors.first,
+                    expectedLineNumber: 7,
+                    expectedColumnNumber: 17,
+                    expectedMessage: "Access control matches the default and can be removed"
+                )
+            } else {
+                XCTFail("Incorrect error case")
+            }
+        })
+    }
+
     func testRedundantForwardedGetterName() throws {
         let sourceFile: SourceFileSyntax = """
             class TestAssembly: ModuleAssembly {
@@ -970,3 +1008,22 @@ private func assertParsesSyntaxTree(
 
     return try XCTUnwrap(configuration.first)
 }
+
+private func assertStandardErrorMessage(
+    sourceFile: SourceFileSyntax,
+    error: Error?,
+    expectedLineNumber: Int,
+    expectedColumnNumber: Int,
+    expectedMessage: String,
+    file: StaticString = #filePath,
+    line: UInt = #line
+) {
+    let locationConverter = SourceLocationConverter(fileName: "TestFile.swift", tree: sourceFile)
+    let standardError = (error as? SyntaxError)?.standardErrorDescription(lineConverter: locationConverter)
+    XCTAssertEqual(
+        standardError,
+        "TestFile.swift:\(expectedLineNumber):\(expectedColumnNumber): error: \(expectedMessage)\n",
+        file: file,
+        line: line
+    )
+}