improvement: Support completions for implicit classes #5904
Conversation
tgodzik
force-pushed
the
implicit-classes
branch
from
69fb9a1 to
b1058ae
Compare
| * artificial package object. Scala 2 doesn't allow for it. | ||
| */ | ||
| val isScala3Implicit = | ||
| dialect.allowExtensionMethods && currRegion.produceSourceToplevel && |
There was a problem hiding this comment.
I think you need to set the termOwner for current region or something to make sure we don't produce this artificial package object more than once. Though I'm not sure if that would cause any actual issues.
There was a problem hiding this comment.
I added a class to input to test it. Should be correct now.
| term(name, pos, Kind.OBJECT, 0) | ||
| } | ||
| owner | ||
| } else currRegion.owner |
There was a problem hiding this comment.
| } else currRegion.owner | |
| } else currRegion.termOwner |
I think that's the one you want here.
There was a problem hiding this comment.
Should be correct now, though I had to do a couple of changes, They're in the second to last commit.
| def isImplicitClass(owner: Symbol) = | ||
| val constructorParam = | ||
| owner.info.allMembers | ||
| .find(_.symbol.isAllOf(Flags.PrivateParamAccessor)) |
There was a problem hiding this comment.
| .find(_.symbol.isAllOf(Flags.PrivateParamAccessor)) | |
| .find(_.symbol.isAllOf(Flags.ParamAccessor)) |
It seems it doesn't have to be private and
implicit class A (val num: Int):
def incr: Int = num + 1 is legal.
There was a problem hiding this comment.
You're right, fixed and changed one of the tests.
| owner.info.allMembers | ||
| .find(_.name == searched) | ||
| .map(_.symbol) | ||
| .toList |
There was a problem hiding this comment.
Wouldn't owner.info.member(termName(value)).symbol achieve the same?
Also can you in the comment mark for which part it doesn't work? (I assume you mean implicit class A, since that's the type.)
There was a problem hiding this comment.
I agree, but maybe with typeName instead of termName?
There was a problem hiding this comment.
Thanks, changed and it works!
| @@ -100,7 +100,7 @@ trait PCSuite { | |||
| case NonFatal(e) => | |||
| println(s"warn: ${e.getMessage}") | |||
| } | |||
| workspace.inputs(filename) = (code2, dialect) | |||
| workspace.inputs(file.toURI.toString()) = (code2, dialect) | |||
There was a problem hiding this comment.
Pure curiosity, why did that change?
There was a problem hiding this comment.
That was actually showing errors whenever running tests, wasn't really problematic, but spamming the logs.
| @@ -21,7 +22,12 @@ class CompilerSearchVisitor( | |||
| val logger: Logger = Logger.getLogger(classOf[CompilerSearchVisitor].getName) | |||
|
|
|||
| private def isAccessible(sym: Symbol): Boolean = try | |||
| sym != NoSymbol && sym.isPublic && sym.isStatic | |||
| sym != NoSymbol && sym.isPublic && sym.isStatic || { | |||
| val owner = sym.maybeOwner | |||
There was a problem hiding this comment.
can we extract this part to seperate method like isAccessibleImplicit ?
There was a problem hiding this comment.
We can also check whether the symbol is accessible from site, by using the compiler's sym.isAccessibleFrom(qualType) at CompilerSearchVisitor creation site
There was a problem hiding this comment.
Good idea @rochala though at this point it would be a much bigger refactor, so I will just move it to a method.
| owner.info.allMembers | ||
| .find(_.name == searched) | ||
| .map(_.symbol) | ||
| .toList |
There was a problem hiding this comment.
I agree, but maybe with typeName instead of termName?
mtags/src/main/scala-3/scala/meta/internal/pc/completions/Completions.scala
Outdated
Show resolved
Hide resolved
tests/cross/src/test/scala/tests/pc/CompletionExtensionMethodSuite.scala
Outdated
Show resolved
Hide resolved
Previously, we would only automatically suggest extension methods, not implicit classes. Now, we also properly suggest implicit classes. We could follow up with support for Scala 2 also.
tgodzik
force-pushed
the
implicit-classes
branch
from
b1058ae to
e643cf5
Compare
tgodzik
force-pushed
the
implicit-classes
branch
3 times, most recently
from
af70623 to
15f8b3e
Compare
| @@ -225,6 +225,15 @@ abstract class BasePCSuite extends BaseSuite with PCSuite { | |||
| } | |||
|
|
|||
| object IgnoreScalaVersion { | |||
|
|
|||
| def or( | |||
There was a problem hiding this comment.
I added IgnoreScalaVersion#and at some point, which (counterintuitively) is the same. Maybe delete one of these.
There was a problem hiding this comment.
I removed the one I added, thanks!
| package example | ||
|
|
||
| // This wasn't possible in Scala 2 | ||
| implicit class Xtension/*example.Xtension().*//*example.Xtension#*/(number/*example.Xtension#number.*/: Int) { |
There was a problem hiding this comment.
I see it's been like this all along but why do we emit a method for implicit class?
There was a problem hiding this comment.
I haven't noticed it before, looks like this was done a long while ago 651a33f
And the comment is // emit symbol for implicit conversion, I guess this might no longer be needed 🤔
tgodzik
force-pushed
the
implicit-classes
branch
from
15f8b3e to
6684c12
Compare
Previously, we would only support extension methods and not the old style implicit classes. Now, those are supported also. Introduced in scalameta/metals#5904
Previously, we would only support extension methods and not the old style implicit classes. Now, those are supported also. Introduced in scalameta/metals#5904
Previously, we would only support extension methods and not the old style implicit classes. Now, those are supported also. Introduced in scalameta/metals#5904
Previously, we would only support extension methods and not the old style implicit classes. Now, those are supported also. Introduced in scalameta/metals#5904 [Cherry-picked 3c5e216]
Previously, we would only automatically suggest extension methods, not implicit classes.
Now, we also properly suggest implicit classes.
We could follow up with support for Scala 2 also.
Connected to #5893 <- I want to close it after also fixing Scala 2