add AST printer support for _documentation attribute #64326
Conversation
fixes #64309 fixes rdar://106657906
QuietMisdreavus
requested review from
hborla,
slavapestov and
xedin
as code owners
|
@swift-ci Please smoke test. |
|
@swift-ci Please build toolchain Linux platform |
| // The @_documentation attribute caused sourcekit-lsp to crash | ||
| // cf. https://github.com/apple/swift/issues/64309 | ||
|
|
||
| // RUN: %sourcekitd-test -req=cursor -pos=2:6 %s -- %s |
There was a problem hiding this comment.
Where in the response is @_documentation showing up? If it's user inaccessible, I wouldn't have expected it in cursor info either (and if we do want it, it would be good to actually check for it).
There was a problem hiding this comment.
@bnbarham I added some output checking in this test; it seems to be showing up in the XML output. I'm not sure if this is expected, but i've updated the test.
There was a problem hiding this comment.
That is definitely very unexpected to me. IIUC the annotated description should not be including user inaccessible attributes. I guess we're missing adding the PrintUserInaccessibleAttrs in printQuickHelpDeclaration. Ideally we'd fix that as well, but if you don't want to do that it's fine.
If we really want a test for this I'd probably prefer something like a -dump-ast (assuming it includes user inaccessible?) in test/attr. Once we fix cursor info this test would be invalid otherwise.
There was a problem hiding this comment.
Adding the test in test/attr makes sense as a way to ensure that the attribute gets printed correctly, but i'd like to leave the test here (without the output checking) since that was the original error case, if that's alright.
There was a problem hiding this comment.
If you're changing the test anyway, I'd strongly prefer just not including it. The crash has a known cause that we can test, so I don't think there's much use in testing the crash itself.
There was a problem hiding this comment.
It looks like -dump-ast doesn't include UserInaccessible attributes either:
(source_file "/Users/victoria_m/swift-base/swift/test/attr/attr_documentation.swift"
(enum_decl range=[/Users/victoria_m/swift-base/swift/test/attr/attr_documentation.swift:5:1 - line:7:1] "E" interface type='E.Type' access=internal non-resilient))
There was a problem hiding this comment.
Ah that's a pity 😓. You can keep the cursor info test without output if you want. Really we should check that inaccessible attributes don't get printed in generated interface/completion/cursor info, but I don't expect you to do that.
There was a problem hiding this comment.
I took the output check out of the test. The test as a whole could be taken out or modified when the UserInaccessible filtering is added in the future.
|
@swift-ci Please smoke test |
|
@swift-ci Please smoke test |
1 similar comment
|
@swift-ci Please smoke test |
|
@swift-ci Please test |
fixes #64309 fixes rdar://106657906
fixes swiftlang#64309 fixes rdar://106657906
fixes #64309
fixes rdar://106657906
When the
@_documentationattribute was added in #60242, i neglected to add a printer implementation for it. This didn't affect SymbolGraphGen, since it doesn't printUserInaccessibleattributes in its declarations. However, sourcekit-lsp does print these attributes, which trips an assertion. This PR adds a printer implementation for the@_documentationattribute, allowing SourceKit to properly collect cursor info.