-
Notifications
You must be signed in to change notification settings - Fork 161
use found disambiguation to calculate diagnostic range if present #1213
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
use found disambiguation to calculate diagnostic range if present #1213
Conversation
|
@swift-ci Please test |
d-ronnqvist
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me. Just one question and some minor non-blocking comments on the test.
Tests/SwiftDocCTests/Infrastructure/DocumentationContext/DocumentationContextTests.swift
Outdated
Show resolved
Hide resolved
Tests/SwiftDocCTests/Infrastructure/DocumentationContext/DocumentationContextTests.swift
Outdated
Show resolved
Hide resolved
| let nextPathComponent = remaining.first! | ||
| let (pathPrefix, _, solutions) = makeCollisionSolutions(from: collisions, nextPathComponent: nextPathComponent, partialResultPrefix: partialResult.pathPrefix) | ||
|
|
||
| let (pathPrefix, foundDisambiguation, solutions) = makeCollisionSolutions( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
question: Is .lookupCollision the only kind of value where this happened or should we add similar checks to the other types of errors as well?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like the rangeAdjustment feature is also used by the unknownDisambiguation and unknownName errors, which could also potentially be wrong. Would you like me to investigate these options in this PR, or land this as-is and investigate those later?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Either way works. If it's easy to investigate and fix it might make sense to do it while all of this is fresh in the mind.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added tests for those cases to see if they would also have problems, and it looks like they're fine. 🤷♀️ I'll leave the tests in for posterity, but i didn't need to change anything for the cases i tested.
rdar://150207195
24b8f9b to
a7b2a37
Compare
|
@swift-ci Please test |
Bug/issue #, if applicable: rdar://150207195
Summary
The link resolver emits diagnostics for symbol links that require a disambiguation suffix (or a more precise suffix) to resolve to a single symbol. The diagnostic also includes fix-it solutions suggesting the available suffixes for the conflicting symbols. However, the logic for reporting the ranges of these possible replacements has a flaw when reporting on a curated symbol link: it always uses the full name of the symbol as the base instead of the written symbol or just the suffix.
This PR updates the diagnostic logic to handle the case of a found disambiguation suffix separately when reporting the diagnostic solutions.
Dependencies
None
Testing
The functionality is difficult to test outside of a unit test environment, as it requires non-overloadable symbols to have conflicting names. This is primarily only going to happen with errors in symbol graph generation that cause separate sets of symbols to exist. Nonetheless, the unit test captures the behavior.
Checklist
Make sure you check off the following items. If they cannot be completed, provide a reason.
./bin/testscript and it succeeded