Skip to content
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.

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

Make code completions and import suggestions work correctly for extensions with leading using clauses #11187

Conversation

prolativ
Copy link
Contributor

This is needed after #10940 got merged.

One disputable aspect might be whether an extension method should be suggested in code completion if it requires a given which is not available in scope.
Before the syntax change only trailing using clauses were allowed in extensions, e.g.

extension (i: Int)(using Foo)
  def xxx = ???

and the current behaviour was that for 123.x 123.xxx would be suggested even if there's no given Foo.
On the one hand not suggesting xxx would de-pollute the namespace and cause less confusion if there are multiple extension methods available which are applicable to anything that implements a type class, e.g.

extension [A](a: A)(using Show[A]) def show: String = ???

On the other hand it is still possible to call such methods if you provide the implicit explicitly.
So it seems to keep the current behaviour (to suggest xxx instead of hide it) as it is consistent with how code completion currently works with normal methods and methods introduced by implicit classes.

With leading using clauses, e.g.

extension (using Foo)(i: Int)
  def xxx = ???

the situation is slightly different. If a given instance of Foo is not in scope it cannot be passed to the method explicitly when one calls it like 123.xxx so the argument against not suggesting xxx doesn't apply anymore.
What's more the type of the receiver parameter might sometimes depend on the leading implicits.
So it makes sense not to suggest xxx when there's no given Foo. This also seems to be how completion works with implicit classes.

In case of import suggestions this currently seems to work analogically to code completions so I decided to keep this analogy (which also allowed some code reuse) even though sometimes the suggested import might not be enough to make an extension method work

@liufengyun liufengyun self-assigned this Jan 22, 2021
@@ -308,40 +308,140 @@ class CompletionTest {

@Test def completeExtensionMethodWithTypeParameter: Unit = {
code"""object Foo
|extension [A](foo: Foo.type) def xxxx: Int = 1
|extension (foo: Foo.type) def xxxx[A]: Int = 1
|object Main { Foo.xx${m1} }""".withSource
.completion(m1, Set(("xxxx", Method, "[A] => Int")))
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it an accidenal change? The same question for the change below.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is intentional. This is actually a new test case. The old one got copied below and renamed to completeExtensionMethodFromExtensionWithTypeParameter because now we need to distinguish between the two

ref.symbol.is(ExtensionMethod)
&& !receiver.isBottomType
&& isApplicableMethodRef(ref, receiver :: Nil, WildcardType)
private def tryApplyingReceiverToTruncatedExtMethod(methodSym: TermSymbol, receiver: Tree)(using Context): scala.util.Try[Tree] =
Copy link
Contributor

Choose a reason for hiding this comment

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

Usage of TermSymbol cannot handle prefix types, it's better to use TermRef.

The name of the method is long, but not informative. I'd suggest make it shorter and add documentation to make its semantics clear.

case meth: MethodType => meth.newLikeThis(meth.paramNames, meth.paramInfos, defn.AnyType)

val truncatedSym = methodSym.copy(owner = defn.RootPackage, name = Names.termName(""), info = truncateExtension(methodSym.info))
val truncatedRef = ref(truncatedSym).withSpan(Span(0, 0)) // Fake span needed to make this work in REPL
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe use the position from receiver?

@liufengyun liufengyun assigned prolativ and unassigned liufengyun Feb 2, 2021
@prolativ
Copy link
Contributor Author

@liufengyun reworks done.
However your remark about not relying on symbols because they might lose some type information forced me to introduce quite a lot of changes because the problem was already there unspotted before code completion for extension methods was added. On the other hand while working on this I found and fixed a few other issues, especially around how imported definitions were added to completions. I also added quite a lot of tests for that

@prolativ prolativ assigned liufengyun and unassigned prolativ Feb 11, 2021
@prolativ prolativ force-pushed the code-completions-with-leading-usings-in-extensions branch from 0c0c126 to 17a8b8f Compare February 11, 2021 19:17
@liufengyun
Copy link
Contributor

Nice job, I'll have a look today 👍

def truncateExtension(tp: Type)(using Context): Type = tp match
case poly: PolyType => poly.newLikeThis(poly.paramNames, poly.paramInfos, truncateExtension(poly.resType))
case meth: MethodType if meth.isContextualMethod => meth.newLikeThis(meth.paramNames, meth.paramInfos, truncateExtension(meth.resType))
case meth: MethodType => meth.newLikeThis(meth.paramNames, meth.paramInfos, defn.AnyType)
Copy link
Contributor

Choose a reason for hiding this comment

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

Nitpick: the lines are too long here (and in some other places), might be good to write the body of case in a new line.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

compiler/src/dotty/tools/dotc/typer/Applications.scala Outdated Show resolved Hide resolved

applyWithoutPostreceiverImplicits(methodRef, receiver)
.toOption
.map(tree => replaceCallee(tree, ref(methodRef)))
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd suggest merge tryApplyingReceiver and applyWithoutPostreceiverImplicits, maybe name it tryApplyExtensionMethod.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I kept them separate for performance as applyWithoutPostreceiverImplicits is also reused when searching for potential import suggestions and there we are only interested in whether it was a success, not in the transformed result. Also IMHO with tryApplyExtensionMethod it would not be less clear that the arguments following the receiver are not passed to the method

Copy link
Contributor

Choose a reason for hiding this comment

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

I kept them separate for performance as applyWithoutPostreceiverImplicits is also reused when searching for potential import suggestions and there we are only interested in whether it was a success, not in the transformed result.

I see, it makes sense to keep them separate, as long as the former is private, as applyWithoutPostreceiverImplicits is highly coupled with tryApplyingReceiver.

with tryApplyExtensionMethod it would not be less clear that the arguments following the receiver are not passed to the method

I don't see it's a concern, as the documentation serves as a specification of the behavior. The long name yyyWithoutXXX or yyyNoXXX only makes sense when we have a version named yyy. The long name does not help here as (1) there is no ambiguity, (2) it's harder for the reader, (3) it's still unclear what it means, as some implicits will be resolved.

On the other hand, tryApplyingExtensionMethod is more informative than tryApplyingReceiver. So I suggest:

  • applyWithoutPostreceiverImplicits -> tryApplyingExtensionMethod1, the return type to Option[Tree]
  • tryApplyingReceiver -> tryApplyingExtensionMethod

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If we then move .toOption to the body of tryApplyingExtensionMethod1 then the performance optimization is lost. If we don't care about it (don't we?) then keeping the method split doesn't make sense

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think this micro-optimization matters, having a more clear return type is more important. The current result type Try[Tree] provides more information than necessary, and can be safely replaced with Option[Tree]

Copy link
Contributor

Choose a reason for hiding this comment

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

BTW, the code can use try/catch to return Option[Tree] without the overhead of .toOption.


def isApplicableExtensionMethod(methodRef: TermRef, receiverType: Type)(using Context): Boolean =
methodRef.symbol.is(ExtensionMethod) && !receiverType.isBottomType &&
applyWithoutPostreceiverImplicits(methodRef, Typed(EmptyTree, TypeTree(receiverType))).isSuccess
Copy link
Contributor

Choose a reason for hiding this comment

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

What's wrong with isApplicableMethodRef in the original code?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

isApplicableMethodRef checks only direct application of arguments without any adjustments like inserting implicits before the supplied arguments list

Copy link
Contributor

@liufengyun liufengyun Feb 12, 2021

Choose a reason for hiding this comment

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

Thanks for the clarification. Then maybe use Typyer.extMethodApply directly on the extension method without any truncating?

Edit: yes, user may provide implicits explicitly.

@prolativ prolativ force-pushed the code-completions-with-leading-usings-in-extensions branch from 17a8b8f to ba382b9 Compare February 12, 2021 14:43
val types = imp.site.member(name.toTypeName).alternatives.map(denot => nameInScope.toTypeName -> denot)
CompletionScope.fromNamed(terms ++ types)

val givenImports = CompletionScope.fromNamed(imp.importedImplicits.map(x => (x.implicitName, x.underlyingRef.denot.asSingleDenotation)))
Copy link
Contributor

Choose a reason for hiding this comment

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

Nitpick: this line is too long. Introduce an intermediate variable.

case name: TermName if name.startsWith(matchingNamePrefix) => Some((denot, name))
case _ => None

types.flatMap{ tpe =>
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
types.flatMap{ tpe =>
types.flatMap { tpe =>

case (termRef, termName) =>
if termRef.symbol.is(ExtensionMethod) && !qual.tpe.isBottomType then
val applied = tryApplyingReceiver(termRef)
applied.map{ denot =>
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
applied.map{ denot =>
applied.map { denot =>

}

private def fromNamed(namedDenots: Seq[(Name, SingleDenotation)])(using Context): CompletionScope = {
val mappings = namedDenots.filter((name, den) => include(den.symbol, name)).toList.groupBy(_._1).map( (name, namedDens) => name.stripModuleClassSuffix -> namedDens.map(_._2))
Copy link
Contributor

Choose a reason for hiding this comment

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

Nitpick: this line is too long.


extension (scope: CompletionScope.type)
private def from(denots: Seq[SingleDenotation])(using Context): CompletionScope = {
val mappings = denots.filter(den => include(den.symbol, den.name)).toList.groupBy(_.name).map( (name, denots) => name.stripModuleClassSuffix -> denots)
Copy link
Contributor

Choose a reason for hiding this comment

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

Nitpick: this line is too long.

.collect {
case (name, (_, denots) :: Nil) => name -> denots
}
CompletionScope(mappings)
Copy link
Contributor

Choose a reason for hiding this comment

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

Add documentation for the intended semantics of the behavior.

@@ -386,6 +366,28 @@ object Completion {
!name.isConstructorName && name.toTermName.info.kind == SimpleNameKind
def isStable = true
}

extension (scope: CompletionScope)
private def withDenots(denotations: Seq[SingleDenotation], name: Name)(using Context): CompletionScope = {
Copy link
Contributor

Choose a reason for hiding this comment

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

Nitpick: I'd reverse the order of the two parameters.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This method is not used anywhere. I just forgot to remove it

}

extension (scope: CompletionScope.type)
private def from(denots: Seq[SingleDenotation])(using Context): CompletionScope = {
Copy link
Contributor

Choose a reason for hiding this comment

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

It constructs an instance of CompletionScope? Maybe rename to apply for easy understanding.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Then it would make sense to also rename fromNamed but they can't have the same name because of type erasure

val empty = CompletionScope()
}

private case class CompletionScope(nameToDenots: Map[Name, List[SingleDenotation]] = Map.empty) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Add documentation for the class CompletionScope.

Just curious, does it make sense to merge CompletionScope and CompletionBuffer?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

CompletionScope was supposed to be a lightweight wrapper over Map taking care of filtering out not matching completions and resolving conflicts between different denotations with the same name.
CompletionBuffer holds much more logic

Copy link
Contributor

Choose a reason for hiding this comment

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

What about keeps a mutable map inside CompletionBuffer, which is closer to what the name buffer suggests.

Copy link
Contributor

Choose a reason for hiding this comment

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

To clarify, I'm not proposing the change. I'm just trying to understand the design considerations here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The name comes from the times when it indeed was mutable but some corner cases seemed too hard to fix with mutability. Any idea for a better name?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not familiar with this part of code, so I'm asking questions to learn more about it.

I think the name is OK, I'm more interested in learning the relationships and responsibilities of the classes.

Creating a separate class with only one field looks dubious. However, in this case it may be justified if there is some special semantic operations defined around for this class.

* the same `Completion`.
*/
def getCompletions(using Context): List[Completion] = {
nameToDenots.toList.groupBy(_._1.toTermName.show).map { (name, namedDenots) =>
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we avoided the groupBy here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think so if we want to keep the current semantics although I'm still wondering of this is what we want for the future #11313 but changing this is not an intension of this PR

@prolativ
Copy link
Contributor Author

@liufengyun any more remarks or are we ready to merge this?

@liufengyun
Copy link
Contributor

@liufengyun any more remarks or are we ready to merge this?

I'll have another look soon.

* taking care of filtering out not matching completions
* and enabling easy resolving of conflicts between different denotations with the same name
*/
case class CompletionScope private(nameToDenots: Map[Name, List[SingleDenotation]] = Map.empty) {
Copy link
Contributor

Choose a reason for hiding this comment

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

The documentation does not say whether TermName and TypeName are merged or not.

.groupBy(_._1)
.collect {
case (name, (_, denots) :: Nil) => name -> denots
}
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems to me the code can be simplified with a fold or while-loop. The code above creates many intermediate objects and goes through the collection several times.

// Merge two scopes using mappings from `that` instead of from `this` in case of name clashes
def mergeShadowedBy(that: CompletionScope) = CompletionScope(this.nameToDenots ++ that.nameToDenots)

// Merge two scopes but discard mappings for names that appear in both scopes
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// Merge two scopes but discard mappings for names that appear in both scopes
/** Merge two scopes but discard mappings for names that appear in both scopes */

Will/should a TermName a considered the same as a TypeName a?

*/
case class CompletionScope private(nameToDenots: Map[Name, List[SingleDenotation]] = Map.empty) {

// Merge two scopes using mappings from `that` instead of from `this` in case of name clashes
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// Merge two scopes using mappings from `that` instead of from `this` in case of name clashes
/** Merge two scopes using mappings from `that` instead of from `this` in case of name clashes */

Will/should a TermName a considered the same as a TypeName a?

.filter((name, denot) => include(denot.symbol, name))
.toList
.groupBy(_._1)
.map((name, namedDenots) => name.stripModuleClassSuffix -> namedDenots.map(_._2))
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you please explain a little bit about namedDenots.map(_._2)? The code might be simpler with a simple loop or fold on namedDenotations .

val imported = grouped.map { (owner, contexts) =>
contexts.collect { case context if context.isImportContext =>
importedCompletions(using context)
}.foldLeft(CompletionScope.empty)(_.mergeDiscardingAmbiguities(_))
Copy link
Contributor

Choose a reason for hiding this comment

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

I still feel uneasy with the code inside scopeCompletions. There are too many chaining of high-order functions. A recursive method or while-loop will be much better in readability and performance.

Could you try to rewrite the code in a plain style to encode the algorithm here? For reference, in the compiler there are many places we use recursion and while-loops to deal with the contex:

https://github.com/lampepfl/dotty/blob/14927459330da0e26f591bd73d330fa6bb198674/compiler/src/dotty/tools/dotc/typer/ImportInfo.scala#L224-L225

* No implicit search is tried for implicits following the receiver or for parameters of the def (D, E).
*/
def tryApplyingExtensionMethod(methodRef: TermRef, receiver: Tree)(using Context): Option[Tree] =
// Drop all parameters sections of an extension method following the receiver; the return type after truncation is not important
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// Drop all parameters sections of an extension method following the receiver; the return type after truncation is not important
// Drop all parameters sections of an extension method following the receiver
// The return type after truncation is not important

case TypeApply(fun, args) => TypeApply(replaceCallee(fun, replacement), args)
case _: Ident => replacement

val truncatedSym = methodRef.symbol.asTerm.copy(owner = defn.RootPackage, name = Names.termName(""), info = truncateExtension(methodRef.info))
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
val truncatedSym = methodRef.symbol.asTerm.copy(owner = defn.RootPackage, name = Names.termName(""), info = truncateExtension(methodRef.info))
val truncatedSym = methodRef.symbol.asTerm.copy(info = truncateExtension(methodRef.info))

It seems the name and owner don't matter here.


def isApplicableExtensionMethod(methodRef: TermRef, receiverType: Type)(using Context): Boolean =
methodRef.symbol.is(ExtensionMethod) && !receiverType.isBottomType &&
tryApplyingExtensionMethod(methodRef, Typed(EmptyTree, TypeTree(receiverType))).nonEmpty
Copy link
Contributor

Choose a reason for hiding this comment

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

It's better to avoid EmptyTree inside expressions:

Suggested change
tryApplyingExtensionMethod(methodRef, Typed(EmptyTree, TypeTree(receiverType))).nonEmpty
tryApplyingExtensionMethod(methodRef, nullLiteral.asInstance(receiverType))).nonEmpty

@prolativ
Copy link
Contributor Author

@liufengyun it took me quite some time but I think I managed to significantly simplify the code in many places while also handling some additional corner cases.
ScopeCompletions class turned out to be both too heavy and not flexible enough for some cases so I switched to using ordinary Maps and performing the filtering exactly where it's needed.
If the implementation of scopeCompletions still seems confusing to you it might be simply because the logic there is complex by itself so I tried to at least document it well.

@liufengyun
Copy link
Contributor

@prolativ Thanks a lot for taking the effort to improve the PR. I'll review and get it merged soon.

liufengyun
liufengyun previously approved these changes Feb 22, 2021
Copy link
Contributor

@liufengyun liufengyun left a comment

Choose a reason for hiding this comment

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

LGTM, well done 🎉


/** Completions introduced by imports directly in this context.
* Completions from outer contexts are not included.
*/
private def importedCompletions(using Context): CompletionScope = {
private def importedCompletions(using Context): Map[Name, Seq[SingleDenotation]] = {
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe add a type alias type CompletionMap = Map[Name, Seq[SingleDenotation]]

*/
private case class ScopedDenotations(
denots: Seq[SingleDenotation],
scope: Scope,
Copy link
Contributor

Choose a reason for hiding this comment

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

For this field, maybe use Context -- usually we use context to represent context information.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The point is that multiple contexts can share the same scope e.g. when you do multiple imports one after another

import Foo.{xxx => zzz}
import Bar.{yyy => zzz}

and scope seems to be the only part of a context that seems to matter here for the distinction

Copy link
Contributor

Choose a reason for hiding this comment

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

I was not clear in my previous message. I meant to use ctx here and use ctx.scope in the comparison.

val typesFirst = denots.sortWith((d1, d2) => d1.isType && !d2.isType)
val desc = description(typesFirst)
Completion(name.show, desc, typesFirst.map(_.symbol))
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Indentation is incorrect hre for the block.

@liufengyun liufengyun merged commit ce1e2a2 into scala:master Feb 22, 2021
@liufengyun liufengyun deleted the code-completions-with-leading-usings-in-extensions branch February 22, 2021 15:48
@Kordyjan Kordyjan added this to the 3.0.0 milestone Aug 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants