[clangd][ObjC] Support nullability annotations

[dgoldman]

Summary

Nullability annotations are implmented using attributes; previusly
clangd would skip over AttributedTypeLoc since their location
points to the attribute instead of the modified type.

Also add quite a few test cases for this.

[dgoldman]

clang-tools-extra/clangd/Selection.cpp

625–627

Let me know if you think it would be better to return false here

[dgoldman]

Lint fixes

[dgoldman]

Fix merge

[sammccall]

clang-tools-extra/clangd/Selection.cpp

607–611

623

AttributeTypeLoc -> AttributedTypeLoc

This isn't true in general actually - attributes can be prefix or postfix (or other things...) and the heuristics in TypeLoc::get{Begin,End}Loc basically assume postfix for Attributed, which is right sometimes and wrong sometimes. (And getSourceRange() doesn't have enough information to get this right - it'd need the SourceManager to compare locations).

TypeLoc ranges in general seem pretty error prone. I've suggested a comment - we should consider just bailing out entirely here in future.

625–627

Yes, it's better to return false, which is more conservative. As written, this code will prevent targeting things within the attribute. (We can't currently target the attribute itself due to DynTypedNode limitations, but I'd like to fix that)

clang-tools-extra/clangd/unittests/HoverTests.cpp

2008

this is the same as the above case (apart from the selection changes, which are tested in SelectionTests)

same for the rest of the MYObject tests.
The ones after that are good as they're testing hovering on a different type of entity.

2052

these tests seem good (though unrelated to the rest of the patch, you might want to land them separately).

The Fooey test relies on selection of a protocol in a protocol-list, you may want to test this directly in SelectionTests.

[dgoldman]

Update with changes requested

• Looking into extract failure

[dgoldman]

With that change, somehow this extract test is now failing, not sure why or if it's intended behavior (seems like it isn't)

• TEST 'Clangd Unit Tests :: ./ClangdTests/ExtractFunctionTest.FunctionTest' FAILED ****

Note: Google Test filter = ExtractFunctionTest.FunctionTest
[==========] Running 1 test from 1 test case.
[----------] Global test environment set-up.
[----------] 1 test from ExtractFunctionTest
[ RUN ] ExtractFunctionTest.FunctionTest
/Users/davg/dev/github/llvm-project/clang-tools-extra/clangd/unittests/TweakTests.cpp:595: Failure
Value of: apply("int [[x = 0]];")
Expected: is equal to "unavailable"

Actual: "void extracted() {\nint x = 0;\n}\nvoid wrapperFunction(){\nextracted();\n}"

[ FAILED ] ExtractFunctionTest.FunctionTest (615 ms)
[----------] 1 test from ExtractFunctionTest (615 ms total)

[----------] Global test environment tear-down
[==========] 1 test from 1 test case ran. (615 ms total)
[ PASSED ] 0 tests.
[ FAILED ] 1 test, listed below:
[ FAILED ] ExtractFunctionTest.FunctionTest

Looks like the selection is off:

Before:

Built preamble of size 206188 for file /clangd-test/TestTU.cpp version null
TranslationUnitDecl

FunctionDecl void wrapperFunction()
  CompoundStmt { …
    DeclStmt int x = 0;
     .VarDecl int x = 0
       *IntegerLiteral 0

After:

Built preamble of size 206188 for file /clangd-test/TestTU.cpp version null
TranslationUnitDecl

FunctionDecl void wrapperFunction()
  CompoundStmt { …
    DeclStmt int x = 0;
     *VarDecl int x = 0
       *IntegerLiteral 0

[sammccall]

clang-tools-extra/clangd/Selection.cpp

631

I'm not sure we actually want to *do it* at this point, as you're seeing we may have some unintended consequences.

[dgoldman]

clang-tools-extra/clangd/Selection.cpp

631

Ah, I think I found the cause: claimRange will claim tokens for the TypeLoc even if the TypeLoc is unselected. So the VarDecl later on appears to be completely selected since the int token has been claimed and claimRange doesn't check the status of the children.

Before:

Built preamble of size 206192 for file /clangd-test/TestTU.cpp version null
Computing selection for </clangd-test/TestTU.cpp:1:6, col:11>
push: VarDecl

hit selection: </clangd-test/TestTU.cpp:1:6>
push: NestedNameSpecifierLoc 
pop: NestedNameSpecifierLoc  selected: 3
skip: BuiltinTypeLoc 
 skipped range = </clangd-test/TestTU.cpp:1:2>
push: IntegerLiteral 
 hit selection: </clangd-test/TestTU.cpp:1:10>
pop: IntegerLiteral  selected: 2
hit selection: </clangd-test/TestTU.cpp:1:2, col:10>

pop: VarDecl selected: 1
Built selection tree
TranslationUnitDecl

.VarDecl int x = 0
  *IntegerLiteral 0

After:

Built preamble of size 206192 for file /clangd-test/TestTU.cpp version null
Computing selection for </clangd-test/TestTU.cpp:1:6, col:11>
push: VarDecl

hit selection: </clangd-test/TestTU.cpp:1:6>
push: NestedNameSpecifierLoc
pop: NestedNameSpecifierLoc  selected: 3
push: BuiltinTypeLoc
pop: BuiltinTypeLoc  selected: 0
push: IntegerLiteral
 hit selection: </clangd-test/TestTU.cpp:1:10>
pop: IntegerLiteral  selected: 2
hit selection: </clangd-test/TestTU.cpp:1:2, col:10>

pop: VarDecl selected: 2
Built selection tree
TranslationUnitDecl

*VarDecl int x = 0
  *IntegerLiteral 0

Like you said probably worth fixing in a follow up, and I should revert to my previous code?

[sammccall]

Yep, let's revert to the previous state and land that, and I'll puzzle over the examples you give (because always returning false shouldn't affect behavior, just performance).

I have put together D89785 for more general attribute support, and it has a generalization of the fix here. (It returns false for any node with an attribute attached).
But it's worth landing this first as it has good tests for the objc cases, and that patch has its own prerequisites and risks of regressions.
(Not a timely coincidence, rather I got curious about the AST around Attrs after seeing this patch)

[dgoldman]

In D89579#2341513, @sammccall wrote:

Yep, let's revert to the previous state and land that, and I'll puzzle over the examples you give (because always returning false shouldn't affect behavior, just performance).

I have put together D89785 for more general attribute support, and it has a generalization of the fix here. (It returns false for any node with an attribute attached).
But it's worth landing this first as it has good tests for the objc cases, and that patch has its own prerequisites and risks of regressions.
(Not a timely coincidence, rather I got curious about the AST around Attrs after seeing this patch)

SGTM, the simplified cause is:

R"cpp( [[int ^x = $C[[0]]^]]; )cpp"

which you can add to TEST(SelectionTest, Selected) in SelectionTests.cpp.

With a proper attribute fix (well type loc) I think to fix this selection issue above we'll need to change claimRange to take children into affect.

[dgoldman]

Revert to previous AttributedTypeLoc behavior

clang-tools-extra/clangd/Selection.cpp

#endif
   // Here the first ("leftmost") sibling claims the tokens it wants, and the
   // other sibling gets what's left. So selecting "int" only includes the left
   // VarDecl in the selection tree.
 
   // An optimization for a common case: nodes outside macro expansions that
   // don't intersect the selection may be recursively skipped.
   bool canSafelySkipNode(const DynTypedNode &N) {
     SourceRange S = N.getSourceRange();
     if (auto *TL = N.get<TypeLoc>()) {
+      // FIXME: TypeLoc::getBeginLoc()/getEndLoc() are pretty fragile
+      // heuristics. We should consider only pruning critical TypeLoc nodes, to
+      // be more robust.
+
       // DeclTypeTypeLoc::getSourceRange() is incomplete, which would lead to
       // failing
       // to descend into the child expression.
       // decltype(2+2);
       // ~~~~~~~~~~~~~ <-- correct range
       // ~~~~~~~~      <-- range reported by getSourceRange()
       // ~~~~~~~~~~~~  <-- range with this hack(i.e, missing closing paren)
       // FIXME: Alter DecltypeTypeLoc to contain parentheses locations and get
       // rid of this patch.
       if (auto DT = TL->getAs<DecltypeTypeLoc>())
         S.setEnd(DT.getUnderlyingExpr()->getEndLoc());
+      // AttributedTypeLoc may point to the attribute's range, NOT the modified
+      // type's range.
+      if (auto AT = TL->getAs<AttributedTypeLoc>())
+        S = AT.getModifiedLoc().getSourceRange();
     }
     if (!SelChecker.mayHit(S)) {
       dlog("{1}skip: {0}", printNodeToString(N, PrintPolicy), indent());
       dlog("{1}skipped range = {0}", S.printToString(SM), indent(1));
       return true;
     }
     return false;
   }
 
   // There are certain nodes we want to treat as leaves in the SelectionTree,
   // although they do have children.
   bool shouldSkipChildren(const Stmt *X) const {
     // UserDefinedLiteral (e.g. 12_i) has two children (12 and _i).

clang-tools-extra/clangd/unittests/FindTargetTests.cpp

Code = R"cpp(
     void test(I *f) {
       [[f.x]].y = 0;
     }
   )cpp";
   EXPECT_DECLS("ObjCPropertyRefExpr",
                "@property(atomic, retain, readwrite) I *x");
 
   Code = R"cpp(
+    @interface MYObject
+    @end
+    @interface Interface
+    @property(retain) [[MYObject]] *x;
+    @end
+  )cpp";
+  EXPECT_DECLS("ObjCInterfaceTypeLoc", "@interface MYObject");
+
+  Code = R"cpp(
+    @interface MYObject2
+    @end
+    @interface Interface
+    @property(retain, nonnull) [[MYObject2]] *x;
+    @end
+  )cpp";
+  EXPECT_DECLS("ObjCInterfaceTypeLoc", "@interface MYObject2");
+
+  Code = R"cpp(
     @protocol Foo
     @end
     id test() {
       return [[@protocol(Foo)]];
     }
   )cpp";
   EXPECT_DECLS("ObjCProtocolExpr", "@protocol Foo");
 

clang-tools-extra/clangd/unittests/HoverTests.cpp

const std::function<void(HoverInfo &)> ExpectedBuilder;
           )cpp",
           [](HoverInfo &HI) {
             HI.Name = "data";
             HI.Type = "char";
             HI.Kind = index::SymbolKind::Field;
             HI.NamespaceScope = "ObjC::"; // FIXME: fix it
             HI.Definition = "char data";
           }},
+      {
+          R"cpp(
+          @interface MYObject
+          @end
+          @interface Interface
+          @property(retain) [[MYOb^ject]] *x;
+          @end
+          )cpp",
+          [](HoverInfo &HI) {
+            HI.Name = "MYObject";
+            HI.Kind = index::SymbolKind::Class;
+            HI.NamespaceScope = "";
+            HI.Definition = "@interface MYObject\n@end";
+          }},
+      {
+          R"cpp(
+          @interface MYObject
+          @end
+          @interface Interface
+          - (void)doWith:([[MYOb^ject]] *)object;
+          @end
+          )cpp",
+          [](HoverInfo &HI) {
+            HI.Name = "MYObject";
+            HI.Kind = index::SymbolKind::Class;
+            HI.NamespaceScope = "";
+            HI.Definition = "@interface MYObject\n@end";
+          }},
   };
 
   // Create a tiny index, so tests above can verify documentation is fetched.
   Symbol IndexSym = func("indexSymbol");
   IndexSym.Documentation = "comment from index";
   SymbolSlab::Builder Symbols;
   Symbols.insert(IndexSym);
   auto Index =
for (const auto &Case : Cases) {
     auto AST = TU.build();
 
     auto H = getHover(AST, T.point(), format::getLLVMStyle(), Index.get());
     ASSERT_TRUE(H);
     HoverInfo Expected;
     Expected.SymRange = T.range();
     Case.ExpectedBuilder(Expected);
 
     SCOPED_TRACE(H->present().asPlainText());
     EXPECT_EQ(H->NamespaceScope, Expected.NamespaceScope);
     EXPECT_EQ(H->LocalScope, Expected.LocalScope);
     EXPECT_EQ(H->Name, Expected.Name);
     EXPECT_EQ(H->Kind, Expected.Kind);
     EXPECT_EQ(H->Documentation, Expected.Documentation);
     EXPECT_EQ(H->Definition, Expected.Definition);
     EXPECT_EQ(H->Type, Expected.Type);
     EXPECT_EQ(H->ReturnType, Expected.ReturnType);

clang-tools-extra/clangd/unittests/SelectionTests.cpp

Case Cases[] = {
 
       {
           R"cpp(
         int a;
         decltype([[^a]] + a) b;
         )cpp",
           "DeclRefExpr"},
 
+      // Objective-C nullability attributes.
+      {
+          R"cpp(
+            @interface I{}
+            @property(nullable) [[^I]] *x;
+            @end
+          )cpp",
+          "ObjCInterfaceTypeLoc"},
+      {
+          R"cpp(
+            @interface I{}
+            - (void)doSomething:(nonnull [[i^d]])argument;
+            @end
+          )cpp",
+          "TypedefTypeLoc"},
+
       // Objective-C OpaqueValueExpr/PseudoObjectExpr has weird ASTs.
       // Need to traverse the contents of the OpaqueValueExpr to the POE,
       // and ensure we traverse only the syntactic form of the PseudoObjectExpr.
       {
           R"cpp(
             @interface I{}
             @property(retain) I*x;
             @property(retain) I*y;