Skip to content

add AST printer support for _documentation attribute #64326

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.

Sign up for GitHub

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

Merged
merged 3 commits into from
Mar 31, 2023
Merged

add AST printer support for _documentation attribute #64326

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
2e77df2
add AST printer support for _documentation attribute
QuietMisdreavus Mar 13, 2023
ef9f875
review: add output checking for doc-attr test
QuietMisdreavus Mar 14, 2023
d096114
review: remove check for cursor info
QuietMisdreavus Mar 15, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 26 additions & 0 deletions lib/AST/Attr.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1394,6 +1394,32 @@ bool DeclAttribute::printImpl(ASTPrinter &Printer, const PrintOptions &Options,
break;
}

case DAK_Documentation: {
auto *attr = cast<DocumentationAttr>(this);

Printer.printAttrName("@_documentation");
Printer << "(";

bool needs_comma = !attr->Metadata.empty() && attr->Visibility;

if (attr->Visibility) {
Printer << "visibility: ";
Printer << getAccessLevelSpelling(*attr->Visibility);
}

if (needs_comma) {
Printer << ", ";
}

if (!attr->Metadata.empty()) {
Printer << "metadata: ";
Printer << attr->Metadata;
}

Printer << ")";
break;
}

case DAK_Count:
llvm_unreachable("exceed declaration attribute kinds");

Expand Down
9 changes: 9 additions & 0 deletions test/SourceKit/CursorInfo/doc_attr.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
@_documentation(visibility: public)
enum E
{
}

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

@bnbarham bnbarham Mar 14, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

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).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

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 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))

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

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 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.