fix: [Scala 3] Add support for signature help in unapply method #3675
Conversation
| ctx: Context | ||
| ): (Int, Int, List[SingleDenotation]) = | ||
| path match | ||
| case (tpd.UnApply(fun, implicits, patterns)) :: _ => |
There was a problem hiding this comment.
This is the main fix, we apply almost the same logic as for Apply in Signatures
There was a problem hiding this comment.
Are we going to remove this pattern match once scala/scala3#14611 merged? In this case, can we wait for the next release that contains scala/scala3#14611 ?
Or leave this method for older Scala3 compilers? In this case, can we add a comment on why there's a kind of duplicated logic in metals and dotty?
There was a problem hiding this comment.
I think we should leave it as a fallback, but I still need to work a bit on this.
| val path = | ||
| Interactive | ||
| .pathTo(trees, pos)(using ctx) | ||
| .dropWhile { |
There was a problem hiding this comment.
This is another place that actually got changes.
tanishiking
left a comment
tanishiking
left a comment
There was a problem hiding this comment.
Metals part (SignatureHelpProvider.contribute) looks good to me :)
But I'm wondering maybe we should wait for the next release of Scala3 compiler 🤔
| .dropWhile { | ||
| case _: tpd.Apply => false | ||
| case unapply: tpd.UnApply => | ||
| // Don't show tuple unapply method |
There was a problem hiding this comment.
What happens if we show the tuple unapply method?
There was a problem hiding this comment.
It's just not really useful, since when writing tuples we are not really interested in signature help.
| ctx: Context | ||
| ): (Int, Int, List[SingleDenotation]) = | ||
| path match | ||
| case (tpd.UnApply(fun, implicits, patterns)) :: _ => |
There was a problem hiding this comment.
Are we going to remove this pattern match once scala/scala3#14611 merged? In this case, can we wait for the next release that contains scala/scala3#14611 ?
Or leave this method for older Scala3 compilers? In this case, can we add a comment on why there's a kind of duplicated logic in metals and dotty?
tgodzik
force-pushed
the
unapply-signature-help
branch
from
3c94fa1 to
4f518ea
Compare
tgodzik
force-pushed
the
unapply-signature-help
branch
from
4f518ea to
eb3bdcd
Compare
| |""".stripMargin, | ||
| compat = Map( | ||
| "3" -> | ||
| """|Two[T](a: T, b: T) |
There was a problem hiding this comment.
I reworked the unapply signature for case classes to show the actual case classes. It's a bit different than in case of Scala 2, but I think this is a bit better since def unapply(a: T, b: T): Two[T] does not exist. The alternative would be to show the parameters alone.
|
The failures are related to my tinkering in the compiler. |
tanishiking
left a comment
tanishiking
left a comment
There was a problem hiding this comment.
Diff seems good! And showing the actual case classes for unapply signature of case classes is a great idea 👍
I just left some questions (as it seems a bit tricky for me), it could be helpful if you can leave some code comments answering my questions. (those comments may help future us to comprehend the program around here :)
| documentation.foreach(info.setDocumentation(_)) | ||
| info | ||
|
|
||
| private def signatureToSignatureInformation( |
There was a problem hiding this comment.
[note] Basically there're no change In paramsToParameterInformation and signatureToSignatureInformation
|
|
||
| val funSymbol = fun.symbol | ||
| val retType = funSymbol.info.finalResultType | ||
| val isSomeMatch = funSymbol.owner.companionClass == defn.SomeClass |
There was a problem hiding this comment.
I assume this will be true for something like case Some(@@), am I right?
There was a problem hiding this comment.
Yeas, that's the special case I wanted to handle.
| val retType = funSymbol.info.finalResultType | ||
| val isSomeMatch = funSymbol.owner.companionClass == defn.SomeClass | ||
| val isExplicitUnapply = | ||
| !isSomeMatch && retType.typeSymbol == defn.OptionClass |
There was a problem hiding this comment.
isExplicitUnapply will be true when we explicitly call unapply method like Foo.unapply(@@)
I thought isExplicitUnapply will always true, for pattern match over case classes
case class Foo(a: Int)
x match {
// here I thought `fun` will be an `unapply` method of `Foo`
// whose `finalResultType` is Option[Foo]
// and it's always true for all case classes.
case Foo(@@)
}Against my assumption, is retType gonna be Foo (instead of Option[Foo])?
There was a problem hiding this comment.
isExplicitUnapply will be true when we explicitly call unapply method like Foo.unapply(@@)
It might be, but we will be at an Apply node, so the code will not be invoked.
There was a problem hiding this comment.
Auch, I got it! isExplicitUnapply = true means unapply method is explicitly defined, not explicitly called
| !isSomeMatch && retType.typeSymbol == defn.OptionClass | ||
| val paramIndex = | ||
| if isExplicitUnapply then implicitsBefore | ||
| else defaultParamIndex |
There was a problem hiding this comment.
Why we use implicitsBefore for Foo.unapply(@@) ?
There was a problem hiding this comment.
You can have in Scala 3 def unapply(using A)(value: Value) = ??? so we need to highlight the first parameter after the using params.
There was a problem hiding this comment.
I got it! Okay, so implicitsBefore means (the number of) "implicit parameters before" (actual parameter(?) of the constructor) 👍
tanishiking
left a comment
tanishiking
left a comment
There was a problem hiding this comment.
Again it's LGTM. This one is what I'd been longing for in Scala3. I look forward to using this feature!
| | } | ||
| |} | ||
| |""".stripMargin, | ||
| """|unapply(implicit x$1: String)(implicit x$2: Boolean)(a: Int): Option[(Int, Int)] |
There was a problem hiding this comment.
For scala 3 shouldn't implicit be called using?
|
Closing this for now, we might get help from the compiler team for this 🎉 |
4 participants
Fixes #1687
Related fix in Dotty: scala/scala3#14611
Most of the code is copied over from either the compiler or ScalaPResentationCompiler class.