[CodeCompletion] Fine grained prioritization #37563
Conversation
|
@swift-ci Please smoke test |
rintaro
force-pushed
the
ide-completion-flair
branch
from
71ba965 to
9641e1c
Compare
|
@swift-ci Please smoke test |
rintaro
force-pushed
the
ide-completion-flair
branch
from
9641e1c to
bc3447a
Compare
|
@swift-ci Please smoke test |
1 similar comment
|
@swift-ci Please smoke test |
include/swift/IDE/CodeCompletion.h
Outdated
| @@ -434,6 +408,19 @@ enum class SemanticContextKind { | |||
| OtherModule, | |||
| }; | |||
|
|
|||
| enum class CodeCompletionFlairBit: uint8_t { | |||
| /// **Deprecated**. Old style "catch all" prioritization. | |||
There was a problem hiding this comment.
Nitpick: it's usually spelled "catch-all".
| @@ -2180,9 +2191,6 @@ class CompletionLookup final : public swift::VisibleDeclConsumer { | |||
| return SemanticContextKind::Local; | |||
|
|
|||
| case DeclVisibilityKind::MemberOfCurrentNominal: | |||
| if (IsSuperRefExpr && | |||
| CurrentMethod && CurrentMethod->getOverriddenDecl() == D) | |||
| return SemanticContextKind::ExpressionSpecific; | |||
There was a problem hiding this comment.
Doesn't really impact this patch, but do you know why this check was on MemberOfCurrentNominal and not MemberOfSuper?
There was a problem hiding this comment.
This is for prioritizing the overridden method after super.
class Base {
func foo() {}
}
class Sub: Base {
override func foo() { super.#^HERE^# /* prioritize `foo()` */ }
}In this case, the base type for lookupVisibleMemberDecls is Base not Sub, so Base.foo() is found as MemberOfCurrentNominal of Base.
But I just noticed this mechanism doesn't work for:
class Base {
func foo() {}
}
class Sub: Base { }
class Sub2: Sub {
override func foo() { super.#^HERE^# }
}😅
There was a problem hiding this comment.
So my change unintentionally fixed this issue.
I added a few cases to test/IDE/complete_after_super.swift
| @@ -2778,7 +2786,8 @@ class CompletionLookup final : public swift::VisibleDeclConsumer { | |||
| if (ParentKind != StmtKind::If && ParentKind != StmtKind::Guard) | |||
| return; | |||
| CodeCompletionResultBuilder Builder(Sink, CodeCompletionResult::ResultKind::Keyword, | |||
| SemanticContextKind::ExpressionSpecific, expectedTypeContext); | |||
| SemanticContextKind::Local, expectedTypeContext); | |||
There was a problem hiding this comment.
Why Local? I would have expected None.
There was a problem hiding this comment.
No strong reason for Local or CurrentModule. But I couldn't just use None because None is rather deprioritized in CodeCompletionOrganizer. Maybe we need a new "neutral" semantic context kind.
There was a problem hiding this comment.
I'm fine with leaving it the way you wrote for the current PR, but I think we should leave a comment about it and we should probably figure out a better solution. Either change the scoring to be more specific so that None is okay for these cases, or else introduce new context(s).
| @@ -4575,13 +4599,14 @@ class CompletionLookup final : public swift::VisibleDeclConsumer { | |||
| continue; | |||
| CodeCompletionResultBuilder Builder( | |||
| Sink, CodeCompletionResult::ResultKind::Pattern, | |||
| SemanticContextKind::ExpressionSpecific, {}); | |||
| SemanticContextKind::Local, {}); | |||
There was a problem hiding this comment.
What's your reasoning for Local here? I wonder if we need a new semantic context for this, or maybe None.
| @@ -6185,12 +6210,14 @@ static void addPlatformConditions(CodeCompletionResultSink &Sink) { | |||
| consumer) { | |||
| CodeCompletionResultBuilder Builder( | |||
| Sink, CodeCompletionResult::ResultKind::Pattern, | |||
| SemanticContextKind::ExpressionSpecific, {}); | |||
| SemanticContextKind::CurrentModule, {}); | |||
There was a problem hiding this comment.
Similar question here about why this is CurrentModule.
| @@ -6231,7 +6258,8 @@ static void addConditionalCompilationFlags(ASTContext &Ctx, | |||
| // TODO: Should we filter out some flags? | |||
| CodeCompletionResultBuilder Builder( | |||
| Sink, CodeCompletionResult::ResultKind::Keyword, | |||
| SemanticContextKind::ExpressionSpecific, {}); | |||
| SemanticContextKind::CurrentModule, {}); | |||
There was a problem hiding this comment.
Similar question here about why this is CurrentModule.
| @@ -1002,11 +1007,11 @@ static void transformAndForwardResults( | |||
| auto topResults = filterInnerResults(results, options.addInnerResults, | |||
| options.addInnerOperators, hasDot, | |||
| hasQDot, hasInit, rules); | |||
| // FIXME: Overriding the default to context "None" is a hack so that they | |||
| // won't overwhelm other results that also match the filter text. | |||
| // FIXME: Clearling the flair (and semantic context) is a hack so that | |||
There was a problem hiding this comment.
Typo: clearling -> clearing
|
|
||
| if (Result->getFlair().contains(CodeCompletionFlairBit::ExpressionSpecific) || | ||
| Result->getFlair().contains(CodeCompletionFlairBit::SuperChain)) { | ||
| Info.SemanticContext = CCCtxExpressionSpecific; |
There was a problem hiding this comment.
Maybe worth a comment that this is to maintain compatibility?
To describe fine grained priorities. Introduce 'CodeCompletionFlair' that is a set of more descriptive flags for prioritizing completion items. This aims to replace ' SemanticContextKind::ExpressionSpecific' which was a "catch all" prioritization flag.
rintaro
force-pushed
the
ide-completion-flair
branch
from
bc3447a to
9f5756d
Compare
|
This is merged with #37725 |
Introduce
CodeCompletionFlairthat is a set of more descriptive flags for prioritizing completion items. This aims to replaceSemanticContextKind:: ExpressionSpecificwhich was a "catch all" flag. Also, useFlairto describe "is argument labels".