subject:"\[PATCH\] D89579\: \[clangd\]\[ObjC\] Support nullability annotations"

[PATCH] D89579: [clangd][ObjC] Support nullability annotations

2020-10-20 Thread David Goldman via Phabricator via cfe-commits

This revision was automatically updated to reflect the committed changes.
Closed by commit rGd5c022d84699: [clangd][ObjC] Support nullability annotations 
(authored by dgoldman).

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D89579/new/

https://reviews.llvm.org/D89579

Files:
  clang-tools-extra/clangd/Selection.cpp
  clang-tools-extra/clangd/unittests/FindTargetTests.cpp
  clang-tools-extra/clangd/unittests/HoverTests.cpp
  clang-tools-extra/clangd/unittests/SelectionTests.cpp

Index: clang-tools-extra/clangd/unittests/SelectionTests.cpp
===
--- clang-tools-extra/clangd/unittests/SelectionTests.cpp
+++ clang-tools-extra/clangd/unittests/SelectionTests.cpp
@@ -356,6 +356,22 @@
 )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.
Index: clang-tools-extra/clangd/unittests/HoverTests.cpp
===
--- clang-tools-extra/clangd/unittests/HoverTests.cpp
+++ clang-tools-extra/clangd/unittests/HoverTests.cpp
@@ -1991,6 +1991,34 @@
 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.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.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.
Index: clang-tools-extra/clangd/unittests/FindTargetTests.cpp
===
--- clang-tools-extra/clangd/unittests/FindTargetTests.cpp
+++ clang-tools-extra/clangd/unittests/FindTargetTests.cpp
@@ -831,6 +831,24 @@
   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
Index: clang-tools-extra/clangd/Selection.cpp
===
--- clang-tools-extra/clangd/Selection.cpp
+++ clang-tools-extra/clangd/Selection.cpp
@@ -605,6 +605,10 @@
   bool canSafelySkipNode(const DynTypedNode ) {
 SourceRange S = N.getSourceRange();
 if (auto *TL = N.get()) {
+  // 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.
@@ -616,6 +620,10 @@
   // rid of this patch.
   if (auto DT = TL->getAs())
 S.setEnd(DT.getUnderlyingExpr()->getEndLoc());
+  // AttributedTypeLoc may point to the attribute's range, NOT the modified
+  // type's range.
+  if (auto AT = TL->getAs())
+S = AT.getModifiedLoc().getSourceRange();
 }
 if (!SelChecker.mayHit(S)) {
   dlog("{1}skip: {0}", printNodeToString(N, PrintPolicy), indent());
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D89579: [clangd][ObjC] Support nullability annotations

2020-10-20 Thread David Goldman via Phabricator via cfe-commits

dgoldman updated this revision to Diff 299356.
dgoldman added a comment.

Revert to previous AttributedTypeLoc behavior


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D89579/new/

https://reviews.llvm.org/D89579

Files:
  clang-tools-extra/clangd/Selection.cpp
  clang-tools-extra/clangd/unittests/FindTargetTests.cpp
  clang-tools-extra/clangd/unittests/HoverTests.cpp
  clang-tools-extra/clangd/unittests/SelectionTests.cpp

Index: clang-tools-extra/clangd/unittests/SelectionTests.cpp
===
--- clang-tools-extra/clangd/unittests/SelectionTests.cpp
+++ clang-tools-extra/clangd/unittests/SelectionTests.cpp
@@ -356,6 +356,22 @@
 )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.
Index: clang-tools-extra/clangd/unittests/HoverTests.cpp
===
--- clang-tools-extra/clangd/unittests/HoverTests.cpp
+++ clang-tools-extra/clangd/unittests/HoverTests.cpp
@@ -1991,6 +1991,34 @@
 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.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.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.
Index: clang-tools-extra/clangd/unittests/FindTargetTests.cpp
===
--- clang-tools-extra/clangd/unittests/FindTargetTests.cpp
+++ clang-tools-extra/clangd/unittests/FindTargetTests.cpp
@@ -820,6 +820,24 @@
   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
Index: clang-tools-extra/clangd/Selection.cpp
===
--- clang-tools-extra/clangd/Selection.cpp
+++ clang-tools-extra/clangd/Selection.cpp
@@ -605,6 +605,10 @@
   bool canSafelySkipNode(const DynTypedNode ) {
 SourceRange S = N.getSourceRange();
 if (auto *TL = N.get()) {
+  // 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.
@@ -616,6 +620,10 @@
   // rid of this patch.
   if (auto DT = TL->getAs())
 S.setEnd(DT.getUnderlyingExpr()->getEndLoc());
+  // AttributedTypeLoc may point to the attribute's range, NOT the modified
+  // type's range.
+  if (auto AT = TL->getAs())
+S = AT.getModifiedLoc().getSourceRange();
 }
 if (!SelChecker.mayHit(S)) {
   dlog("{1}skip: {0}", printNodeToString(N, PrintPolicy), indent());
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D89579: [clangd][ObjC] Support nullability annotations

2020-10-20 Thread David Goldman via Phabricator via cfe-commits

dgoldman added a comment.

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 I think to fix this selection issue above we'll 
need to change claimRange 
 
to take children into affect.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D89579/new/

https://reviews.llvm.org/D89579

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D89579: [clangd][ObjC] Support nullability annotations

2020-10-20 Thread Sam McCall via Phabricator via cfe-commits

sammccall added a comment.

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)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D89579/new/

https://reviews.llvm.org/D89579

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D89579: [clangd][ObjC] Support nullability annotations

2020-10-19 Thread David Goldman via Phabricator via cfe-commits

dgoldman added inline comments.



Comment at: clang-tools-extra/clangd/Selection.cpp:611
+  // robust.
+  return false;
 if (!SelChecker.mayHit(S)) {

sammccall wrote:
> I'm not sure we actually want to *do it* at this point, as you're seeing we 
> may have some unintended consequences.
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 
>  push: VarDecl 
>   hit selection: 
>   push: NestedNameSpecifierLoc 
>   pop: NestedNameSpecifierLoc  selected: 3
>   skip: BuiltinTypeLoc 
>skipped range = 
>   push: IntegerLiteral 
>hit selection: 
>   pop: IntegerLiteral  selected: 2
>   hit selection: 
>  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 
>  push: VarDecl
>   hit selection: 
>   push: NestedNameSpecifierLoc
>   pop: NestedNameSpecifierLoc  selected: 3
>   push: BuiltinTypeLoc
>   pop: BuiltinTypeLoc  selected: 0
>   push: IntegerLiteral
>hit selection: 
>   pop: IntegerLiteral  selected: 2
>   hit selection: 
>  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?




Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D89579/new/

https://reviews.llvm.org/D89579

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D89579: [clangd][ObjC] Support nullability annotations

2020-10-19 Thread Sam McCall via Phabricator via cfe-commits

sammccall added inline comments.



Comment at: clang-tools-extra/clangd/Selection.cpp:611
+  // robust.
+  return false;
 if (!SelChecker.mayHit(S)) {

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


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D89579/new/

https://reviews.llvm.org/D89579

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D89579: [clangd][ObjC] Support nullability annotations

2020-10-19 Thread David Goldman via Phabricator via cfe-commits

dgoldman added a comment.

With that change, somehow this extract test is now failing:

- 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


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D89579/new/

https://reviews.llvm.org/D89579

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D89579: [clangd][ObjC] Support nullability annotations

2020-10-19 Thread David Goldman via Phabricator via cfe-commits

dgoldman updated this revision to Diff 299038.
dgoldman marked 5 inline comments as done.
dgoldman added a comment.

Update with changes requested

- Looking into extract failure


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D89579/new/

https://reviews.llvm.org/D89579

Files:
  clang-tools-extra/clangd/Selection.cpp
  clang-tools-extra/clangd/unittests/FindTargetTests.cpp
  clang-tools-extra/clangd/unittests/HoverTests.cpp
  clang-tools-extra/clangd/unittests/SelectionTests.cpp

Index: clang-tools-extra/clangd/unittests/SelectionTests.cpp
===
--- clang-tools-extra/clangd/unittests/SelectionTests.cpp
+++ clang-tools-extra/clangd/unittests/SelectionTests.cpp
@@ -356,6 +356,22 @@
 )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.
Index: clang-tools-extra/clangd/unittests/HoverTests.cpp
===
--- clang-tools-extra/clangd/unittests/HoverTests.cpp
+++ clang-tools-extra/clangd/unittests/HoverTests.cpp
@@ -1991,6 +1991,34 @@
 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.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.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.
Index: clang-tools-extra/clangd/unittests/FindTargetTests.cpp
===
--- clang-tools-extra/clangd/unittests/FindTargetTests.cpp
+++ clang-tools-extra/clangd/unittests/FindTargetTests.cpp
@@ -820,6 +820,24 @@
   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
Index: clang-tools-extra/clangd/Selection.cpp
===
--- clang-tools-extra/clangd/Selection.cpp
+++ clang-tools-extra/clangd/Selection.cpp
@@ -604,19 +604,11 @@
   // don't intersect the selection may be recursively skipped.
   bool canSafelySkipNode(const DynTypedNode ) {
 SourceRange S = N.getSourceRange();
-if (auto *TL = N.get()) {
-  // 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())
-S.setEnd(DT.getUnderlyingExpr()->getEndLoc());
-}
+if (auto *TL = N.get())
+  // TypeLoc::getBeginLoc()/getEndLoc() are pretty fragile heuristics.
+  // We might consider only pruning critical TypeLoc nodes, to be more
+  // robust.
+  return false;
 if (!SelChecker.mayHit(S)) {
   dlog("{1}skip: {0}", printNodeToString(N, PrintPolicy), indent());
   dlog("{1}skipped range = {0}", S.printToString(SM), indent(1));

[PATCH] D89579: [clangd][ObjC] Support nullability annotations

2020-10-19 Thread Sam McCall via Phabricator via cfe-commits

sammccall added inline comments.



Comment at: clang-tools-extra/clangd/Selection.cpp:621-623
+  if (auto AT = TL->getAs())
+S = AT.getModifiedLoc()
+.getSourceRange(); // should we just return false?

dgoldman wrote:
> Let me know if you think it would be better to return false here
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)



Comment at: clang-tools-extra/clangd/Selection.cpp:607
 SourceRange S = N.getSourceRange();
 if (auto *TL = N.get()) {
   // DeclTypeTypeLoc::getSourceRange() is incomplete, which would lead to





Comment at: clang-tools-extra/clangd/Selection.cpp:619
 S.setEnd(DT.getUnderlyingExpr()->getEndLoc());
+  // AttributeTypeLoc points to the attribute's range, NOT the modified
+  // type's range.

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.



Comment at: clang-tools-extra/clangd/unittests/HoverTests.cpp:2008
+  }},
+  // Should work even with nullability attribute.
+  {

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.



Comment at: clang-tools-extra/clangd/unittests/HoverTests.cpp:2052
+  {
+  R"cpp(
+  @class ForwardDeclared;

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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D89579/new/

https://reviews.llvm.org/D89579

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D89579: [clangd][ObjC] Support nullability annotations

2020-10-16 Thread David Goldman via Phabricator via cfe-commits

dgoldman updated this revision to Diff 298759.
dgoldman added a comment.

Fix merge


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D89579/new/

https://reviews.llvm.org/D89579

Files:
  clang-tools-extra/clangd/Selection.cpp
  clang-tools-extra/clangd/unittests/FindTargetTests.cpp
  clang-tools-extra/clangd/unittests/HoverTests.cpp
  clang-tools-extra/clangd/unittests/SelectionTests.cpp

Index: clang-tools-extra/clangd/unittests/SelectionTests.cpp
===
--- clang-tools-extra/clangd/unittests/SelectionTests.cpp
+++ clang-tools-extra/clangd/unittests/SelectionTests.cpp
@@ -356,6 +356,22 @@
 )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.
Index: clang-tools-extra/clangd/unittests/HoverTests.cpp
===
--- clang-tools-extra/clangd/unittests/HoverTests.cpp
+++ clang-tools-extra/clangd/unittests/HoverTests.cpp
@@ -1991,6 +1991,91 @@
 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.Name = "MYObject";
+HI.Kind = index::SymbolKind::Class;
+HI.NamespaceScope = "";
+HI.Definition = "@interface MYObject\n@end";
+  }},
+  // Should work even with nullability attribute.
+  {
+  R"cpp(
+  @interface MYObject
+  @end
+  @interface Interface
+  @property(nonnull) [[MYOb^ject]] *x;
+  @end
+  )cpp",
+  [](HoverInfo ) {
+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.Name = "MYObject";
+HI.Kind = index::SymbolKind::Class;
+HI.NamespaceScope = "";
+HI.Definition = "@interface MYObject\n@end";
+  }},
+  {
+  R"cpp(
+  @interface MYObject
+  @end
+  @interface Interface
+  - (void)doWith:(nullable [[MYOb^ject]] *)object;
+  @end
+  )cpp",
+  [](HoverInfo ) {
+HI.Name = "MYObject";
+HI.Kind = index::SymbolKind::Class;
+HI.NamespaceScope = "";
+HI.Definition = "@interface MYObject\n@end";
+  }},
+  {
+  R"cpp(
+  @class ForwardDeclared;
+  @interface Interface
+  @property(retain) [[Forward^Declared]] *x;
+  @end
+  )cpp",
+  [](HoverInfo ) {
+HI.Name = "ForwardDeclared";
+HI.Kind = index::SymbolKind::Class;
+HI.NamespaceScope = "";
+HI.Definition = "@class ForwardDeclared;";
+  }},
+  {
+  R"cpp(
+  @protocol Fooey
+  - (void)foo;
+  @end
+  @interface Interface
+  @property(retain) id<[[Fo^oey]]> fooey;
+  @end
+  )cpp",
+  [](HoverInfo ) {
+HI.Name = "Fooey";
+HI.Kind = index::SymbolKind::Protocol;
+HI.NamespaceScope = "";
+HI.Definition = "@protocol Fooey\n@end";
+  }},
   };
 
   // Create a tiny index, so tests above can verify documentation is fetched.
Index: clang-tools-extra/clangd/unittests/FindTargetTests.cpp
===
--- clang-tools-extra/clangd/unittests/FindTargetTests.cpp
+++ clang-tools-extra/clangd/unittests/FindTargetTests.cpp
@@ -820,6 +820,24 @@
   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

[PATCH] D89579: [clangd][ObjC] Support nullability annotations

2020-10-16 Thread David Goldman via Phabricator via cfe-commits

dgoldman updated this revision to Diff 298757.
dgoldman added a comment.

Lint fixes


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D89579/new/

https://reviews.llvm.org/D89579

Files:
  clang-tools-extra/clangd/Selection.cpp
  clang-tools-extra/clangd/unittests/FindTargetTests.cpp
  clang-tools-extra/clangd/unittests/HoverTests.cpp


Index: clang-tools-extra/clangd/unittests/HoverTests.cpp
===
--- clang-tools-extra/clangd/unittests/HoverTests.cpp
+++ clang-tools-extra/clangd/unittests/HoverTests.cpp
@@ -2005,8 +2005,8 @@
 HI.NamespaceScope = "";
 HI.Definition = "@interface MYObject\n@end";
   }},
+  // Should work even with nullability attribute.
   {
-  // Should work even with nullability attribute.
   R"cpp(
   @interface MYObject
   @end
Index: clang-tools-extra/clangd/unittests/FindTargetTests.cpp
===
--- clang-tools-extra/clangd/unittests/FindTargetTests.cpp
+++ clang-tools-extra/clangd/unittests/FindTargetTests.cpp
@@ -827,8 +827,7 @@
 @property(retain) [[MYObject]] *x;
 @end
   )cpp";
-  EXPECT_DECLS("ObjCInterfaceTypeLoc",
-   "@interface MYObject");
+  EXPECT_DECLS("ObjCInterfaceTypeLoc", "@interface MYObject");
 
   Code = R"cpp(
 @interface MYObject2
@@ -837,8 +836,7 @@
 @property(retain, nonnull) [[MYObject2]] *x;
 @end
   )cpp";
-  EXPECT_DECLS("ObjCInterfaceTypeLoc",
-   "@interface MYObject2");
+  EXPECT_DECLS("ObjCInterfaceTypeLoc", "@interface MYObject2");
 
   Code = R"cpp(
 @protocol Foo
Index: clang-tools-extra/clangd/Selection.cpp
===
--- clang-tools-extra/clangd/Selection.cpp
+++ clang-tools-extra/clangd/Selection.cpp
@@ -619,7 +619,8 @@
   // AttributeTypeLoc points to the attribute's range, NOT the modified
   // type's range.
   if (auto AT = TL->getAs())
-S = AT.getModifiedLoc().getSourceRange();
+S = AT.getModifiedLoc()
+.getSourceRange(); // should we just return false?
 }
 if (!SelChecker.mayHit(S)) {
   dlog("{1}skip: {0}", printNodeToString(N, PrintPolicy), indent());


Index: clang-tools-extra/clangd/unittests/HoverTests.cpp
===
--- clang-tools-extra/clangd/unittests/HoverTests.cpp
+++ clang-tools-extra/clangd/unittests/HoverTests.cpp
@@ -2005,8 +2005,8 @@
 HI.NamespaceScope = "";
 HI.Definition = "@interface MYObject\n@end";
   }},
+  // Should work even with nullability attribute.
   {
-  // Should work even with nullability attribute.
   R"cpp(
   @interface MYObject
   @end
Index: clang-tools-extra/clangd/unittests/FindTargetTests.cpp
===
--- clang-tools-extra/clangd/unittests/FindTargetTests.cpp
+++ clang-tools-extra/clangd/unittests/FindTargetTests.cpp
@@ -827,8 +827,7 @@
 @property(retain) [[MYObject]] *x;
 @end
   )cpp";
-  EXPECT_DECLS("ObjCInterfaceTypeLoc",
-   "@interface MYObject");
+  EXPECT_DECLS("ObjCInterfaceTypeLoc", "@interface MYObject");
 
   Code = R"cpp(
 @interface MYObject2
@@ -837,8 +836,7 @@
 @property(retain, nonnull) [[MYObject2]] *x;
 @end
   )cpp";
-  EXPECT_DECLS("ObjCInterfaceTypeLoc",
-   "@interface MYObject2");
+  EXPECT_DECLS("ObjCInterfaceTypeLoc", "@interface MYObject2");
 
   Code = R"cpp(
 @protocol Foo
Index: clang-tools-extra/clangd/Selection.cpp
===
--- clang-tools-extra/clangd/Selection.cpp
+++ clang-tools-extra/clangd/Selection.cpp
@@ -619,7 +619,8 @@
   // AttributeTypeLoc points to the attribute's range, NOT the modified
   // type's range.
   if (auto AT = TL->getAs())
-S = AT.getModifiedLoc().getSourceRange();
+S = AT.getModifiedLoc()
+.getSourceRange(); // should we just return false?
 }
 if (!SelChecker.mayHit(S)) {
   dlog("{1}skip: {0}", printNodeToString(N, PrintPolicy), indent());
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D89579: [clangd][ObjC] Support nullability annotations

2020-10-16 Thread David Goldman via Phabricator via cfe-commits

dgoldman added inline comments.



Comment at: clang-tools-extra/clangd/Selection.cpp:621-623
+  if (auto AT = TL->getAs())
+S = AT.getModifiedLoc()
+.getSourceRange(); // should we just return false?

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


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D89579/new/

https://reviews.llvm.org/D89579

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D89579: [clangd][ObjC] Support nullability annotations

2020-10-16 Thread David Goldman via Phabricator via cfe-commits

dgoldman created this revision.
Herald added subscribers: cfe-commits, usaxena95, kadircet, arphaman.
Herald added a project: clang.
dgoldman requested review of this revision.
Herald added subscribers: MaskRay, ilya-biryukov.

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.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D89579

Files:
  clang-tools-extra/clangd/Selection.cpp
  clang-tools-extra/clangd/unittests/FindTargetTests.cpp
  clang-tools-extra/clangd/unittests/HoverTests.cpp
  clang-tools-extra/clangd/unittests/SelectionTests.cpp

Index: clang-tools-extra/clangd/unittests/SelectionTests.cpp
===
--- clang-tools-extra/clangd/unittests/SelectionTests.cpp
+++ clang-tools-extra/clangd/unittests/SelectionTests.cpp
@@ -356,6 +356,22 @@
 )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.
Index: clang-tools-extra/clangd/unittests/HoverTests.cpp
===
--- clang-tools-extra/clangd/unittests/HoverTests.cpp
+++ clang-tools-extra/clangd/unittests/HoverTests.cpp
@@ -1991,6 +1991,91 @@
 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.Name = "MYObject";
+HI.Kind = index::SymbolKind::Class;
+HI.NamespaceScope = "";
+HI.Definition = "@interface MYObject\n@end";
+  }},
+  {
+  // Should work even with nullability attribute.
+  R"cpp(
+  @interface MYObject
+  @end
+  @interface Interface
+  @property(nonnull) [[MYOb^ject]] *x;
+  @end
+  )cpp",
+  [](HoverInfo ) {
+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.Name = "MYObject";
+HI.Kind = index::SymbolKind::Class;
+HI.NamespaceScope = "";
+HI.Definition = "@interface MYObject\n@end";
+  }},
+  {
+  R"cpp(
+  @interface MYObject
+  @end
+  @interface Interface
+  - (void)doWith:(nullable [[MYOb^ject]] *)object;
+  @end
+  )cpp",
+  [](HoverInfo ) {
+HI.Name = "MYObject";
+HI.Kind = index::SymbolKind::Class;
+HI.NamespaceScope = "";
+HI.Definition = "@interface MYObject\n@end";
+  }},
+  {
+  R"cpp(
+  @class ForwardDeclared;
+  @interface Interface
+  @property(retain) [[Forward^Declared]] *x;
+  @end
+  )cpp",
+  [](HoverInfo ) {
+HI.Name = "ForwardDeclared";
+HI.Kind = index::SymbolKind::Class;
+HI.NamespaceScope = "";
+HI.Definition = "@class ForwardDeclared;";
+  }},
+  {
+  R"cpp(
+  @protocol Fooey
+  - (void)foo;
+  @end
+  @interface Interface
+  @property(retain) id<[[Fo^oey]]> fooey;
+  @end
+  )cpp",
+  [](HoverInfo ) {
+HI.Name = "Fooey";
+HI.Kind = index::SymbolKind::Protocol;
+HI.NamespaceScope = "";
+HI.Definition = "@protocol Fooey\n@end";
+  }},
   };
 
   // Create a tiny index, so tests above can verify documentation is fetched.
Index: clang-tools-extra/clangd/unittests/FindTargetTests.cpp
===
--- clang-tools-extra/clangd/unittests/FindTargetTests.cpp
+++ clang-tools-extra/clangd/unittests/FindTargetTests.cpp
@@ -820,6 +820,26 @@

[PATCH] D89579: [clangd][ObjC] Support nullability annotations

[PATCH] D89579: [clangd][ObjC] Support nullability annotations

[PATCH] D89579: [clangd][ObjC] Support nullability annotations

[PATCH] D89579: [clangd][ObjC] Support nullability annotations

[PATCH] D89579: [clangd][ObjC] Support nullability annotations

[PATCH] D89579: [clangd][ObjC] Support nullability annotations

[PATCH] D89579: [clangd][ObjC] Support nullability annotations

[PATCH] D89579: [clangd][ObjC] Support nullability annotations

[PATCH] D89579: [clangd][ObjC] Support nullability annotations

[PATCH] D89579: [clangd][ObjC] Support nullability annotations

[PATCH] D89579: [clangd][ObjC] Support nullability annotations

[PATCH] D89579: [clangd][ObjC] Support nullability annotations

[PATCH] D89579: [clangd][ObjC] Support nullability annotations

13 matches

Site Navigation

Mail list logo

Footer information