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

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

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

Conversation

jkciesluk
Copy link
Member

No description provided.

@jkciesluk jkciesluk force-pushed the ctx-bounds-dec branch 2 times, most recently from 4559f71 to da88569 Compare March 12, 2024 12:11
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

|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.>>)*/
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

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, 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

Copy link
Member Author

Choose a reason for hiding this comment

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

@@ -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.

def unapply(tree: Tree): Option[(List[Symbol], Position, Boolean)] =
def unapply(
tree: Tree
): Option[(List[(Symbol, Boolean)], Position, Boolean)] =
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe:

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

It does compile for me, what is an error you are getting. I would really use AnyVal in such cases

Copy link
Contributor

@tgodzik tgodzik Mar 15, 2024

Choose a reason for hiding this comment

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

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(
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.

"""|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

def unapply(tree: Tree): Option[(List[Symbol], Position, Boolean)] =
def unapply(
tree: Tree
): Option[(List[(Symbol, Boolean)], Position, Boolean)] =
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe:

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

@@ -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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants