Add logic to look for overload groups when emitting warnings #885

patshaughnessy · 2024-04-13T00:45:30Z

Add logic to look for overload groups when emitting warnings and suggestions about ambiguous symbols. If exactly one of the ambiguous symbols is an overload group, then suggest the author simply remove the incorrect disambiguation instead of selecting a different disambiguation hash.

rdar://126382989

Summary

Today when an author links to a page using an invalid disambiguation hash, DocC emits a warning similar to this listing all of the valid disambiguation hashes:

warning: '7w26t' isn't a disambiguation for 'init(content:)' at '/MyFramework/SomeStruct'
 --> SomeStruct.md:7:19-7:25
5 | ### Some Interesting Topic
6 |
7 + - ``init(content:)-7w26t``
  |                   ├─suggestion: Replace '7w26t' with '25rvu' for'something'
  |                   ├─suggestion: Replace '7w26t' with '4tm5u' for'something-else'
  |                   ╰─suggestion: Replace '7w26t' with '5di5z' for'something-even-different'
8 | - ``init(content:)-8mfe``

However, we expect that most of the time authors need to specify disambiguation hashes when they are linking directly to one member of a set of overloaded symbols. (Symbols that have the same parameters but different types).

Therefore, this PR changes the diagnostic logic for ambiguous symbols to emit a warning similar to this instead:

warning: '7w26t' isn't a disambiguation for 'init(content:)' at '/MyFramework/SomeStruct'
 --> SomeStruct.md:7:19-7:25
5 | ### Some Interesting Topic
6 |
7 + - ``init(content:)-7w26t``
  |                   ╰─suggestion: Remove '-7w26t' to use overload group 'something'
8 | - ``init(content:)-8mfe``

Now DocC will encourage writers to use simpler paths when possible, allowing readers to navigate to overload group pages.

We might consider adding logic in a follow-on PR to emit warnings for valid links that use disambiguation hashes unnecessarily.

Dependencies

None

Testing

Steps:

Compile documentation that contains a broken link due to an incorrect disambiguation hash. Use the --enable-experimental-overloaded-symbol-presentation command line argument to enable the experimental overloads feature flag.
Check that the target page is one symbol is a set of overloaded symbols
Test that DocC emits a warning similar to the example above.
Repeat the test when the broken link selects ambiguous symbols that are not all overloads in the same group. DocC should emit the original warning.

Checklist

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

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

suggestions about ambiguous symbols. If exactly one of the ambiguous symbols is an overload group, then suggest the author simply remove the incorrect disambiguation.

patshaughnessy · 2024-04-13T00:53:19Z

@swift-ci please test

d-ronnqvist

This shouldn't be special behavior for overload groups. It should be a general behavior whenever there is only one candidate who isn't disfavored in collisions (and therefore doesn't need disambiguation in its link).

QuietMisdreavus

Thanks so much for this! There are a few tweaks i spotted that i'd like to see, but overall i'm super glad to see this change.

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

Tests/SwiftDocCTests/Infrastructure/DocumentationBundleInfoTests.swift

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

about selecting the preferred symbol without any disambiguation hash, followed by a list of all the remaining symbols to select from with their hashes.

…ad-groups

patshaughnessy · 2024-04-16T21:02:11Z

@swift-ci please test

patshaughnessy · 2024-04-16T22:57:43Z

I've refactored this to display all the ambiguous links like before, but showing the preferred ("favored") symbol first. Example:

'3xblz' isn't a disambiguation for 'init(param:)' at '/MyFramework/SomeClass'.
Remove '-3xblz' to select preferred symbol 'init(param:)'
Replace '3xblz' with '6swzo' for 'init(param:)'
Replace '3xblz' with '3bg7x' for etc...

d-ronnqvist · 2024-04-17T07:36:42Z

I've refactored this to display all the ambiguous links like before, but showing the preferred ("favored") symbol first. Example:
'3xblz' isn't a disambiguation for 'init(param:)' at '/MyFramework/SomeClass'.
Remove '-3xblz' to select preferred symbol 'init(param:)'
Replace '3xblz' with '6swzo' for 'init(param:)'
Replace '3xblz' with '3bg7x' for etc...

Did we stop displaying the full declaration or is that something you simplified in this example? Without the full declaration there's no way to know which disambiguation refers to which symbol.

patshaughnessy · 2024-04-17T16:42:20Z

Did we stop displaying the full declaration or is that something you simplified in this example?
No. I simplified the example.

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

Tests/SwiftDocCTests/Infrastructure/PathHierarchyTests.swift

- PathHierarchy.makePartialResultError checks for a preferred symbol, normally an overload group, when throwing processing unknownDisambiguation errors. - Simplify makeTopicReferenceResolutionErrorInfo and limit it to formatting the warning message.

…ad-groups

patshaughnessy · 2024-04-23T23:43:11Z

@swift-ci please test

d-ronnqvist · 2024-04-24T07:29:15Z

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

+        let candidates = disambiguationTree.disambiguatedValues()
+        let favoredSuffix = favoredSuffix(from: candidates)
+        let suffixes = candidates.map { suffix(for: $0.disambiguation) }
+        let candidatesAndSuffixes = zip(candidates, suffixes).map { (candidate, suffix) in
+            if suffix == favoredSuffix {
+                return (node: candidate.value, disambiguation: "")
+            } else {
+                return (node: candidate.value, disambiguation: suffix)
+            }
+        }


I think it would be useful for this logic to happen in disambiguatedValues(). That way, for example, the URL for the overloads page wouldn't need a hash.

I agree! I investigated this a bit and I think it will lead to cleaner code and simpler URLs. However, I'd suggest we make this additional change in a separate PR in the near future. Changing the URLs will require updating a large number of unit test expectations, and also will lead to moving pages around in production, leading to more testing, creating the necessary redirects and generally more risk.

For now, I think updating the warning is a good first step.

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

d-ronnqvist · 2024-04-24T07:44:12Z

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

-                        Replacement(range: replacementRange, replacement: "-" + disambiguation)
-                    ])
+                    // Suggest removing the disambiguation hash entirely
+                    if disambiguation.isEmpty {


Ideally this would happen automatically, without the special check here.

I found that we need to keep this check. It is possible to remove this check entirely, but then we would have to add the - prefix to all the unknownDisambiguation warnings throughout all the unit tests.

Example:

Replace '-abc123' with '-method' for\n'func `init`()'"

I think this reads fine for users, but I wouldn't want to make this change across all the unit test expectations unless you like this idea,

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

PathHierarchy.makePartialResultError further.

…ad-groups

patshaughnessy · 2024-04-24T19:48:50Z

@swift-ci please test

…ad-groups

- Further simplify logic for displaying unknown disambiguation warnings

patshaughnessy · 2024-04-25T23:30:17Z

@swift-ci please test

patshaughnessy · 2024-04-26T00:21:48Z

@d-ronnqvist I simplified PathHierarchy+Error.swift even more, and updated some unit test expectations to account for the extra hyphen in the warning message. I think the code looks very clean now, and also the warnings read even better or the same as before.

QuietMisdreavus

This looks wonderful, great work!

…ad-groups

patshaughnessy · 2024-04-26T16:50:20Z

@swift-ci please test

…ng#885) * Add logic to look for overload groups when emitting warnings and suggestions about ambiguous symbols. If exactly one of the ambiguous symbols is an overload group, then suggest the author simply remove the incorrect disambiguation. * Refactor logic for ambiguous symbol warnings to first emit a message about selecting the preferred symbol without any disambiguation hash, followed by a list of all the remaining symbols to select from with their hashes. * PR Feedback: - PathHierarchy.makePartialResultError checks for a preferred symbol, normally an overload group, when throwing processing unknownDisambiguation errors. - Simplify makeTopicReferenceResolutionErrorInfo and limit it to formatting the warning message. * PR Feedback: Simplify PathHierarchy+Error.swift and PathHierarchy.makePartialResultError further. * PR Feedback: - Further simplify logic for displaying unknown disambiguation warnings (cherry picked from commit 1357a2d)

…ng#885) * Add logic to look for overload groups when emitting warnings and suggestions about ambiguous symbols. If exactly one of the ambiguous symbols is an overload group, then suggest the author simply remove the incorrect disambiguation. * Refactor logic for ambiguous symbol warnings to first emit a message about selecting the preferred symbol without any disambiguation hash, followed by a list of all the remaining symbols to select from with their hashes. * PR Feedback: - PathHierarchy.makePartialResultError checks for a preferred symbol, normally an overload group, when throwing processing unknownDisambiguation errors. - Simplify makeTopicReferenceResolutionErrorInfo and limit it to formatting the warning message. * PR Feedback: Simplify PathHierarchy+Error.swift and PathHierarchy.makePartialResultError further. * PR Feedback: - Further simplify logic for displaying unknown disambiguation warnings

…906) * Add logic to look for overload groups when emitting warnings and suggestions about ambiguous symbols. If exactly one of the ambiguous symbols is an overload group, then suggest the author simply remove the incorrect disambiguation. * Refactor logic for ambiguous symbol warnings to first emit a message about selecting the preferred symbol without any disambiguation hash, followed by a list of all the remaining symbols to select from with their hashes. * PR Feedback: - PathHierarchy.makePartialResultError checks for a preferred symbol, normally an overload group, when throwing processing unknownDisambiguation errors. - Simplify makeTopicReferenceResolutionErrorInfo and limit it to formatting the warning message. * PR Feedback: Simplify PathHierarchy+Error.swift and PathHierarchy.makePartialResultError further. * PR Feedback: - Further simplify logic for displaying unknown disambiguation warnings (cherry picked from commit 1357a2d)

Add logic to look for overload groups when emitting warnings and

9476a06

suggestions about ambiguous symbols. If exactly one of the ambiguous symbols is an overload group, then suggest the author simply remove the incorrect disambiguation.

patshaughnessy requested review from d-ronnqvist and QuietMisdreavus April 13, 2024 00:45

d-ronnqvist requested changes Apr 15, 2024

View reviewed changes

QuietMisdreavus requested changes Apr 15, 2024

View reviewed changes

patshaughnessy force-pushed the r126382989-warnings-ambiguous-symbols-overload-groups branch from 84db713 to 2f756bf Compare April 16, 2024 20:41

Refactor logic for ambiguous symbol warnings to first emit a message

d610548

about selecting the preferred symbol without any disambiguation hash, followed by a list of all the remaining symbols to select from with their hashes.

patshaughnessy force-pushed the r126382989-warnings-ambiguous-symbols-overload-groups branch from 9520ba7 to d610548 Compare April 16, 2024 21:01

Merge branch 'main' into r126382989-warnings-ambiguous-symbols-overlo…

b588c9d

…ad-groups

patshaughnessy requested review from d-ronnqvist and QuietMisdreavus April 16, 2024 22:42

d-ronnqvist requested changes Apr 19, 2024

View reviewed changes

patshaughnessy added 2 commits April 23, 2024 16:31

PR Feedback:

c1b107a

- PathHierarchy.makePartialResultError checks for a preferred symbol, normally an overload group, when throwing processing unknownDisambiguation errors. - Simplify makeTopicReferenceResolutionErrorInfo and limit it to formatting the warning message.

Merge branch 'main' into r126382989-warnings-ambiguous-symbols-overlo…

ca5d4da

…ad-groups

patshaughnessy requested a review from d-ronnqvist April 23, 2024 23:41

d-ronnqvist reviewed Apr 24, 2024

View reviewed changes

Sources/SwiftDocC/Infrastructure/Link Resolution/PathHierarchy+Find.swift Outdated Show resolved Hide resolved

d-ronnqvist reviewed Apr 24, 2024

View reviewed changes

Sources/SwiftDocC/Infrastructure/Link Resolution/PathHierarchy+Error.swift Outdated Show resolved Hide resolved

patshaughnessy added 2 commits April 24, 2024 12:43

PR Feedback: Simplify PathHierarchy+Error.swift and

3492fcc

PathHierarchy.makePartialResultError further.

Merge branch 'main' into r126382989-warnings-ambiguous-symbols-overlo…

779e612

…ad-groups

patshaughnessy requested a review from d-ronnqvist April 24, 2024 22:18

d-ronnqvist approved these changes Apr 25, 2024

View reviewed changes

patshaughnessy added 2 commits April 25, 2024 16:14

Merge branch 'main' into r126382989-warnings-ambiguous-symbols-overlo…

11c700b

…ad-groups

PR Feedback:

9745fbd

- Further simplify logic for displaying unknown disambiguation warnings

QuietMisdreavus approved these changes Apr 26, 2024

View reviewed changes

Merge branch 'main' into r126382989-warnings-ambiguous-symbols-overlo…

9754e48

…ad-groups

patshaughnessy merged commit 1357a2d into swiftlang:main Apr 26, 2024

patshaughnessy deleted the r126382989-warnings-ambiguous-symbols-overload-groups branch April 26, 2024 18:42

patshaughnessy mentioned this pull request Apr 29, 2024

[6.0] Add logic to look for overload groups when emitting warnings (#885) #906

Merged

Add logic to look for overload groups when emitting warnings #885

Add logic to look for overload groups when emitting warnings #885

Uh oh!

Conversation

patshaughnessy commented Apr 13, 2024

Summary

Dependencies

Testing

Checklist

Uh oh!

patshaughnessy commented Apr 13, 2024

Uh oh!

d-ronnqvist left a comment

Choose a reason for hiding this comment

Uh oh!

QuietMisdreavus left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

patshaughnessy commented Apr 16, 2024

Uh oh!

patshaughnessy commented Apr 16, 2024

Uh oh!

d-ronnqvist commented Apr 17, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

patshaughnessy commented Apr 17, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

patshaughnessy commented Apr 23, 2024

Uh oh!

d-ronnqvist Apr 24, 2024

Choose a reason for hiding this comment

Uh oh!

patshaughnessy Apr 24, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

d-ronnqvist Apr 24, 2024

Choose a reason for hiding this comment

Uh oh!

patshaughnessy Apr 24, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

patshaughnessy commented Apr 24, 2024

Uh oh!

patshaughnessy commented Apr 25, 2024

Uh oh!

patshaughnessy commented Apr 26, 2024

Uh oh!

QuietMisdreavus left a comment

Choose a reason for hiding this comment

Uh oh!

patshaughnessy commented Apr 26, 2024

Uh oh!

Uh oh!

d-ronnqvist commented Apr 17, 2024 •

edited

Loading