[SourceKit] Add documentation range in structure (SR-2487) #11264

marcelofabri · 2017-07-30T18:43:16Z

This PR adds the key.docoffset and key.doclength keys for declaration structures.

c78f169 removed the __raw_doc_comment attribute from structure, which was used by SwiftLint to implement some rules, for example checking if all public declarations have documentation. You can see an issue that was filled when that happened: realm/SwiftLint#728.

A while ago, @keith opened #9868 to revert that commit, but that wasn't accepted and the suggestion was to introduce new fields exposing the range of a doc comment. This is what this PR is about.

Feel free to make suggestions or point better alternative implementations.

Resolves SR-2487.

marcelofabri · 2017-08-01T06:12:23Z

cc @nkcsgexi

nkcsgexi · 2017-08-01T17:24:21Z

lib/IDE/SyntaxModel.cpp

@@ -772,6 +774,7 @@ bool ModelASTWalker::walkToDeclPre(Decl *D) {
                                                     AFD->getBodySourceRange());
      SN.NameRange = charSourceRangeFromSourceRange(SM,
                          AFD->getSignatureSourceRange());
+      SN.DocRange = findDocCommentRange(AFD->getAttrs());


can we have a function called setDecl(SyntaxStructureNode&,Decl*); where it sets both the decl pointer SN.Dcl and SN.DocRange? so all these added lines can call this function.

nkcsgexi · 2017-08-01T17:25:18Z

tools/SourceKit/lib/SwiftLang/SwiftEditor.cpp

+    }
+    else {
+      DocOffset = DocEnd = 0;
+    }


Initialize DocOffset and DocEnd with 0 so we don't need the else block.

nkcsgexi · 2017-08-01T17:27:15Z

lib/IDE/SyntaxModel.cpp

+    }
+  }
+  return CharSourceRange();
+}


We really don't need a member function here. A static function in the file will be sufficient.

marcelofabri · 2017-08-01T20:56:20Z

@nkcsgexi I've rebased and addressed your feedback 💯

nkcsgexi · 2017-08-01T20:58:26Z

lib/IDE/SyntaxModel.cpp

@@ -418,6 +418,21 @@ CharSourceRange parameterNameRangeOfCallArg(const TupleExpr *TE,
  return CharSourceRange();
 }

+CharSourceRange findDocCommentRange(DeclAttributes Attrs) {


can we declare this function as static explicitly?

nkcsgexi · 2017-08-01T20:59:22Z

lib/IDE/SyntaxModel.cpp

+  return CharSourceRange();
+}
+
+void setDecl(SyntaxStructureNode &N, Decl *D) {


Same here, declared as static void setDecl()

nkcsgexi · 2017-08-01T21:02:24Z

@swift-ci please smoke test and merge

nkcsgexi · 2017-08-01T23:40:39Z

@swift-ci please smoke test and merge

This PR adds the “key.docoffset” and “key.doclength” keys for declaration structures.

marcelofabri · 2017-08-02T07:10:43Z

I've rebased and (hopefully) addressed the CI failure.

CodaFi · 2017-08-02T07:16:13Z

@swift-ci please smoke test

rintaro · 2017-08-02T07:35:02Z

lib/IDE/SyntaxModel.cpp

+static CharSourceRange findDocCommentRange(DeclAttributes Attrs) {
+  for (auto Attr : Attrs) {
+    if (auto DocAttr = dyn_cast_or_null<RawDocCommentAttr>(Attr)) {
+      return DocAttr->getCommentRange();


RawDocCommentAttr::getCommentRange returns raw comment range. For example:

// Copyright (c) FooBar Inc. /// secret. func doSomething() {

IIRC, getCommentRange() returns from // Copy on line 1 to secret.\n on line 3.
Is that desired behavior?

Is that desired behavior?

Not really 😅

I've tested it and it indeed returns everything. Do you have any suggestions on how to improve this?

Well, interface printing uses Decl::getRawComment for extracting doc comments, but I'm not sure we can use that here. It might be too costly for this purpose.
@nkcsgexi ?

Good catch! @rintaro I cannot think of any reason not using Decl::getRawComment here, If we care only attached doc comment in the use case.

how do I get a range from a RawComment? 🤔

Could we add a function getCharSourceRange() to RawComment that analyses the enclosed SingleRawComment where range is available?

Something like this I guess:

RawComment Raw = ... if (!Raw.isEmpty()) { auto Start = Raw.Comments.front().Range.getStart(); auto End = Raw.Comments.back().Range.getEnd();

nkcsgexi

Good job on addressing comments! some more inline : )

nkcsgexi · 2017-08-02T19:11:16Z

lib/AST/RawComment.cpp

+
+  auto Start = this->Comments.front().Range.getStart();
+  auto End = this->Comments.back().Range.getEnd();
+  return CharSourceRange(SM, Start, End);


We actually don't need SM here, it's reasonable to assume Start and End are well formed. so something like:

return CharSourceRange(Start, (char*)End.getOpaquePointerValue() - (char*)Start.getOpaquePointerValue());

nkcsgexi · 2017-08-02T19:12:03Z

lib/AST/RawComment.cpp

+    return CharSourceRange();
+  }
+
+  auto Start = this->Comments.front().Range.getStart();


Could we have an early return if Start is invalid? cause the doc comment may come from a deserialized module file.

nkcsgexi · 2017-08-02T19:13:06Z

lib/IDE/SyntaxModel.cpp

@@ -418,6 +418,12 @@ CharSourceRange parameterNameRangeOfCallArg(const TupleExpr *TE,
  return CharSourceRange();
 }

+static void setDecl(SyntaxStructureNode &N, Decl *D, const SourceManager &SM) {


so, we don't need SM here either.

nkcsgexi · 2017-08-02T19:49:07Z

@swift-ci please smoke test

llvm::SourceMgr caches the line and column of the last query. It usually scans from that position for subsequent queries. However, if the query is for a position ahead of the last query, it re-scan from the top of the whole buffer. This significally slows down the performance. This change effectively mitigate performance regression introduced in swiftlang#11264 without functional changes.

…ent (#14733) llvm::SourceMgr caches the line and column of the last query. It usually scans from that position for subsequent queries. However, if the query is for a position ahead of the last query, it re-scan from the top of the whole buffer. This significally slows down the performance. This change effectively mitigate performance regression introduced in #11264 without functional changes.

marcelofabri mentioned this pull request Jul 31, 2017

file_header should ignore doc comments realm/SwiftLint#1719

Closed

nkcsgexi self-requested a review August 1, 2017 17:22

nkcsgexi reviewed Aug 1, 2017

View reviewed changes

nkcsgexi requested changes Aug 1, 2017

View reviewed changes

nkcsgexi reviewed Aug 1, 2017

View reviewed changes

marcelofabri force-pushed the doc-range-sourcekit branch from 89143ab to ade7a6a Compare August 1, 2017 20:55

nkcsgexi reviewed Aug 1, 2017

View reviewed changes

marcelofabri force-pushed the doc-range-sourcekit branch from ade7a6a to 7092db1 Compare August 1, 2017 21:00

nkcsgexi approved these changes Aug 1, 2017

View reviewed changes

[SourceKit] Add documentation range in structure (SR-2487)

8b4bc18

This PR adds the “key.docoffset” and “key.doclength” keys for declaration structures.

marcelofabri force-pushed the doc-range-sourcekit branch from 7092db1 to 8b4bc18 Compare August 2, 2017 06:17

rintaro reviewed Aug 2, 2017

View reviewed changes

Use Decl::getRawComment instead of RawDocCommentAttr

c61e0f3

nkcsgexi requested changes Aug 2, 2017

View reviewed changes

PR feedback

f3d49e1

nkcsgexi approved these changes Aug 2, 2017

View reviewed changes

nkcsgexi merged commit d16cce3 into swiftlang:master Aug 2, 2017

marcelofabri mentioned this pull request Aug 2, 2017

[81] Issue #81 - August 3, 2017 SwiftWeekly/swiftweekly.github.io#293

Closed

marcelofabri deleted the doc-range-sourcekit branch August 2, 2017 20:53

acecilia mentioned this pull request Nov 10, 2017

Rule request: bring back missing_docs realm/SwiftLint#1652

Closed

rintaro mentioned this pull request Feb 20, 2018

[AST] Retrive EndLine after StartLineAndColumn for SingleRawComment #14733

Merged

rintaro mentioned this pull request Feb 20, 2018

[4.1][AST] Retrive EndLine after StartLineAndColumn for SingleRawComment #14735

Merged

marcelofabri mentioned this pull request Jul 30, 2017

[SR-2487] sourcekitd doesn't include __raw_doc_comment in structure #45092

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SourceKit] Add documentation range in structure (SR-2487) #11264

[SourceKit] Add documentation range in structure (SR-2487) #11264

marcelofabri commented Jul 30, 2017

marcelofabri commented Aug 1, 2017

nkcsgexi Aug 1, 2017

nkcsgexi Aug 1, 2017

nkcsgexi Aug 1, 2017

marcelofabri commented Aug 1, 2017

nkcsgexi Aug 1, 2017

nkcsgexi Aug 1, 2017

nkcsgexi commented Aug 1, 2017

nkcsgexi commented Aug 1, 2017

marcelofabri commented Aug 2, 2017

CodaFi commented Aug 2, 2017

rintaro Aug 2, 2017

marcelofabri Aug 2, 2017

rintaro Aug 2, 2017

nkcsgexi Aug 2, 2017

marcelofabri Aug 2, 2017

nkcsgexi Aug 2, 2017

rintaro Aug 2, 2017 •

edited

Loading

marcelofabri Aug 2, 2017

nkcsgexi left a comment

nkcsgexi Aug 2, 2017

nkcsgexi Aug 2, 2017

nkcsgexi Aug 2, 2017

nkcsgexi commented Aug 2, 2017

[SourceKit] Add documentation range in structure (SR-2487) #11264

[SourceKit] Add documentation range in structure (SR-2487) #11264

Conversation

marcelofabri commented Jul 30, 2017

marcelofabri commented Aug 1, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marcelofabri commented Aug 1, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nkcsgexi commented Aug 1, 2017

nkcsgexi commented Aug 1, 2017

marcelofabri commented Aug 2, 2017

CodaFi commented Aug 2, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rintaro Aug 2, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nkcsgexi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nkcsgexi commented Aug 2, 2017

rintaro Aug 2, 2017 •

edited

Loading