Skip to content

Conversation

@d-ronnqvist
Copy link
Contributor

@d-ronnqvist d-ronnqvist commented Sep 29, 2022

Bug/issue #, if applicable: #397 rdar://100436859

Summary

This shows the full symbol declarations in the error message for a symbol link that's ambiguous to help developers distinguish between the colliding symbols.

This also fixes a minor issue where the descriptions of unresolved references contained percent encoding, resulting in hard to read diagnostics.

These changes together refines this error message

Topic reference 'emit(_%3A)' couldn't be resolved. Reference is ambiguous after '/SwiftDocC/DiagnosticEngine': Add '1xqdb' to refer to 'emit(_:)'. Add '5rcke' to refer to 'emit(_:)'. 

into this error message

Topic reference 'emit(_:)' couldn't be resolved. Reference is ambiguous after '/SwiftDocC/DiagnosticEngine': Append '-1xqdb' to refer to 'func emit(_ problem: Problem)'. Append '-5rcke' to refer to 'func emit(_ problems: [Problem])'. 

Dependencies

n/a

Testing

With DOCC_USE_HIERARCHY_BASED_LINK_RESOLVER=YES write an ambiguous link, for example by adding this code

public enum CollisionsWithDifferentFunctionArguments {
    public func something(argument: Int) -> Int { 0 }
    public func something(argument: String) -> Int { 0 }
}

and linking to CollisionsWithDifferentFunctionArguments/something(argument:)

In the resulting error message it should be possible to distinguish the collisions.

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
Copy link
Contributor Author

@swift-ci please test


case .lookupCollision(let partialResult, let collisions):
let collisionDescription = collisions.map { "Add \($0.disambiguation.singleQuoted) to refer to \($0.node.fullNameOfValue(context: context).singleQuoted)"}.sorted()
let collisionDescription = collisions.map { "Append '-\($0.disambiguation)' to refer to \($0.node.fullNameOfValue(context: context).singleQuoted)"}.sorted()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we also delete the following "singleQuoted" call?

Suggested change
let collisionDescription = collisions.map { "Append '-\($0.disambiguation)' to refer to \($0.node.fullNameOfValue(context: context).singleQuoted)"}.sorted()
let collisionDescription = collisions.map { "Append '-\($0.disambiguation)' to refer to \($0.node.fullNameOfValue(context: context))"}.sorted()

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think so. I feel the the quotation marks makes it easier to follow where each declaration starts and ends in the error message.

I surrounded the disambiguation interpolation with quotation marks because I also needed to prefix it with a dash and I felt that '-\($0.disambiguation)' was more readable than \(("-" + $0.disambiguation).singleQuoted). In the rest of the code I think that we're mostly using .singleQuoted.

@d-ronnqvist
Copy link
Contributor Author

@swift-ci please test

try assertFindsPath("/MixedFramework/MyObjectiveCCompatibleProtocol/myProtocolMethod", in: tree, asSymbolID: "c:@M@MixedFramework@objc(pl)MyObjectiveCCompatibleProtocol(im)myProtocolMethod")
try assertFindsPath("/MixedFramework/MyObjectiveCCompatibleProtocol/myProtocolMethod()", in: tree, asSymbolID: "c:@M@MixedFramework@objc(pl)MyObjectiveCCompatibleProtocol(im)myProtocolMethod")
try assertFindsPath("/MixedFramework/MyObjectiveCCompatibleProtocol/myProtocolProperty", in: tree, asSymbolID: "c:@M@MixedFramework@objc(pl)MyObjectiveCCompatibleProtocol(py)myProtocolProperty")
// Objective-C class properties have a "property" kind instead of a "type.property" kind (rdar://92927788)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems unrelated, might be worth splitting this into a separate commit.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is a separate commit in the PR. Did you mean open a separate PR for this change?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, my bad I got confused, when reading the commit logs.

@d-ronnqvist
Copy link
Contributor Author

@swift-ci please test

@d-ronnqvist d-ronnqvist merged commit f5238c4 into swiftlang:main Oct 3, 2022
@d-ronnqvist d-ronnqvist deleted the decls-in-symbol-link-collision-diagnostics branch October 3, 2022 17:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Show declarations when suggesting what disambiguation to add to symbol links

4 participants