[IDE] Fix name range of wildcard declarations

[fwcd]

Visiting a wildcard variable declaration (e.g. let _ = 4) with SourceEntityWalker::walkToDeclPre(Decl *, CharSourceRange) currently yields a range of length zero, whereas e.g. let x = 4 yields a range of length 1.

The motivating use case for having a length-1-range here instead is to provide an accurate inlay type hint after the variable identifier, see swiftlang/sourcekit-lsp#408 (comment) for details

This PR therefore fixes the issue by defaulting to length 1 if the declaration doesn't have a name and an underscore occurs at the corresponding location in the source file.

cc @ahoppen

[ahoppen]

A quick preliminary comment inline. I will look at the PR in more detail tomorrow.

Also, I think it would be worth adding a test case for CollectVariableType for variables that are escaped by backticks.

[ahoppen]

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

[fwcd]

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.

[fwcd]

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

[ahoppen]

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).

[fwcd]

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.

[ahoppen]

Sleeping over it for a night, I think the fix is fine as-is. It would be good, though, to add test cases for invalid code containing variables without names like:

let = 2
let : Int = 4

And maybe some for closure parameters as well.

I don’t care too much whether we do show type annotations here, but would like to make sure that, if we are, we are correctly using an empty range for the variable’s name.

[ahoppen]

@swift-ci Please smoke test