Fix #438 (Structure suggestions for link resolution warnings as fixits) #455

theMomax · 2023-01-06T13:58:27Z

Bug/issue #, if applicable: #438

Summary

This PR adds fixits to the warnings generated by reference resolution for near-miss and collision scenarios.

The fixits are always generated so that they replace the whole reference, i.e. the replacement area includes the whole reference link, plus the surrounding backticks.

Fixits only fix the first faulty segment in a reference. They do not alter the valid prefix and keep the unchecked suffix in place.

For example:

/MyKit/MyClas -> /MyKit/MyClass
MyClas -> MyClass
MyClas/validMember -> MyClass/validMember
MyClas/invalidMember -> MyClass/invalidMember -> MyClass/validMember (the second fixit only becomes available after running docc again)

The implementation is structured as follows:

the actual replacement strings are generated in PathHierarchy/Error/replacements(for:)
PathHierarchyBasedLinkResolver/resolve(_:in:fromSymbolLink:context:) calls PathHierarchy/Error/replacements(for:) and attaches them as candidates to the UnresolvedTopicReference returned in the TopicReferenceResolutionResult/failure(_:errorMessage:)
(Adding the candidates field to UnresolvedTopicReference avoids a breaking change in TopicReferenceResolutionResult/failure(_:errorMessage:)).
MarkupReferenceResolver and ReferenceResolver pass the candidates to unresolvedReferenceProblem(reference:source:range:severity:uncuratedArticleMatch:underlyingErrorMessage:candidates:), which then generates Solutions if there are candidates available and attaches them to the Problem

Beyond that I had to make minor adaptions to make sure range offsets are applied not only to a Problem's diagnostic, but also to possibleSolutions.

Dependencies

None.

Testing

Updated SymbolTests/testUnresolvedReferenceWarningsInDocumentationExtension() to also test fixits and added a couple of test-cases relevant for fixits.

Make sure to set DOCC_USE_HIERARCHY_BASED_LINK_RESOLVER=YES when running the tests.

For manual testing also make sure to enable fixits with --emit-fixits, e.g.: DOCC_USE_HIERARCHY_BASED_LINK_RESOLVER=YES .build/debug/docc convert Sources/SwiftDocC/SwiftDocC.docc --emit-fixits --additional-symbol-graph-dir .build/arm64-apple-macosx/symbolgraph

Checklist

Make sure you check off the following items. If they cannot be completed, provide a reason.

Added tests
Ran the ./bin/test script and it succeeded
Updated documentation if necessary

d-ronnqvist · 2023-01-07T01:58:24Z

Thanks for this PR. I will review it early next week.

d-ronnqvist

This is looking like a great start.

For a developer being presented with one of these diagnostics it would be more helpful if more information was moved to the suggestions instead of the error message. (The suggestions were originally added to the error message because there wasn't any more structure.)

If a developer got a collision error message with all solutions in the error message they'd have to either assume that the order of in the error message is the same as the order of the replacements or they'd have to compare the replacements (if their IDE displays that information) against the error message to know which replacement to pick to get the expected symbol. Inspecting the replacements could be somewhat cumbersome if the link is long since the full link is replaced. It would be easier to inspect the replacements if only the modified path component was replaced (or if collision error messages were insertions instead of replacements).

Like you've identified in the Error.replacements(for:) comment; "No similar pages. Available children: ..." is not a very helpful replacement. Maybe it would be better presented as a note on the diagnostic but it's probably fine to keep that in the error message.

In the future it would be helpful to offer a replacement for PathHierarchy.Error.nonSymbolMatchForSymbolLink that replaces ``Link`` with <doc:Link> .

It's a smart way to avoid the breaking change by adding the candidates to the unresolved topic reference but in order to achieve some of the error structure described above—now or in the future—I think we'll want to add more information that will outgrow UnresolvedTopicReference.

I think we'll want to consolidate the various error information into one place. Over time this may grow to include: the error message, suggestion messages, text and subranges for replacements, notes, etc.

It's not necessarily wrong to have this information on the unresolved reference but I feel that the distinction between not-yet-resolved and failed-to-resolve is better modeled by TopicReferenceResolutionResult (not-yet-resolved reference as input and either successfully-resolved or failed-to-resolved reference as output).

It was shortsighted of me to only have an error message on TopicReferenceResolutionResult but I think it'd be worth a breaking change if we could create well structured diagnostics and had a type that we could improve and extend without breaking changes in the future. One such example would be to replace the error message with a struct that wraps all the error information.

At a minimal I think this should include the error message and a separate message and replacement for each suggestion.

It would be a nice improvement if this also included a subrange for each suggestion. That way .partialResult error could replace only one path component and .lookupCollision could insert at the end of a certain path component. It would also be an improvement if this could support messages for notes. These improvements could be added later by someone else but it's important that, going forward, adding more error information like this isn't another breaking change.

Sources/SwiftDocC/Infrastructure/Diagnostics/Problem.swift

Sources/SwiftDocC/Model/Identifier.swift

Sources/SwiftDocCTestUtilities/Sequence+XCTAssert.swift

Sources/SwiftDocC/Model/Identifier.swift

Sources/SwiftDocC/Semantics/ReferenceResolver.swift

Tests/SwiftDocCTests/Semantics/SymbolTests.swift

theMomax · 2023-01-18T15:42:35Z

@d-ronnqvist thanks for taking the time to review this. Your reviews are always well thought-out and provide so much detail, they really are the best!

I agree with all your comments. I just wanted to offer this minimally intrusive solution (also as a basis for discussion) before investing too much time into a solution that could be fundamentally different from what you imagined.

Please note that I'd like to add a couple more tests, but the implementation is ready for another review. I'd be particularly interested on what public interface you'd like to see on TopicReferenceResolutionError. The way it's designed right now is that everything but the conformance to Error is internal or private, and even the public localizedDescription, which is synthesized for all errors won't be particularly useful, as it won't show the same text as the internal custom property. I could fix that by conforming the type to DescribedError, but that would already inflate the public API quite a bit.

d-ronnqvist · 2023-01-20T00:47:24Z

Apologies, I won’t have time to look at this until sometime next week.

d-ronnqvist

This looks really good.

I left a few minor comments but nothing that's blocking this PR (and some that would be better to address separately)

Tests/SwiftDocCTests/Semantics/SymbolTests.swift

d-ronnqvist · 2023-01-26T21:24:08Z

Sources/SwiftDocC/Semantics/ReferenceResolver.swift

+        let innerRange = fromSymbolLink
+            // ``my/reference``
+            ? SourceLocation(line: range.lowerBound.line, column: range.lowerBound.column+2, source: range.lowerBound.source)..<SourceLocation(line: range.upperBound.line, column: range.upperBound.column-2, source: range.upperBound.source)
+            // <doc:my/reference>


It's also possible that the link could have been written with the []() syntax, for example: [](doc:my/reference), [doc:my/reference](doc:my/reference), or [custom title](doc:my/reference).

DocC doesn't have a good way to know what how the original link was written—and also doesn't support custom titles (see https://github.com/apple/swift-docc/issues/271)—so I feel that this is fine for the replacement text. In the future when we have more information we can improve the replacement.

Good point! I created #470 and added a FIXME.

d-ronnqvist · 2023-01-26T21:31:04Z

Tests/SwiftDocCTests/Infrastructure/PathHierarchyTests.swift

@@ -314,24 +312,14 @@ class PathHierarchyTests: XCTestCase {
            (symbolID: "s:14MixedFramework29CollisionsWithEscapedKeywordsC4inityyF", disambiguation: "method"),
            (symbolID: "s:14MixedFramework29CollisionsWithEscapedKeywordsC4inityyFZ", disambiguation: "type.method"),
        ])
-        try assertPathRaisesErrorMessage("/MixedFramework/CollisionsWithEscapedKeywords/init()", in: tree, context: context, expectedErrorMessage: """
-        Reference is ambiguous after '/MixedFramework/CollisionsWithEscapedKeywords': \
-        Append '-init' to refer to 'init()'. \


It would be nice to have test assertions for potential notes and solutions. This doesn't have to be done in this PR.

It may make the call site hard to read if all the expected values were passed as arguments. One alternative could be to have a trailing closure argument that passes the error struct in so that the call site can make the individual assertions.

I added a trailing closure argument as you suggested and use it to check the semantic information contained in the solutions that was previously part of the error message. I don't check every detail such as the location of replacements, as that is not really the focus of this test.

I defined some local helper extensions and structures, which IMO provide for pretty clean test code:

try assertPathRaisesErrorMessage("/MixedFramework/CollisionsWithEscapedKeywords/init()", in: tree, context: context, expectedErrorMessage: "Reference is ambiguous after '/MixedFramework/CollisionsWithEscapedKeywords'.") { error in XCTAssertEqual(error.solutions, [ .init(summary: "Insert disambiguation suffix for 'func `init`()'", replacements: ["-method"]), .init(summary: "Insert disambiguation suffix for 'init()'", replacements: ["-init"]), .init(summary: "Insert disambiguation suffix for 'static func `init`()'", replacements: ["-type.method"]), ]) }

d-ronnqvist · 2023-01-26T21:41:06Z

Sources/SwiftDocC/Infrastructure/Link Resolution/PathHierarchy.swift

+            return TopicReferenceResolutionError("No local documentation matches this reference.")
+        case .nonSymbolMatchForSymbolLink:
+            return TopicReferenceResolutionError("Symbol links can only resolve symbols.", solutions: [
+                Solution(summary: "Use a '<doc:>' style reference.", replacements: [


Nice solution and replacement

Thanks! I think its a good idea to have this error category but, unfortunately, often times the resolution algorithm already aborts with a different error during the symbol-restricted lookup, even though it could resolve it correctly if the correct reference syntax had been used. This causes some very misleading error to be emitted (e.g. "no top-level symbol with name MyArticle"), because the nonSymbolMatchForSymbolLink error only gets thrown if the symbol-restricted lookup succeeds without result. It would be great to have a better analysis of the root-cause there some day, but that's definitely out of scope for this PR.

Tests/SwiftDocCTests/Infrastructure/PathHierarchyTests.swift

d-ronnqvist · 2023-01-26T21:52:13Z

Sources/SwiftDocC/Infrastructure/Link Resolution/PathHierarchyBasedLinkResolver.swift

@@ -222,7 +222,12 @@ final class PathHierarchyBasedLinkResolver {
            if let resolvedFallbackReference = fallbackResolver.resolve(unresolvedReference, in: parent, fromSymbolLink: isCurrentlyResolvingSymbolLink, context: context) {
                return .success(resolvedFallbackReference)
            } else {
-                return .failure(unresolvedReference, errorMessage: error.errorMessage(context: context))
+                var originalReferenceString = unresolvedReference.path
+                if let fragment = unresolvedReference.topicURL.components.fragment {


I'm mostly curious why this is needed. Is there a bug where the unresolved reference drops the fragment? If so, please add a FIXME comment that mentions the issue and gives context to why the originalReferenceString is needed.

I need the originalReferenceString in order to calculate the proper ranges for replacements.

The only information I have on the valid prefix of the reference based solely on the PathHierarchy.Error is the partialResult. This doesn't give me information on which ancestor the reference is relative to, thus I don't know the length of the original string. Whether or not the reference includes a fragment doesn't really matter.

d-ronnqvist · 2023-01-26T22:06:22Z

Sources/SwiftDocC/Model/Identifier.swift

+    // we store the base as an `Error` so that
+    // we can potentailly pass through a `DescribedError`
+    // conformance in the future
+    private let base: Error


If the message and note was stored directly instead of storing the base error then this struct can conform to DescribedError. It also makes it easier to conform to Hashable (although I think Solution and Replacement would need to explicitly declare conformance as well).

The functionality of accepting any error as input can still be achieved with an initializer that takes an error argument and reads the localizedDescription and recoverySuggestion.

I added all four properties of the DescribedError protocol as stored constants to the error so I don't loose information when passing though an error. Hashable conformance is better now 👍

Sources/SwiftDocC/Infrastructure/DocumentationContext.swift

d-ronnqvist · 2023-02-07T17:12:37Z

If you update this PR (either merge or rebase) I can merge it for you. I can also do it for you if you prefer.

theMomax · 2023-02-08T10:04:03Z

Thanks David for reminding me, I was a little occupied with other things last week. I updated the PR and ran bin/test again. It's ready for merge now. Feel free to update it yourself in case it's become outdated again by the time you get to merging it 👍

d-ronnqvist · 2023-02-08T17:09:30Z

@swift-ci please test

… as fixits)

- create TopicReferenceResolutionError with minimal public interface - replace TopicReferenceResolutionResult.failure's errorMessage with TopicReferenceResolutionError - add support for non-symbol reference syntax - add Solution for PathHierarchy.Error.nonSymbolMatchForSymbolLink - add meaningful information to Solution.summary - scope Replacements as small as possible (on disambiguation, path segment, or reference delimiters) - add note listing all available candidates even if near-miss suggestions are available - add Solutions if no near-miss is available, but there are only three candidates or less - make offsetWithRange mutating - remove XCTAssertElements test helper

- add DescribedError conformance TopicReferenceResolutionError - improve Hashable implementation of TopicReferenceResolutionError by storing properties demanded by DescribedError instead of raw Error - conform Replacement and Solution to Hashable to aid Hashable implementation of TopicReferenceResolutionError - add fixme regarding support for regular markdown link-style references when creating solutions (swiftlang#470) - add basic semantic information about possible solutions back into PathHierarchyTests - remove debug print in SymbolTests

…rningsInDocComment() + improve improve documentation about indices in source locations

d-ronnqvist · 2023-02-14T19:20:18Z

@swift-ci please test

d-ronnqvist · 2023-02-14T20:17:55Z

I've been holding off on merging this for a few days while I've been investigating how these diagnostics present in IDEs and on the command line.

Because of limitations with how diagnostics are output, the information about which disambiguation refers to which symbol never gets presented. I opened #478 about this.

I think that my plan is to merge this PR sometime in the next few days and immediately follow it up with another PR that restores the presentation of this information by including the suggested solution messages in the general diagnostic message. That way we get all the infrastructure benefits of have structured diagnostics values in the code while maintaining an equivalent presentation to what we have today.

Further follow up PRs at a later time can work to introduce new output formats for tools and for people that better utilize these structured diagnostics values.

d-ronnqvist · 2023-02-27T19:04:32Z

@swift-ci please test

theMomax force-pushed the near-miss-fixits branch from 8ff15f0 to 025d425 Compare January 6, 2023 14:06

d-ronnqvist self-assigned this Jan 6, 2023

d-ronnqvist self-requested a review January 6, 2023 17:23

d-ronnqvist reviewed Jan 10, 2023

View reviewed changes

Tests/SwiftDocCTests/Semantics/SymbolTests.swift Outdated Show resolved Hide resolved

theMomax force-pushed the near-miss-fixits branch from 025d425 to 79f7219 Compare January 18, 2023 15:40

theMomax requested a review from d-ronnqvist January 18, 2023 15:53

d-ronnqvist approved these changes Jan 26, 2023

View reviewed changes

theMomax mentioned this pull request Jan 27, 2023

Fixits produce invalid references when used with the regular markdown link syntax #470

Open

2 tasks

theMomax force-pushed the near-miss-fixits branch from 79f7219 to 16a0833 Compare January 29, 2023 11:21

theMomax force-pushed the near-miss-fixits branch from 19eb060 to f54a17c Compare February 8, 2023 10:03

theMomax added 4 commits February 9, 2023 09:53

fix swiftlang#438 (Structure suggestions for link resolution warnings…

bb28aa6

… as fixits)

add assertions for solutions in SymbolTests/testUnresolvedReferenceWa…

6911594

…rningsInDocComment() + improve improve documentation about indices in source locations

theMomax force-pushed the near-miss-fixits branch from f54a17c to 6911594 Compare February 9, 2023 09:29

d-ronnqvist mentioned this pull request Feb 14, 2023

Diagnostics output doesn't display messages for suggested solutions. #478

Closed

2 tasks

Merge branch 'main' into near-miss-fixits

0fa1797

d-ronnqvist merged commit 94d8aca into swiftlang:main Feb 27, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix #438 (Structure suggestions for link resolution warnings as fixits) #455

Fix #438 (Structure suggestions for link resolution warnings as fixits) #455

theMomax commented Jan 6, 2023

d-ronnqvist commented Jan 7, 2023

d-ronnqvist left a comment

theMomax commented Jan 18, 2023

d-ronnqvist commented Jan 20, 2023

d-ronnqvist left a comment

d-ronnqvist Jan 26, 2023

theMomax Jan 27, 2023

d-ronnqvist Jan 26, 2023

theMomax Jan 29, 2023

d-ronnqvist Jan 26, 2023

theMomax Jan 29, 2023

d-ronnqvist Jan 26, 2023

theMomax Jan 29, 2023

d-ronnqvist Jan 26, 2023

theMomax Jan 29, 2023

d-ronnqvist commented Feb 7, 2023

theMomax commented Feb 8, 2023

d-ronnqvist commented Feb 8, 2023

d-ronnqvist commented Feb 14, 2023

d-ronnqvist commented Feb 14, 2023

d-ronnqvist commented Feb 27, 2023

Fix #438 (Structure suggestions for link resolution warnings as fixits) #455

Fix #438 (Structure suggestions for link resolution warnings as fixits) #455

Conversation

theMomax commented Jan 6, 2023

Summary

Dependencies

Testing

Checklist

d-ronnqvist commented Jan 7, 2023

d-ronnqvist left a comment

Choose a reason for hiding this comment

theMomax commented Jan 18, 2023

d-ronnqvist commented Jan 20, 2023

d-ronnqvist left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

d-ronnqvist commented Feb 7, 2023

theMomax commented Feb 8, 2023

d-ronnqvist commented Feb 8, 2023

d-ronnqvist commented Feb 14, 2023

d-ronnqvist commented Feb 14, 2023

d-ronnqvist commented Feb 27, 2023