improvement: Add separate option for decorations for evidence params #6219
Conversation
jkciesluk
force-pushed
the
ctx-bounds-dec
branch
2 times, most recently
from
4559f71
to
da88569
Compare
| val allImplicit = providedArgs.isEmpty || providedArgs.forall { | ||
| case Ident(name) => name == nme.MISSING | ||
| case _ => false | ||
| fun.typeOpt.widen.paramNamess.headOption.map {paramNames => |
There was a problem hiding this comment.
is there a need for widening ?
There was a problem hiding this comment.
Yes, without it GivenAlias test fails
given (using Xg): Yg with
def doY = "7"
val y = given_Yg<<(using Xg)>> // fun.typeOpt has no params, we need to widen
mtags/src/main/scala-3/scala/meta/internal/pc/PcInlayHintsProvider.scala
Show resolved
Hide resolved
mtags/src/main/scala-3/scala/meta/internal/pc/PcInlayHintsProvider.scala
Outdated
Show resolved
Hide resolved
| |object O { | ||
| | implicit val i: Int = 123 | ||
| | def test[T: Ordering](x: T)(y: T)/*: Nothing<<scala/Nothing#>>*/ = ??? | ||
| | test/*[Int<<scala/Int#>>]*/(1)(2)/*(using Int<<scala/math/Ordering.Int.>>)*/ |
There was a problem hiding this comment.
Ideally this should be
test/*[Int<<scala/Int#>>]*/(1)(2)/*(using Int<<scala/math/Ordering.Int.>>)*/test/*[Int<<scala/Int#>>]*/(1)(2)/*(using Ordering.Int<<scala/math/Ordering.Int.>>)*/But I guess this is not possible with current shortened type printer. I recommend creating an issue for this, so we don't forget to change it.
There was a problem hiding this comment.
Yes, it's connected to the printer, this PR only adds an option to decouple decorations for context bounds arguments from other implicit arguments. I will create an issue and link it here
jkciesluk
force-pushed
the
ctx-bounds-dec
branch
from
da88569
to
d3a6581
Compare
| @@ -260,6 +261,16 @@ object UserConfiguration { | |||
| |shown in the hover. | |||
| |""".stripMargin, | |||
| ), | |||
| UserConfigurationOption( | |||
There was a problem hiding this comment.
We could also do the same trick as with type, where we have minimal as an option. So for showImplicitArguments minimal would not show evidence, however with true it would
There was a problem hiding this comment.
I'm not really a fan of this solution in inferred types.
I would rather have a separate option for inferred type parameters, and use true/minimal for showing type annotations in bind:
val x = List(1) match
case head :: tail => 1
case _ => 0
// with 'minimal'
val x: Int = List(1) match
case head :: tail => 1
case _ => 0
// with 'true' (currently also with minimal)
val x: Int = List(1) match
case head: Int :: tail: List[Int] => 1
case _ => 0
There was a problem hiding this comment.
I agree with @jkciesluk, doing it this way makes it far more flexible. At some point we can consider better options for the user to configure those settings but that is more of a client problem.
| def unapply(tree: Tree): Option[(List[Symbol], Position, Boolean)] = | ||
| def unapply( | ||
| tree: Tree | ||
| ): Option[(List[(Symbol, Boolean)], Position, Boolean)] = |
There was a problem hiding this comment.
Maybe it'd be nice to have a dedicated class for (Symbol, Boolean), e.g.
sealed trait ImplicitParamSym
case class Param(sym: Symbol) extends ImplicitParamSym
case class ContextBound(sym: Symbol) extends ImplicitParamSym
just because Boolean in itself is quite meaningless. But it's up to you.
There was a problem hiding this comment.
Maybe:
sealed trait ImplicitParamSym
case class Param(sym: Symbol) extends AnyVal with ImplicitParamSym
case class ContextBound(sym: Symbol) extends AnyVal with ImplicitParamSym
There was a problem hiding this comment.
Maybe:
sealed trait ImplicitParamSym case class Param(sym: Symbol) extends AnyVal with ImplicitParamSym case class ContextBound(sym: Symbol) extends AnyVal with ImplicitParamSym
This doesn't compile, I used @kasiaMarek solution
There was a problem hiding this comment.
It does compile for me, what is an error you are getting. I would really use AnyVal in such cases
There was a problem hiding this comment.
Wait, no:
sealed trait ImplicitParamSym extends Any
case class Param(sym: Symbol) extends AnyVal with ImplicitParamSym
case class ContextBound(sym: Symbol) extends AnyVal with ImplicitParamSym
should work
| @@ -260,6 +261,16 @@ object UserConfiguration { | |||
| |shown in the hover. | |||
| |""".stripMargin, | |||
| ), | |||
| UserConfigurationOption( | |||
There was a problem hiding this comment.
I agree with @jkciesluk, doing it this way makes it far more flexible. At some point we can consider better options for the user to configure those settings but that is more of a client problem.
| """|When this option is enabled, each place where a context bound is used has it | ||
| |displayed either as additional decorations if they are supported by the editor or | ||
| |shown in the hover. | ||
| |""".stripMargin, |
There was a problem hiding this comment.
Might be good to include examples here and the client later
| def unapply(tree: Tree): Option[(List[Symbol], Position, Boolean)] = | ||
| def unapply( | ||
| tree: Tree | ||
| ): Option[(List[(Symbol, Boolean)], Position, Boolean)] = |
There was a problem hiding this comment.
Maybe:
sealed trait ImplicitParamSym
case class Param(sym: Symbol) extends AnyVal with ImplicitParamSym
case class ContextBound(sym: Symbol) extends AnyVal with ImplicitParamSym
d3a6581
to
07c82e5
Compare
jkciesluk
force-pushed
the
ctx-bounds-dec
branch
from
07c82e5
to
448ea81
Compare
| @@ -616,6 +616,7 @@ class Compilers( | |||
| userConfig().showInferredType.contains("true"), | |||
| implicitParameters = userConfig().showImplicitArguments, | |||
| implicitConversions = userConfig().showImplicitConversionsAndClasses, | |||
| contextBounds = userConfig().showEvidenceParams, | |||
There was a problem hiding this comment.
Let's move all the settings to a single object first.
No description provided.