[CodeCompletion] SR-943 'self' for dot expressions

[AnthonyLatsis]

May dot expressions finally have a 'self' completion.

Resolves SR-943.

[xedin]

@AnthonyLatsis Could you please add some tests as well? We have a bunch of examples in test/IDE and validation-test/IDE

[AnthonyLatsis]

@xedin Was about to ask where the tests are. Yes, sure.

[AnthonyLatsis]

What is the purpose of the -NOT, -DAG, -NEXT suffixes? Also, sometimes there is an end completions line, sometimes not, and it seems alright.

[rintaro]

As for -NOT et al, see https://llvm.org/docs/CommandGuide/FileCheck.html#the-check-next-directive.
As for End completions line, I think there's no good reason unless it's -NEXT.

[AnthonyLatsis]

@rintaro Thank you. Is this alright as well (Not written by me)? (Note '7 items')

// FOO_OPTIONAL_1: Begin completions, 7 items
// FOO_OPTIONAL_1-DAG: Decl[InstanceMethod]/CurrNominal/Erase[1]: ?.myFunction({#(foobar): Bar#})[#Void#]; name=myFunction(foobar: Bar)
// FOO_OPTIONAL_1: Keyword[self]/CurrNominal: self[#Foo<Bar>?#]; name=self
// FOO_OPTIONAL_1: End completions

[benlangmuir]

@AnthonyLatsis unless you really care that there are exactly 7 items, I would change that line to // FOO_OPTIONAL_1: Begin completions.

Also, the idea with DAG is that it lets you have two lines that can be reordered. In your example you probably want the Keyword line to also be DAG.

[benlangmuir]

This code path is also used for completing afterModuleName., where self is not correct. If you move this into getValueExprCompletions after the code that fixes ExprType, you can check whether ExprType is a module (see tryModuleCompletions for the check). There should also be a test for this.

Thanks for working on this!

[AnthonyLatsis]

Thanks for the hint. By the way, is it ok to show self completions after self or instances?

I'm asking because although this isn't wrong in terms of the language, it is either senseless in practice (self.self...) or something a user would very rarely do for some mysterious reason.

Also, the two CompletionKind that currently should show self and Type are TypeIdentifierWithDot and TypeIdentifierWithoutDot. I was a bit confused when I first discovered this. What cases do they cover?

[benlangmuir]

I'm asking because although this isn't wrong in terms of the language, it is either senseless in practice (self.self...) or something a user would very rarely do for some mysterious reason.

Showing it is fine.

Also, the two CompletionKind that currently should show self and Type are TypeIdentifierWithDot and TypeIdentifierWithoutDot. I was a bit confused when I first discovered this. What cases do they cover?

This is when we know syntactically that the LHS is a type name. These are not the only cases you would want to handle because they won't include typenames in expressions.

let a: Foo<TypeIdentifierWithoutDot >
let b: Foo.<TypeIdentifierWithDot >
_ = Foo.<Neither of the above>

[AnthonyLatsis]

I see. I noticed we don't have a Type completion for appropriate dot expressions as well.

If you move this into getValueExprCompletions after the code that fixes ExprType, you can check whether ExprType is a module (see tryModuleCompletions for the check). There should also be a test for this.

I'm not familiar enough with the compiler to understand what precisely happens in the "This is horrible" section, but it looks like it waits for generic types, so I suppose it doesn't affect modules whatsoever. That said and taking into account that getValueExprCompletions is invoked from several other paths apart from DotExpr, wouldn't it be better to keep everything where it is? Otherwise I would have to check if we're dealing with DotExpr and write specific logic in a more general function that shouldn't care about that:

https://github.com/apple/swift/blob/19d8dd43fb29228e424550e14ba39a12fc6283d2/lib/IDE/CodeCompletion.cpp#L3254-L3255

[benlangmuir]

Fair enough, it doesn't actually matter if you do the check before or after that "horrible" section. But you don't just want to handle DotExpr; for example, a postfix expression without a dot:

Foo <HERE>

Should still show self, but it should also add the leading ".". This can be done by calling addLeadingDot which will add the dot only if it's needed.

[AnthonyLatsis]

Handled those as well. I've tidied up the code but still haven't moved anything because otherwise I ended up touching paren expressions. I can change that, but there would have to be a way to check for completionKind in getValueExprCompletions.

[AnthonyLatsis]

@benlangmuir can you help to understand how results are ordered?

[AnthonyLatsis]

Don't worry about reviewing the test files too much – I simply added the self or .self completion anywhere it was expected.

[benlangmuir]

@swift-ci please test

[swift-ci]

Build failed
Swift Test Linux Platform
Git Sha - c88e1c9

[swift-ci]

Build failed
Swift Test OS X Platform
Git Sha - c88e1c9

[AnthonyLatsis]

Wierd...I just ran the tests on the failed files and they passed.

[AnthonyLatsis]

How are SourceKit tests related to code completion? Is SourceKit responsible for providing completions from other modules?

[benlangmuir]

SourceKit's completion is built on top of the code in lib/IDE.

[AnthonyLatsis]

Apologies, I did not suspect that I also needed to run sourcekitd-test. The tests naturally didn't fail without building it.

[AnthonyLatsis]

@benlangmuir could you trigger the tests please?

[xedin]

@swift-ci please test

[swift-ci]

Build failed
Swift Test OS X Platform
Git Sha - 7b41a41

[swift-ci]

Build failed
Swift Test Linux Platform
Git Sha - 7b41a41

[AnthonyLatsis]

In case you're not too busy, any nitpicks, @benlangmuir?

[benlangmuir]

Nope, looks good. Thanks!