This is an archive of the discontinued LLVM Phabricator instance.

[clangd][ObjC] Support nullability annotations
ClosedPublic

Authored by dgoldman on Oct 16 2020, 11:40 AM.

Details

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.

Diff Detail

Event Timeline

dgoldman created this revision.Oct 16 2020, 11:40 AM
dgoldman requested review of this revision.Oct 16 2020, 11:40 AM
dgoldman added inline comments.
clang-tools-extra/clangd/Selection.cpp
621–623

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

dgoldman updated this revision to Diff 298757.Oct 16 2020, 2:26 PM

Lint fixes

dgoldman updated this revision to Diff 298759.Oct 16 2020, 2:30 PM

Fix merge

sammccall added inline comments.Oct 19 2020, 5:04 AM
clang-tools-extra/clangd/Selection.cpp
607
619

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.

621–623

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 updated this revision to Diff 299038.Oct 19 2020, 6:41 AM
dgoldman marked 5 inline comments as done.

Update with changes requested

  • Looking into extract failure
dgoldman added a comment.EditedOct 19 2020, 6:45 AM

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 added inline comments.Oct 19 2020, 7:48 AM
clang-tools-extra/clangd/Selection.cpp
628

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

dgoldman added inline comments.Oct 19 2020, 9:01 AM
clang-tools-extra/clangd/Selection.cpp
628

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?

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 added a comment.EditedOct 20 2020, 7:10 AM

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 updated this revision to Diff 299356.Oct 20 2020, 7:38 AM

Revert to previous AttributedTypeLoc behavior

sammccall accepted this revision.Oct 20 2020, 8:31 AM
This revision is now accepted and ready to land.Oct 20 2020, 8:31 AM
This revision was automatically updated to reflect the committed changes.