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.

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

Jump to bottom

improvement: Add separate option for decorations for evidence params #6219

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

improvement: Add separate option for decorations for evidence params #6219

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
1 change: 1 addition & 0 deletions metals-bench/src/main/scala/bench/InlayHintsBench.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -94,6 +94,7 @@ class InlayHintsBench extends PcBenchmark {
true,
true,
true,
true,
)
pc.inlayHints(pcParams).get().asScala.toList
}
Expand Down
1 change: 1 addition & 0 deletions metals/src/main/scala/scala/meta/internal/metals/Compilers.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -616,6 +616,7 @@ class Compilers(
userConfig().showInferredType.contains("true"),
implicitParameters = userConfig().showImplicitArguments,
implicitConversions = userConfig().showImplicitConversionsAndClasses,
contextBounds = userConfig().showEvidenceParams,
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's move all the settings to a single object first.

)

pc
Expand Down
14 changes: 14 additions & 0 deletions metals/src/main/scala/scala/meta/internal/metals/UserConfiguration.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@ case class UserConfiguration(
showInferredType: Option[String] = None,
showImplicitArguments: Boolean = false,
showImplicitConversionsAndClasses: Boolean = false,
showEvidenceParams: Boolean = false,
enableStripMarginOnTypeFormatting: Boolean = true,
enableIndentOnPaste: Boolean = false,
enableSemanticHighlighting: Boolean = true,
Expand Down Expand Up @@ -260,6 +261,16 @@ object UserConfiguration {
|shown in the hover.
|""".stripMargin,
),
UserConfigurationOption(
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

"show-evidence-params",
"false",
"false",
"Should display context bounds evidence parameters at usage sites",
"""|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,
Copy link
Contributor

Choose a reason for hiding this comment

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

Might be good to include examples here and the client later

),
UserConfigurationOption(
"enable-semantic-highlighting",
"true",
Expand Down Expand Up @@ -538,6 +549,8 @@ object UserConfiguration {
getBooleanKey("show-implicit-arguments").getOrElse(false)
val showImplicitConversionsAndClasses =
getBooleanKey("show-implicit-conversions-and-classes").getOrElse(false)
val showEvidenceParams =
getBooleanKey("show-evidence-params").getOrElse(false)
val enableStripMarginOnTypeFormatting =
getBooleanKey("enable-strip-margin-on-type-formatting").getOrElse(true)
val enableIndentOnPaste =
Expand Down Expand Up @@ -609,6 +622,7 @@ object UserConfiguration {
showInferredType,
showImplicitArguments,
showImplicitConversionsAndClasses,
showEvidenceParams,
enableStripMarginOnTypeFormatting,
enableIndentOnPaste,
enableSemanticHighlighting,
Expand Down
4 changes: 4 additions & 0 deletions mtags-interfaces/src/main/java/scala/meta/pc/InlayHintsParams.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,4 +29,8 @@ public interface InlayHintsParams extends RangeParams {
*/
boolean implicitConversions();

default boolean contextBounds() {
return false;
}

}
3 changes: 2 additions & 1 deletion mtags-shared/src/main/scala/scala/meta/internal/metals/CompilerInlayHintsParams.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,8 @@ case class CompilerInlayHintsParams(
inferredTypes: Boolean,
typeParameters: Boolean,
implicitParameters: Boolean,
implicitConversions: Boolean
implicitConversions: Boolean,
override val contextBounds: Boolean
) extends InlayHintsParams {
override def uri(): URI = rangeParams.uri
override def text(): String = rangeParams.text
Expand Down
71 changes: 48 additions & 23 deletions mtags/src/main/scala-2/scala/meta/internal/pc/PcInlayHintsProvider.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,17 +57,24 @@ final class PcInlayHintsProvider(
LabelPart(")") :: Nil,
InlayHintKind.Parameter
)
case ImplicitParameters(symbols, pos, allImplicit)
if params.implicitParameters() =>
val labelParts = symbols.map(s => List(labelPart(s, s.decodedName)))
val label =
if (allImplicit) labelParts.separated("(", ", ", ")")
else labelParts.separated(", ")
inlayHints.add(
adjustPos(pos).focusEnd.toLsp,
label,
InlayHintKind.Parameter
)
case ImplicitParameters(args, pos, allImplicit)
if params.implicitParameters() || params.contextBounds() =>
val symbols = args.collect {
case Param(sym) if params.implicitParameters() => sym
case ContextBound(sym) if params.contextBounds() => sym
}
if (symbols.isEmpty) inlayHints
else {
val labelParts = symbols.map(s => List(labelPart(s, s.decodedName)))
val label =
if (allImplicit) labelParts.separated("(", ", ", ")")
else labelParts.separated(", ")
inlayHints.add(
adjustPos(pos).focusEnd.toLsp,
label,
InlayHintKind.Parameter
)
}
case ValueOf(label, pos) if params.implicitParameters() =>
inlayHints.add(
adjustPos(pos).focusEnd.toLsp,
Expand Down Expand Up @@ -160,25 +167,43 @@ final class PcInlayHintsProvider(
fun.pos.isOffset && fun.symbol != null && fun.symbol.isImplicit
}
object ImplicitParameters {
def unapply(tree: Tree): Option[(List[Symbol], Position, Boolean)] =
def unapply(
tree: Tree
): Option[(List[ImplicitParamSym], Position, Boolean)] =
tree match {
case Apply(_, args)
case Apply(fun, args)
if args.exists(isSyntheticArg) && !tree.pos.isOffset =>
val (implicitArgs, providedArgs) = args.partition(isSyntheticArg)
val allImplicit = providedArgs.isEmpty
val pos = providedArgs.lastOption.fold(tree.pos)(_.pos)
Some(
implicitArgs.map(_.symbol),
pos,
allImplicit
)
fun.tpe.widen match {
case mt: MethodType if mt.params.nonEmpty =>
val (implicitArgs0, providedArgs) =
args.zip(mt.params).partition { case (arg, _) =>
isSyntheticArg(arg)
}
val implicitArgs = implicitArgs0.map(ImplicitParamSym(_))
val allImplicit = providedArgs.isEmpty
val pos = providedArgs.lastOption.fold(tree.pos)(_._1.pos)
Some((implicitArgs, pos, allImplicit))
case _ => None
}

case _ => None
}

private def isSyntheticArg(arg: Tree): Boolean =
arg.pos.isOffset && arg.symbol != null && arg.symbol.isImplicit
}

}
sealed trait ImplicitParamSym
case class Param(sym: Symbol) extends ImplicitParamSym
case class ContextBound(sym: Symbol) extends ImplicitParamSym
object ImplicitParamSym {
def apply(argWithName: (Tree, Symbol)): ImplicitParamSym = {
val (arg, name) = argWithName
if (isContextBoundParam(name)) ContextBound(arg.symbol)
else Param(arg.symbol)
}
private def isContextBoundParam(sym: Symbol) =
sym.name.toString.startsWith(nme.EVIDENCE_PARAM_PREFIX)
}
object ValueOf {
def unapply(tree: Tree): Option[(String, Position)] = tree match {
case Apply(ta: TypeApply, Apply(fun, _) :: _)
Expand Down
59 changes: 41 additions & 18 deletions mtags/src/main/scala-3/scala/meta/internal/pc/PcInlayHintsProvider.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ import dotty.tools.dotc.ast.tpd
import dotty.tools.dotc.ast.tpd.*
import dotty.tools.dotc.core.Contexts.Context
import dotty.tools.dotc.core.Flags
import dotty.tools.dotc.core.Names.Name
import dotty.tools.dotc.core.StdNames.*
import dotty.tools.dotc.core.Symbols.*
import dotty.tools.dotc.core.Types.*
Expand Down Expand Up @@ -73,17 +74,23 @@ class PcInlayHintsProvider(
LabelPart(")") :: Nil,
InlayHintKind.Parameter,
)
case ImplicitParameters(symbols, pos, allImplicit)
if params.implicitParameters() =>
val labelParts = symbols.map(s => List(labelPart(s, s.decodedName)))
val label =
if allImplicit then labelParts.separated("(using ", ", ", ")")
else labelParts.separated(", ")
inlayHints.add(
adjustPos(pos).toLsp,
label,
InlayHintKind.Parameter,
)
case ImplicitParameters(args, pos, allImplicit)
if params.implicitParameters() || params.contextBounds() =>
val symbols = args.collect {
case Param(sym) if params.implicitParameters() => sym
case ContextBound(sym) if params.contextBounds() => sym
}
if symbols.isEmpty then inlayHints
else
val labelParts = symbols.map(s => List(labelPart(s, s.decodedName)))
val label =
if allImplicit then labelParts.separated("(using ", ", ", ")")
else labelParts.separated(", ")
inlayHints.add(
adjustPos(pos).toLsp,
label,
InlayHintKind.Parameter,
)
case ValueOf(label, pos) if params.implicitParameters() =>
inlayHints.add(
adjustPos(pos).toLsp,
Expand Down Expand Up @@ -210,17 +217,20 @@ object ImplicitConversion:
end ImplicitConversion

object ImplicitParameters:
def unapply(tree: Tree)(using Context) =
def unapply(tree: Tree)(using Context): Option[(List[ImplicitParamSym], SourcePosition, Boolean)] =
tree match
jkciesluk marked this conversation as resolved.
Show resolved Hide resolved
case Apply(fun, args)
if args.exists(isSyntheticArg) && !tree.sourcePos.span.isZeroExtent =>
val (implicitArgs, providedArgs) = args.partition(isSyntheticArg)
val allImplicit = providedArgs.isEmpty || providedArgs.forall {
case Ident(name) => name == nme.MISSING
case _ => false
fun.typeOpt.widen.paramNamess.headOption.map {paramNames =>
Copy link
Collaborator

Choose a reason for hiding this comment

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

is there a need for widening ?

Copy link
Member Author

Choose a reason for hiding this comment

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

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

val (implicitArgs0, providedArgs) = args.zip(paramNames).partition((arg, _) => isSyntheticArg(arg))
val firstImplicitPos = implicitArgs0.head._1.sourcePos
val implicitArgs = implicitArgs0.map(ImplicitParamSym(_))
val allImplicit = providedArgs.isEmpty || providedArgs.forall {
case (Ident(name), _) => name == nme.MISSING
case _ => false
}
(implicitArgs, firstImplicitPos, allImplicit)
}
val pos = implicitArgs.head.sourcePos
Some(implicitArgs.map(_.symbol), pos, allImplicit)
case _ => None

private def isSyntheticArg(tree: Tree)(using Context) = tree match
Expand All @@ -229,6 +239,19 @@ object ImplicitParameters:
case _ => false
end ImplicitParameters

sealed trait ImplicitParamSym extends Any
case class Param(sym: Symbol) extends AnyVal with ImplicitParamSym
case class ContextBound(sym: Symbol) extends AnyVal with ImplicitParamSym

object ImplicitParamSym:
def apply(argWithName: (Tree, Name))(using Context) =
val (arg, name) = argWithName
if isContextBoundParam(name) then ContextBound(arg.symbol)
else Param(arg.symbol)
private def isContextBoundParam(name: Name) =
// In 3.1.3 NameKinds.ContextBoundParamName.separator is not available
name.toString.startsWith("evidence$")

object ValueOf:
def unapply(tree: Tree)(using Context) =
tree match
Expand Down
16 changes: 11 additions & 5 deletions tests/cross/src/main/scala/tests/BaseInlayHintsSuite.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,12 @@ class BaseInlayHintsSuite extends BasePCSuite {
name: TestOptions,
base: String,
expected: String,
compat: Map[String, String] = Map.empty
compat: Map[String, String] = Map.empty,
showInferredType: Boolean = true,
showTypeArguments: Boolean = true,
showImplicitArguments: Boolean = true,
showImplicitConversions: Boolean = true,
showContextBounds: Boolean = true
)(implicit location: Location): Unit =
test(name) {
def pkgWrap(text: String) =
Expand All @@ -31,10 +36,11 @@ class BaseInlayHintsSuite extends BasePCSuite {
)
val pcParams = CompilerInlayHintsParams(
rangeParams,
true,
true,
true,
true
showInferredType,
showTypeArguments,
showImplicitArguments,
showImplicitConversions,
showContextBounds
)

val inlayHints = presentationCompiler
Expand Down
Loading
Loading