Skip to content

[IDE] Fix name range of wildcard declarations #38085

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 4 commits into from
Jun 29, 2021
Merged

[IDE] Fix name range of wildcard declarations #38085

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
748f60d
Add wildcard parameter case to VariableType test
fwcd Jun 24, 2021
b2fb9e4
Fix name range of wildcard names in walkToDeclPre
fwcd Jun 24, 2021
88dd082
Add VariableType test case for backticked names
fwcd Jun 24, 2021
8a79268
Add VariableType test for unnamed variables
fwcd Jun 25, 2021
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
12 changes: 8 additions & 4 deletions lib/IDE/SourceEntityWalker.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -125,11 +125,15 @@ bool SemaAnnotator::walkToDeclPre(Decl *D) {
bool IsExtension = false;

if (auto *VD = dyn_cast<ValueDecl>(D)) {
if (VD->hasName() && !VD->isImplicit()) {
if (!VD->isImplicit()) {
SourceManager &SM = VD->getASTContext().SourceMgr;
NameLen = VD->getBaseName().userFacingName().size();
if (Loc.isValid() && SM.extractText({Loc, 1}) == "`")
NameLen += 2;
if (VD->hasName()) {
NameLen = VD->getBaseName().userFacingName().size();
if (Loc.isValid() && SM.extractText({Loc, 1}) == "`")
NameLen += 2;
} else if (Loc.isValid() && SM.extractText({Loc, 1}) == "_") {
Copy link
Member

Choose a reason for hiding this comment

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

I think this doesn’t work if the identifier starts with an underscore. E.g. let _foo = 2. If it does work, I think it would be worth adding a test case for it.

I also don’t understand yet why this should be necessary. If the identifier is empty, then DeclBaseName::userFacingName() should return a _, which has a length of 1. 🤔
https://github.com/apple/swift/blob/7ccc1b054a607881978b8efa0d7042d8bcd3d63f/include/swift/AST/Identifier.h#L339

Copy link
Member Author

Choose a reason for hiding this comment

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

If we have something like _x, hasName() returns true, so it should work. The reason why this is necessary for _ is because it doesn't go down that branch if the identifier is empty, because hasName() returns false.

Copy link
Member Author

Choose a reason for hiding this comment

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

Perhaps we could drop the hasName()-check completely since userFacingName() returns the right thing in any case?

Copy link
Member

Choose a reason for hiding this comment

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

One thing to also consider is invalid declarations like let = 1 or struct {} (not relevant for your implementation but for other users of SemaAnnotator), which also don’t have a name and whose name isn’t _ either.

Just an idea if the logic starts to get too complicated here: You could also check for the standard straightforward cases here (no backticks, name not empty) and if that doesn’t match, use Lexer::getTokenAtLocation to look the actual source code up. The downside is that Lexer::getTokenAtLocation needs to set up a Lexer instance, which is slower (although it’s not really slow either and I think we’re doing it in a variety of places).

Copy link
Member Author

Choose a reason for hiding this comment

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

Hm yes, dropping hasName() also seems to cause test/IDE/annotation.swift to fail as the annotator now reports things like

var <Var>protocolProperty1</Var>: <iStruct@>Int</iStruct> { <Accessor>g</Accessor>et }

instead of

var <Var>protocolProperty1</Var>: <iStruct@>Int</iStruct> { get }

I think the standard straightforward cases as well as the backtick and _ case should be handled correctly with the PR implementation right now, though using a lexer might be a bit cleaner than looking up the source code directly.

NameLen = 1;
}
}

auto ReportParamList = [&](ParameterList *PL) {
Expand Down
3 changes: 3 additions & 0 deletions test/SourceKit/VariableType/basic.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,8 @@ func foo() {
var local = 5
}

let `else` = 3

// RUN: %sourcekitd-test -req=collect-var-type %s -- %s | %FileCheck %s
// CHECK: (1:5, 1:6): Int (explicit type: 1)
// CHECK: (2:5, 2:6): String (explicit type: 0)
Expand All @@ -30,3 +32,4 @@ func foo() {
// CHECK: (14:7, 14:8): [Int : Int] (explicit type: 1)
// CHECK: (15:7, 15:8): (Int) -> Int (explicit type: 1)
// CHECK: (19:7, 19:12): Int (explicit type: 0)
// CHECK: (22:5, 22:11): Int (explicit type: 0)
4 changes: 4 additions & 0 deletions test/SourceKit/VariableType/params.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,9 +8,13 @@ let z = { (param: String) in
param.count
}

let w: (String, Int) -> Void = { (_, x) in }

// RUN: %sourcekitd-test -req=collect-var-type %s -- %s | %FileCheck %s
// CHECK: (1:10, 1:15): Int (explicit type: 1)
// CHECK: (5:5, 5:6): (String) -> Void (explicit type: 1)
// CHECK: (5:29, 5:34): String (explicit type: 0)
// CHECK: (7:5, 7:6): (String) -> Int (explicit type: 0)
// CHECK: (7:12, 7:17): String (explicit type: 1)
// CHECK: (11:35, 11:36): String (explicit type: 0)
// CHECK: (11:38, 11:39): Int (explicit type: 0)
10 changes: 10 additions & 0 deletions test/SourceKit/VariableType/unnamed.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
let = 2
let : Int = 4

let x: (String) -> Void = { (: String) in }

// RUN: %sourcekitd-test -req=collect-var-type %s -- %s | %FileCheck %s
// CHECK: <VariableTypes>
// CHECK-NEXT: (4:5, 4:6): (String) -> Void (explicit type: 1)
// CHECK-NEXT: </VariableTypes>