Skip to content

Conversation

@QuietMisdreavus
Copy link
Contributor

Resolves rdar://104930571

When SymbolGraphGen renders a custom attribute which it knows the USR for, it splits the declaration fragment for that attribute into two: one with the @ sign with kind attribute, and one with the name of the attribute with name typeIdentifier and a reference to the attribute definition USR. SymbolGraphGen forces any fragment with a link USR needs to be of kind typeIdentifier due to an implementation detail of Swift-DocC-Render; however, this produces an awkward rendering where the attribute appears in two colors because of the differing fragment kinds.

This PR changes the rendering code to keep attribute fragments intact when printing a type reference, instead of breaking off a separate typeIdentifier fragment.

@QuietMisdreavus
Copy link
Contributor Author

@swift-ci Please test

@ethan-kusters
Copy link
Contributor

Should we consider keeping these separated? I wouldn't expect the @ piece to be linked in a rendered version of the declaration since it's not actually part of the type name – it's just a piece of syntax.

{
    "kind": "attribute",
    "text": "@"
},
{
    "kind": "attribute",
    "preciseIdentifier": "s:10CustomAttr11SomeWrapperV",
    "text": "SomeWrapper"
},

@QuietMisdreavus
Copy link
Contributor Author

I could go either way. In my head, the @ of an attribute like this is part of the name, since that's how it will actually be used in real code. It may not be the name of the actual type, but the @ is part of how it is used in a declaration like this.

@ethan-kusters
Copy link
Contributor

I could go either way. In my head, the @ of an attribute like this is part of the name, since that's how it will actually be used in real code. It may not be the name of the actual type, but the @ is part of how it is used in a declaration like this.

I can see the argument for that. Maybe we already have precedent here with other syntax elements like [] though? For example: https://swiftpackageindex.com/apple/swift-argument-parser/1.2.2/documentation/argumentparser/parsablecommand/parseasroot(_:).

Even though [String] is referring to a string array type, we still link the String type specifically since that's what the documentation we're linking to is for. When you arrive on the documentation page for @AttributeName – the page will be for something like @resultBuilder AttributeName not @AttributeName.

@QuietMisdreavus
Copy link
Contributor Author

[String] is a distinct type from String, though. The square brackets make it into a different type entirely, and we could spell it out by linking the square brackets to the Array type, if we wanted to be maximally verbose. The @, however, isn't representative of any type at all. If anything, it's representative of the @resultBuilder or @propertyWrapper attributes, which affect how the type is used. You would hardly use SomeWrapper (from the test example) by itself; instead you use it as a decorator, as @SomeWrapper.

@ethan-kusters
Copy link
Contributor

Right I agree – but when I click on @SomeWrapper I would land on the page for SomeWrapper. The symbol graph should link the actual types – not the way they're used in the current syntax.

@ethan-kusters
Copy link
Contributor

I think Sendable is probably a better example: https://developer.apple.com/documentation/swift/sendable.

It can be written both as a protocol conformance and as an attribute so I think it would be odd to link to a different piece of text in the attribute-case.

@QuietMisdreavus
Copy link
Contributor Author

I went ahead and pushed a commit that breaks the token apart again like before, just with the attribute kind instead of the old typeIdentifier.

@QuietMisdreavus
Copy link
Contributor Author

@swift-ci Please test

@QuietMisdreavus
Copy link
Contributor Author

@swift-ci Please smoke test

@QuietMisdreavus QuietMisdreavus merged commit 594257f into main Feb 16, 2023
@QuietMisdreavus QuietMisdreavus deleted the QuietMisdreavus/attr-token-kind branch February 16, 2023 22:38
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.

4 participants