[CodeCompletion][SR-7670] Duplicate generic param completions #16579
Conversation
fixed duplicate completions for generic parameters in non-generic members this also fixed the SemanticContextKind for some tests
lib/AST/LookupVisibleDecls.cpp
Outdated
| if (auto decl = DC->getAsDeclOrDeclExtensionContext()) { | ||
| if (auto GC = decl->getAsGenericContext()) { | ||
| auto params = GC->getGenericParams(); | ||
| namelookup::FindLocalVal(SM, Loc, Consumer).checkGenericParams(params); |
There was a problem hiding this comment.
This just inlines getGenericParamsOfContext() - but drops a check for being in an extension. Could you revert to just the call?
There was a problem hiding this comment.
The check is pointless since I'm retrieving only the parameters of the current context. What do you mean by reverting to just the call?
|
The explanation in the SR is correct but the fix being in NameLookup is suspicious. @benlangmuir Is there somewhere in code completion where generic parameters can be de-duplicated? |
|
I've dug up the problem and I am sure that the generic parameters of a nominal type are looked up as members. Because of the way |
|
Yes, the diagnosis is correct. But this change to name lookup doesn't seem so. If CodeCompletion is making multiple requests and receiving duplicate values because they share an ancestor DeclContext, then CodeCompletion needs to handle that, no? |
|
Edit I misunderstood. I see what you mean now. I don't really see though how duplicates can happen this way if we are looking up one concrete thing (the completions at the position of the cursor). And I don't think there is a de-duplicator – that would be expensive (code completion is already slow). Let's see what Ben says. |
|
Can you add a new test that fails without your change? |
|
@slavapestov Good idea. Give me an hour |
|
What is the difference between and ? |
|
|
|
@xedin So in other words it will ignore the name? |
|
@AnthonyLatsis Yes, that's right. |
|
@slavapestov Thank you for calling me to do the failing tests, I discovered the problem still persisted for subscripts and fixed that as well. |
|
Thanks! The change looks good to me, but @benlangmuir or @nkcsgexi should also take a look. |
|
@swift-ci Please smoke test |
|
It failed right away?
No problem, I'm all for it. |
|
@AnthonyLatsis yeah, there's some kind of problem with Linux CI right now. |
|
@swift-ci smoke test |
1 similar comment
|
@swift-ci smoke test |
|
Are we waiting for Ben to have a look? |
|
@benlangmuir any more comments on this PR? |
|
@nkcsgexi go ahead and merge if you're happy with this. If you think there's something here that I should review let me know. |
|
OK! 🚢 |
@xedin, do you know if it is possible to do the same trick with another part of the string? For example: I've tried with no success. |
|
@AnthonyLatsis You can do it anywhere in the string since it's just a regex |
6 participants
Fixed duplicate completions for generic parameters in non-generic members.
This also fixed the
SemanticContextKindfor some related tests.Resolves SR-7670.