Skip to content

Don't show enum completions in new keyword context #20304

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 1 commit into from
May 6, 2024
Merged

Don't show enum completions in new keyword context #20304

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
31 changes: 24 additions & 7 deletions compiler/src/dotty/tools/dotc/interactive/Completion.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,7 +75,7 @@ object Completion:
customMatcher: Option[Name => Boolean] = None
)(using Context): CompletionMap =
val adjustedPath = typeCheckExtensionConstructPath(untpdPath, tpdPath, pos)
computeCompletions(pos, mode, rawPrefix, adjustedPath, customMatcher)
computeCompletions(pos, mode, rawPrefix, adjustedPath, untpdPath, customMatcher)

/**
* Inspect `path` to determine what kinds of symbols should be considered.
Expand Down Expand Up @@ -199,12 +199,16 @@ object Completion:
.flatten.getOrElse(tpdPath)

private def computeCompletions(
pos: SourcePosition, mode: Mode, rawPrefix: String, adjustedPath: List[tpd.Tree], matches: Option[Name => Boolean]
pos: SourcePosition,
mode: Mode, rawPrefix: String,
adjustedPath: List[tpd.Tree],
untpdPath: List[untpd.Tree],
matches: Option[Name => Boolean]
)(using Context): CompletionMap =
val hasBackTick = rawPrefix.headOption.contains('`')
val prefix = if hasBackTick then rawPrefix.drop(1) else rawPrefix
val matches0 = matches.getOrElse(_.startsWith(prefix))
val completer = new Completer(mode, pos, matches0)
val completer = new Completer(mode, pos, untpdPath, matches0)

val result = adjustedPath match
// Ignore synthetic select from `This` because in code it was `Ident`
Expand Down Expand Up @@ -279,6 +283,12 @@ object Completion:
if denot.isType then denot.symbol.showFullName
else denot.info.widenTermRefExpr.show


def isInNewContext(untpdPath: List[untpd.Tree]): Boolean =
untpdPath match
case _ :: untpd.New(selectOrIdent: (untpd.Select | untpd.Ident)) :: _ => true
case _ => false

/** Include in completion sets only symbols that
* 1. is not absent (info is not NoType)
* 2. are not a primary constructor,
Expand All @@ -290,7 +300,11 @@ object Completion:
* 8. symbol is not a constructor proxy module when in type completion mode
* 9. have same term/type kind as name prefix given so far
*/
def isValidCompletionSymbol(sym: Symbol, completionMode: Mode)(using Context): Boolean =
def isValidCompletionSymbol(sym: Symbol, completionMode: Mode, isNew: Boolean)(using Context): Boolean =

lazy val isEnum = sym.is(Enum) ||
(sym.companionClass.exists && sym.companionClass.is(Enum))

sym.exists &&
!sym.isAbsent() &&
!sym.isPrimaryConstructor &&
Expand All @@ -300,6 +314,7 @@ object Completion:
!sym.isPackageObject &&
!sym.is(Artifact) &&
!(completionMode.is(Mode.Type) && sym.isAllOf(ConstructorProxyModule)) &&
!(isNew && isEnum) &&
(
(completionMode.is(Mode.Term) && (sym.isTerm || sym.is(ModuleClass))
|| (completionMode.is(Mode.Type) && (sym.isType || sym.isStableMember)))
Expand All @@ -323,7 +338,7 @@ object Completion:
* For the results of all `xyzCompletions` methods term names and type names are always treated as different keys in the same map
* and they never conflict with each other.
*/
class Completer(val mode: Mode, pos: SourcePosition, matches: Name => Boolean):
class Completer(val mode: Mode, pos: SourcePosition, untpdPath: List[untpd.Tree], matches: Name => Boolean):
/** Completions for terms and types that are currently in scope:
* the members of the current class, local definitions and the symbols that have been imported,
* recursively adding completions from outer scopes.
Expand Down Expand Up @@ -530,7 +545,7 @@ object Completion:
// There are four possible ways for an extension method to be applicable

// 1. The extension method is visible under a simple name, by being defined or inherited or imported in a scope enclosing the reference.
val termCompleter = new Completer(Mode.Term, pos, matches)
val termCompleter = new Completer(Mode.Term, pos, untpdPath, matches)
val extMethodsInScope = termCompleter.scopeCompletions.toList.flatMap:
case (name, denots) => denots.collect:
case d: SymDenotation if d.isTerm && d.termRef.symbol.is(Extension) => (d.termRef, name.asTermName)
Expand All @@ -557,14 +572,16 @@ object Completion:
}
extMethodsWithAppliedReceiver.groupByName

lazy val isNew: Boolean = isInNewContext(untpdPath)

/** Include in completion sets only symbols that
* 1. match the filter method,
* 2. satisfy [[Completion.isValidCompletionSymbol]]
*/
private def include(denot: SingleDenotation, nameInScope: Name)(using Context): Boolean =
matches(nameInScope) &&
completionsFilter(NoType, nameInScope) &&
isValidCompletionSymbol(denot.symbol, mode)
isValidCompletionSymbol(denot.symbol, mode, isNew)

private def extractRefinements(site: Type)(using Context): Seq[SingleDenotation] =
site match
Expand Down
19 changes: 19 additions & 0 deletions language-server/test/dotty/tools/languageserver/CompletionTest.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1704,4 +1704,23 @@ class CompletionTest {
.completion(m1, Set(
("getOrElse", Method, "[V1 >: String](key: Int, default: => V1): V1"),
))

@Test def noEnumCompletionInNewContext: Unit =
code"""|enum TestEnum:
| case TestCase
|object M:
| TestEnu$m1
| TestEnum.TestCa$m2
| val x: TestEnu$m3
| val y: TestEnum.Tes$m4
| new TestEnu$m5
| new TestEnum.TestCas$m6
|"""
.completion(m1, Set(("TestEnum", Module, "TestEnum")))
.completion(m2, Set(("TestCase", Field, "TestEnum")))
.completion(m3, Set(("TestEnum", Module, "TestEnum"), ("TestEnum", Class, "TestEnum")))
.completion(m4, Set(("TestCase", Field, "TestEnum")))
.completion(m5, Set())
.completion(m6, Set())

}
7 changes: 2 additions & 5 deletions presentation-compiler/src/main/dotty/tools/pc/completions/Completions.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,10 +70,7 @@ class Completions(
case _ :: (_: (Import | Export)) :: _ => false
case _ => true

private lazy val isNew: Boolean =
path match
case _ :: New(selectOrIdent: (Select | Ident)) :: _ => true
case _ => false
private lazy val isNew: Boolean = Completion.isInNewContext(adjustedPath)

def includeSymbol(sym: Symbol)(using Context): Boolean =
def hasSyntheticCursorSuffix: Boolean =
Expand Down Expand Up @@ -535,7 +532,7 @@ class Completions(
val query = completionPos.query
if completionMode.is(Mode.Scope) && query.nonEmpty then
val visitor = new CompilerSearchVisitor(sym =>
if Completion.isValidCompletionSymbol(sym, completionMode) &&
if Completion.isValidCompletionSymbol(sym, completionMode, isNew) &&
Copy link
Member

Choose a reason for hiding this comment

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

I feel like maybe it would be a bit more natural to pass untyped tree here instead of isNew, since isNew is always calculated via the same function and can be postponed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We would compute it on every isValidSymbol call, and it can be hundreds of calls, tho ? I did especially this way because we don't recompute this check

Copy link
Member

Choose a reason for hiding this comment

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

Oh, I see, makes sense.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is still duplication, as we're first filtering them in the compiler completions, and then again in Completions.scala from presentation compiler, but I think that this check is fast enough, that eliminating completions before searching for apply / constructors in withCompletionSuffix is completely worth it.

!(sym.is(Flags.ExtensionMethod) || (sym.maybeOwner.is(Flags.Implicit) && sym.maybeOwner.isClass))
then
indexedContext.lookupSym(sym) match
Expand Down
31 changes: 31 additions & 0 deletions presentation-compiler/test/dotty/tools/pc/tests/completion/CompletionSuite.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1877,3 +1877,34 @@ class CompletionSuite extends BaseCompletionSuite:
|""".stripMargin,
topLines = Some(2)
)

@Test def `no-enum-completions-in-new-context` =
check(
"""enum TestEnum:
| case TestCase
|object M:
| new TestEnu@@
|""".stripMargin,
""
)

@Test def `no-enum-case-completions-in-new-context` =
check(
"""enum TestEnum:
| case TestCase
|object M:
| new TestEnum.TestCas@@
|""".stripMargin,
""
)

@Test def `deduplicated-enum-completions` =
check(
"""enum TestEnum:
| case TestCase
|object M:
| val x: TestEn@@
|""".stripMargin,
"""TestEnum test
|""".stripMargin,
)
Loading