From 3de0e1351affdabb8cf9949abd85c154ca19d8fc Mon Sep 17 00:00:00 2001 From: Vera Mitchell Date: Wed, 7 May 2025 14:22:57 -0600 Subject: [PATCH 1/5] use found disambiguation to calculate diagnostic range if present rdar://150207195 --- .../Link Resolution/PathHierarchy+Error.swift | 20 +++++-- .../Semantics/ReferenceResolver.swift | 6 +++ .../DocumentationContextTests.swift | 53 ++++++++++++++++++- 3 files changed, 75 insertions(+), 4 deletions(-) diff --git a/Sources/SwiftDocC/Infrastructure/Link Resolution/PathHierarchy+Error.swift b/Sources/SwiftDocC/Infrastructure/Link Resolution/PathHierarchy+Error.swift index 9f85fc1a2c..67851a2126 100644 --- a/Sources/SwiftDocC/Infrastructure/Link Resolution/PathHierarchy+Error.swift +++ b/Sources/SwiftDocC/Infrastructure/Link Resolution/PathHierarchy+Error.swift @@ -223,13 +223,27 @@ extension PathHierarchy.Error { case .lookupCollision(partialResult: let partialResult, remaining: let remaining, collisions: let collisions): let nextPathComponent = remaining.first! - let (pathPrefix, _, solutions) = makeCollisionSolutions(from: collisions, nextPathComponent: nextPathComponent, partialResultPrefix: partialResult.pathPrefix) - + let (pathPrefix, foundDisambiguation, solutions) = makeCollisionSolutions( + from: collisions, + nextPathComponent: nextPathComponent, + partialResultPrefix: partialResult.pathPrefix) + + let rangeAdjustment: SourceRange + if !foundDisambiguation.isEmpty { + rangeAdjustment = .makeRelativeRange( + startColumn: pathPrefix.count - foundDisambiguation.count, + length: foundDisambiguation.count) + } else { + rangeAdjustment = .makeRelativeRange( + startColumn: pathPrefix.count - nextPathComponent.full.count, + length: nextPathComponent.full.count) + } + return TopicReferenceResolutionErrorInfo(""" \(nextPathComponent.full.singleQuoted) is ambiguous at \(partialResult.node.pathWithoutDisambiguation().singleQuoted) """, solutions: solutions, - rangeAdjustment: .makeRelativeRange(startColumn: pathPrefix.count - nextPathComponent.full.count, length: nextPathComponent.full.count) + rangeAdjustment: rangeAdjustment ) } } diff --git a/Sources/SwiftDocC/Semantics/ReferenceResolver.swift b/Sources/SwiftDocC/Semantics/ReferenceResolver.swift index d3498fb041..6d646962d8 100644 --- a/Sources/SwiftDocC/Semantics/ReferenceResolver.swift +++ b/Sources/SwiftDocC/Semantics/ReferenceResolver.swift @@ -44,6 +44,12 @@ func unresolvedReferenceProblem(source: URL?, range: SourceRange?, severity: Dia let diagnosticRange: SourceRange? if var rangeAdjustment = errorInfo.rangeAdjustment, let referenceSourceRange { rangeAdjustment.offsetWithRange(referenceSourceRange) + assert(rangeAdjustment.lowerBound.column >= 0 && rangeAdjustment.upperBound.column >= 0, """ + Unresolved topic reference range adjustment created range with negative column. + Source: \(source?.absoluteString ?? "nil") + Range: \(rangeAdjustment.lowerBound.description):\(rangeAdjustment.upperBound.description) + Summary: \(errorInfo.message) + """) diagnosticRange = rangeAdjustment } else { diagnosticRange = referenceSourceRange diff --git a/Tests/SwiftDocCTests/Infrastructure/DocumentationContext/DocumentationContextTests.swift b/Tests/SwiftDocCTests/Infrastructure/DocumentationContext/DocumentationContextTests.swift index 148b37fa91..93a7b31c9e 100644 --- a/Tests/SwiftDocCTests/Infrastructure/DocumentationContext/DocumentationContextTests.swift +++ b/Tests/SwiftDocCTests/Infrastructure/DocumentationContext/DocumentationContextTests.swift @@ -5266,7 +5266,58 @@ let expected = """ } } } - + + func testContextDiagnosesInsufficientDisambiguationWithCorrectRange() throws { + // This test deliberately does not turn on the overloads feature + // to ensure the symbol link below does not accidentally resolve correctly. + let symbolKindID = SymbolGraph.Symbol.KindIdentifier.allCases.first { !$0.isOverloadableKind }! + // Generate a 4 symbols with the same name for every non overloadable symbol kind + let symbols: [SymbolGraph.Symbol] = [ + makeSymbol(id: "first-\(symbolKindID.identifier)-id", kind: symbolKindID, pathComponents: ["SymbolName"]), + makeSymbol(id: "second-\(symbolKindID.identifier)-id", kind: symbolKindID, pathComponents: ["SymbolName"]), + makeSymbol(id: "third-\(symbolKindID.identifier)-id", kind: symbolKindID, pathComponents: ["SymbolName"]), + makeSymbol(id: "fourth-\(symbolKindID.identifier)-id", kind: symbolKindID, pathComponents: ["SymbolName"]), + ] + + let catalog = + Folder(name: "unit-test.docc", content: [ + JSONFile(name: "ModuleName.symbols.json", content: makeSymbolGraph( + moduleName: "ModuleName", + symbols: symbols + )), + + TextFile(name: "ModuleName.md", utf8Content: """ + # ``ModuleName`` + + This is a test file for ModuleName. + + ## Topics + + - ``SymbolName-\(symbolKindID.identifier)`` + """) + ]) + + let (_, context) = try loadBundle(catalog: catalog) + + let problems = context.problems.sorted(by: \.diagnostic.summary) + XCTAssertEqual(problems.count, 1) + + let problem = try XCTUnwrap(problems.first) + + XCTAssertEqual(problem.diagnostic.summary, "'SymbolName-\(symbolKindID.identifier)' is ambiguous at '/ModuleName'") + + for solution in problem.possibleSolutions { + XCTAssertEqual(solution.replacements.count, 1) + let replacement = try XCTUnwrap(solution.replacements.first) + + XCTAssertEqual(replacement.range.lowerBound, .init(line: 7, column: 15, source: nil)) + XCTAssertEqual( + replacement.range.upperBound, + .init(line: 7, column: 16 + symbolKindID.identifier.count, source: nil) + ) + } + } + func testResolveExternalLinkFromTechnologyRoot() throws { enableFeatureFlag(\.isExperimentalLinkHierarchySerializationEnabled) From 117350f141283e2f3e6653aa8be3fac47319b3fb Mon Sep 17 00:00:00 2001 From: Vera Mitchell Date: Thu, 8 May 2025 13:17:58 -0600 Subject: [PATCH 2/5] review: use all the non-overloadable symbol kinds --- .../DocumentationContextTests.swift | 69 ++++++++++--------- 1 file changed, 35 insertions(+), 34 deletions(-) diff --git a/Tests/SwiftDocCTests/Infrastructure/DocumentationContext/DocumentationContextTests.swift b/Tests/SwiftDocCTests/Infrastructure/DocumentationContext/DocumentationContextTests.swift index 93a7b31c9e..39953e0419 100644 --- a/Tests/SwiftDocCTests/Infrastructure/DocumentationContext/DocumentationContextTests.swift +++ b/Tests/SwiftDocCTests/Infrastructure/DocumentationContext/DocumentationContextTests.swift @@ -5270,51 +5270,52 @@ let expected = """ func testContextDiagnosesInsufficientDisambiguationWithCorrectRange() throws { // This test deliberately does not turn on the overloads feature // to ensure the symbol link below does not accidentally resolve correctly. - let symbolKindID = SymbolGraph.Symbol.KindIdentifier.allCases.first { !$0.isOverloadableKind }! - // Generate a 4 symbols with the same name for every non overloadable symbol kind - let symbols: [SymbolGraph.Symbol] = [ - makeSymbol(id: "first-\(symbolKindID.identifier)-id", kind: symbolKindID, pathComponents: ["SymbolName"]), - makeSymbol(id: "second-\(symbolKindID.identifier)-id", kind: symbolKindID, pathComponents: ["SymbolName"]), - makeSymbol(id: "third-\(symbolKindID.identifier)-id", kind: symbolKindID, pathComponents: ["SymbolName"]), - makeSymbol(id: "fourth-\(symbolKindID.identifier)-id", kind: symbolKindID, pathComponents: ["SymbolName"]), - ] + for symbolKindID in SymbolGraph.Symbol.KindIdentifier.allCases where !symbolKindID.isOverloadableKind { + // Generate a 4 symbols with the same name for every non overloadable symbol kind + let symbols: [SymbolGraph.Symbol] = [ + makeSymbol(id: "first-\(symbolKindID.identifier)-id", kind: symbolKindID, pathComponents: ["SymbolName"]), + makeSymbol(id: "second-\(symbolKindID.identifier)-id", kind: symbolKindID, pathComponents: ["SymbolName"]), + makeSymbol(id: "third-\(symbolKindID.identifier)-id", kind: symbolKindID, pathComponents: ["SymbolName"]), + makeSymbol(id: "fourth-\(symbolKindID.identifier)-id", kind: symbolKindID, pathComponents: ["SymbolName"]), + ] - let catalog = - Folder(name: "unit-test.docc", content: [ - JSONFile(name: "ModuleName.symbols.json", content: makeSymbolGraph( - moduleName: "ModuleName", - symbols: symbols - )), + let catalog = + Folder(name: "unit-test.docc", content: [ + JSONFile(name: "ModuleName.symbols.json", content: makeSymbolGraph( + moduleName: "ModuleName", + symbols: symbols + )), - TextFile(name: "ModuleName.md", utf8Content: """ - # ``ModuleName`` + TextFile(name: "ModuleName.md", utf8Content: """ + # ``ModuleName`` - This is a test file for ModuleName. + This is a test file for ModuleName. - ## Topics + ## Topics - - ``SymbolName-\(symbolKindID.identifier)`` - """) - ]) + - ``SymbolName-\(symbolKindID.identifier)`` + """) + ]) - let (_, context) = try loadBundle(catalog: catalog) + let (_, context) = try loadBundle(catalog: catalog) - let problems = context.problems.sorted(by: \.diagnostic.summary) - XCTAssertEqual(problems.count, 1) + let problems = context.problems.sorted(by: \.diagnostic.summary) + XCTAssertEqual(problems.count, 1) - let problem = try XCTUnwrap(problems.first) + let problem = try XCTUnwrap(problems.first) - XCTAssertEqual(problem.diagnostic.summary, "'SymbolName-\(symbolKindID.identifier)' is ambiguous at '/ModuleName'") + XCTAssertEqual(problem.diagnostic.summary, "'SymbolName-\(symbolKindID.identifier)' is ambiguous at '/ModuleName'") - for solution in problem.possibleSolutions { - XCTAssertEqual(solution.replacements.count, 1) - let replacement = try XCTUnwrap(solution.replacements.first) + for solution in problem.possibleSolutions { + XCTAssertEqual(solution.replacements.count, 1) + let replacement = try XCTUnwrap(solution.replacements.first) - XCTAssertEqual(replacement.range.lowerBound, .init(line: 7, column: 15, source: nil)) - XCTAssertEqual( - replacement.range.upperBound, - .init(line: 7, column: 16 + symbolKindID.identifier.count, source: nil) - ) + XCTAssertEqual(replacement.range.lowerBound, .init(line: 7, column: 15, source: nil)) + XCTAssertEqual( + replacement.range.upperBound, + .init(line: 7, column: 16 + symbolKindID.identifier.count, source: nil) + ) + } } } From ba5fff23f24373bcd308cc0426aab7a5557df5b3 Mon Sep 17 00:00:00 2001 From: Vera Mitchell Date: Thu, 8 May 2025 13:18:45 -0600 Subject: [PATCH 3/5] review: assert that solutions exist before iterating on them --- .../DocumentationContext/DocumentationContextTests.swift | 2 ++ 1 file changed, 2 insertions(+) diff --git a/Tests/SwiftDocCTests/Infrastructure/DocumentationContext/DocumentationContextTests.swift b/Tests/SwiftDocCTests/Infrastructure/DocumentationContext/DocumentationContextTests.swift index 39953e0419..15842d77d6 100644 --- a/Tests/SwiftDocCTests/Infrastructure/DocumentationContext/DocumentationContextTests.swift +++ b/Tests/SwiftDocCTests/Infrastructure/DocumentationContext/DocumentationContextTests.swift @@ -5306,6 +5306,8 @@ let expected = """ XCTAssertEqual(problem.diagnostic.summary, "'SymbolName-\(symbolKindID.identifier)' is ambiguous at '/ModuleName'") + XCTAssertEqual(problem.possibleSolutions.count, 4) + for solution in problem.possibleSolutions { XCTAssertEqual(solution.replacements.count, 1) let replacement = try XCTUnwrap(solution.replacements.first) From 1176cef7db039c8471efa363209f7f5877f266eb Mon Sep 17 00:00:00 2001 From: Vera Mitchell Date: Thu, 8 May 2025 13:19:43 -0600 Subject: [PATCH 4/5] review: remove redundant assertion condition --- Sources/SwiftDocC/Semantics/ReferenceResolver.swift | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Sources/SwiftDocC/Semantics/ReferenceResolver.swift b/Sources/SwiftDocC/Semantics/ReferenceResolver.swift index 6d646962d8..9703463ad0 100644 --- a/Sources/SwiftDocC/Semantics/ReferenceResolver.swift +++ b/Sources/SwiftDocC/Semantics/ReferenceResolver.swift @@ -44,7 +44,7 @@ func unresolvedReferenceProblem(source: URL?, range: SourceRange?, severity: Dia let diagnosticRange: SourceRange? if var rangeAdjustment = errorInfo.rangeAdjustment, let referenceSourceRange { rangeAdjustment.offsetWithRange(referenceSourceRange) - assert(rangeAdjustment.lowerBound.column >= 0 && rangeAdjustment.upperBound.column >= 0, """ + assert(rangeAdjustment.lowerBound.column >= 0, """ Unresolved topic reference range adjustment created range with negative column. Source: \(source?.absoluteString ?? "nil") Range: \(rangeAdjustment.lowerBound.description):\(rangeAdjustment.upperBound.description) From a7b2a3729e3484b75d95df809500d5b5266776d6 Mon Sep 17 00:00:00 2001 From: Vera Mitchell Date: Thu, 8 May 2025 17:05:51 -0600 Subject: [PATCH 5/5] review: add tests for wrong suffix and wrong name --- .../DocumentationContextTests.swift | 104 ++++++++++++++++++ 1 file changed, 104 insertions(+) diff --git a/Tests/SwiftDocCTests/Infrastructure/DocumentationContext/DocumentationContextTests.swift b/Tests/SwiftDocCTests/Infrastructure/DocumentationContext/DocumentationContextTests.swift index 15842d77d6..b9e67fcb4c 100644 --- a/Tests/SwiftDocCTests/Infrastructure/DocumentationContext/DocumentationContextTests.swift +++ b/Tests/SwiftDocCTests/Infrastructure/DocumentationContext/DocumentationContextTests.swift @@ -5321,6 +5321,110 @@ let expected = """ } } + func testContextDiagnosesIncorrectDisambiguationWithCorrectRange() throws { + // This test deliberately does not turn on the overloads feature + // to ensure the symbol link below does not accidentally resolve correctly. + for symbolKindID in SymbolGraph.Symbol.KindIdentifier.allCases where !symbolKindID.isOverloadableKind { + // Generate a 4 symbols with the same name for every non overloadable symbol kind + let symbols: [SymbolGraph.Symbol] = [ + makeSymbol(id: "first-\(symbolKindID.identifier)-id", kind: symbolKindID, pathComponents: ["SymbolName"]), + makeSymbol(id: "second-\(symbolKindID.identifier)-id", kind: symbolKindID, pathComponents: ["SymbolName"]), + makeSymbol(id: "third-\(symbolKindID.identifier)-id", kind: symbolKindID, pathComponents: ["SymbolName"]), + makeSymbol(id: "fourth-\(symbolKindID.identifier)-id", kind: symbolKindID, pathComponents: ["SymbolName"]), + ] + + let catalog = + Folder(name: "unit-test.docc", content: [ + JSONFile(name: "ModuleName.symbols.json", content: makeSymbolGraph( + moduleName: "ModuleName", + symbols: symbols + )), + + TextFile(name: "ModuleName.md", utf8Content: """ + # ``ModuleName`` + + This is a test file for ModuleName. + + ## Topics + + - ``SymbolName-abc123`` + """) + ]) + + let (_, context) = try loadBundle(catalog: catalog) + + let problems = context.problems.sorted(by: \.diagnostic.summary) + XCTAssertEqual(problems.count, 1) + + let problem = try XCTUnwrap(problems.first) + + XCTAssertEqual(problem.diagnostic.summary, "'abc123' isn't a disambiguation for 'SymbolName' at '/ModuleName'") + + XCTAssertEqual(problem.possibleSolutions.count, 4) + + for solution in problem.possibleSolutions { + XCTAssertEqual(solution.replacements.count, 1) + let replacement = try XCTUnwrap(solution.replacements.first) + + // "Replace '-abc123' with '-(hash)'" where 'abc123' is at 7:15-7:22 + XCTAssertEqual(replacement.range.lowerBound, .init(line: 7, column: 15, source: nil)) + XCTAssertEqual(replacement.range.upperBound, .init(line: 7, column: 22, source: nil)) + } + } + } + + func testContextDiagnosesIncorrectSymbolNameWithCorrectRange() throws { + // This test deliberately does not turn on the overloads feature + // to ensure the symbol link below does not accidentally resolve correctly. + for symbolKindID in SymbolGraph.Symbol.KindIdentifier.allCases where !symbolKindID.isOverloadableKind { + // Generate a 4 symbols with the same name for every non overloadable symbol kind + let symbols: [SymbolGraph.Symbol] = [ + makeSymbol(id: "first-\(symbolKindID.identifier)-id", kind: symbolKindID, pathComponents: ["SymbolName"]), + makeSymbol(id: "second-\(symbolKindID.identifier)-id", kind: symbolKindID, pathComponents: ["SymbolName"]), + makeSymbol(id: "third-\(symbolKindID.identifier)-id", kind: symbolKindID, pathComponents: ["SymbolName"]), + makeSymbol(id: "fourth-\(symbolKindID.identifier)-id", kind: symbolKindID, pathComponents: ["SymbolName"]), + ] + + let catalog = + Folder(name: "unit-test.docc", content: [ + JSONFile(name: "ModuleName.symbols.json", content: makeSymbolGraph( + moduleName: "ModuleName", + symbols: symbols + )), + + TextFile(name: "ModuleName.md", utf8Content: """ + # ``ModuleName`` + + This is a test file for ModuleName. + + ## Topics + + - ``Symbol`` + """) + ]) + + let (_, context) = try loadBundle(catalog: catalog) + + let problems = context.problems.sorted(by: \.diagnostic.summary) + XCTAssertEqual(problems.count, 1) + + let problem = try XCTUnwrap(problems.first) + + XCTAssertEqual(problem.diagnostic.summary, "'Symbol' doesn't exist at '/ModuleName'") + + XCTAssertEqual(problem.possibleSolutions.count, 1) + let solution = try XCTUnwrap(problem.possibleSolutions.first) + + XCTAssertEqual(solution.summary, "Replace 'Symbol' with 'SymbolName'") + + XCTAssertEqual(solution.replacements.count, 1) + let replacement = try XCTUnwrap(solution.replacements.first) + + XCTAssertEqual(replacement.range.lowerBound, .init(line: 7, column: 5, source: nil)) + XCTAssertEqual(replacement.range.upperBound, .init(line: 7, column: 11, source: nil)) + } + } + func testResolveExternalLinkFromTechnologyRoot() throws { enableFeatureFlag(\.isExperimentalLinkHierarchySerializationEnabled)