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

Cannot define a path-dependent Conversion instance #7412

Open
neko-kai opened this issue Oct 12, 2019 · 20 comments
Open

Cannot define a path-dependent Conversion instance #7412

neko-kai opened this issue Oct 12, 2019 · 20 comments
Labels
backlog No work planned on this by the core team for the time being. itype:language enhancement

Comments

@neko-kai
Copy link
Contributor

minimized code

class Named[S <: String with Singleton](s: S)

object Named {
  given Conversion[(s: String), Named[s.type]] {
    def apply(s: String): Named[s.type] = new Named(s)
  }

  // this is ok:
  // implicit def convert(s: String): Named[s.type] = new Named[s.type](s)

  "abc": Named["abc"]
}

expectation

Expected being able to define an instance of Conversion equivalent to implicit def convert(s: String): Named[s.type]. However the definition above fails to parse!

[error] example.scala:22:20: an identifier expected, but '(' found
[error]   given Conversion[(s: String), Named[s.type]] {
[error]                    ^
[error] example.scala:28:3: Found:    String("abc")
[error] Required: Named[String("abc")]
[error]   "abc": Named["abc"]
[error]   ^
[error] two errors found
@ashwinbhaskar
Copy link
Collaborator

can I pick this up?

@ashwinbhaskar
Copy link
Collaborator

@neko-kai can you assign this to me?

@neko-kai
Copy link
Contributor Author

neko-kai commented Nov 5, 2019

@ashwinbhaskar I can't, i'm not a dotty developer and don't have any permissions. Maybe @smarter could? (sorry for bothering)

@bishabosha
Copy link
Member

@ashwinbhaskar I assigned you

@ashwinbhaskar
Copy link
Collaborator

@neko-kai @bishabosha thank you:)

@smarter
Copy link
Member

smarter commented Nov 6, 2019

Conversion[(s: String), Named[s.type]]

This syntax does not make sense in general (what happens if the second type parameter appears in a context where s is not bound ?) so we can't support it just for Conversion. What we could possibly support is something like:

  given Conversion[(s: String), Named[_]] {
    def apply(s: String): Named[s.type] = new Named(s)
  }

But that would also require some special-casing of Conversion. Since we want to discourage people from using conversions, this might not be worth it. If it is, then a different syntax (like given conversion (s: String): Named[s.Type] = ... ?) might be more appropriate, but that's a tall order at this point...

@neko-kai
Copy link
Contributor Author

neko-kai commented Nov 6, 2019

@smarter

This syntax does not make sense in general (what happens if the second type parameter appears in a context where s is not bound ?) so we can't support it just for Conversion

IMHO it can conceivably make sense for all subtypes of Function* where these type parameters appear in appropriate positions with appropriate variance.

But that would also require some special-casing of Conversion.

AFAIU the special case would appear only in Implicits.scala? (i.e. use refined apply signature for search & result, not type parameters) That seems completely fine to me, Conversion is already special-cased there.

Since we want to discourage people from using conversions, this might not be worth it

shapeless, akka, singleton-ops, spire etc. afaik use dependent conversions. An even more absolutely massive amount of libraries use implicit def macro conversions. IMHO this will come up again and it will be easier to support earlier before the codebase becomes more mature. Also scalafix rules to rewrite conversions cannot be made comprehensive until Conversion reaches parity with implicit def.

@smarter
Copy link
Member

smarter commented Nov 6, 2019

AFAIU the special case would appear only in Implicits.scala? (i.e. use refined apply signature for search & result, not type parameters) That seems completely fine to me, Conversion is already special-cased there.

Yes, it's not too bad.

shapeless, akka, singleton-ops, spire etc. afaik use dependent conversions.

Good point, could you link to some examples ?

@neko-kai
Copy link
Contributor Author

neko-kai commented Nov 6, 2019

@smarter

Off the top:

shapeless:

sbt:

My company uses both dependent and macro conversions in open-source and closed-source as well.

I can find more usages if you ask again for more proofs of usage.

@julienrf
Copy link
Collaborator

julienrf commented Nov 6, 2019

This syntax does not make sense in general (what happens if the second type parameter appears in a context where s is not bound ?) so we can't support it just for Conversion.

If the second type parameter appears in a context where s is not bound, then this is a compilation error.

But I think this syntax does make sense, in general. We already have it for Function1. Indeed, the following type-expression is valid: (a: A) => a.B, and conceptually it desugars to Function1[(a: A), a.B].

@ashwinbhaskar
Copy link
Collaborator

@julienrf The syntax does not work for Function1 as well. This gives an error.

trait Foo
    type Baz
    val baz: Baz

val bazExtractor: Function1[(f: Foo), f.baz] = ???

Did you mean that if it is made to work for Function1 then it would work for Conversion as well?

@julienrf
Copy link
Collaborator

julienrf commented Nov 7, 2019

Did you mean that if it is made to work for Function1 then it would work for Conversion as well?

Yes, we could maybe generalize the path-dependent syntax to other types than Function1.

@ashwinbhaskar
Copy link
Collaborator

@smarter @julienrf What is the consensus on this? Should we allow such a syntax?

@ashwinbhaskar ashwinbhaskar removed their assignment Nov 13, 2019
@dwijnand
Copy link
Member

I can find more usages if you ask again for more proofs of usage.

@neko-kai Do you remember where Akka used these? I'd be interested to check that out.

@dwijnand
Copy link
Member

dwijnand commented Nov 21, 2019

sbt:

The only dependent conversions I could find in sbt is (edit: derp, actually not dependent)

implicit def applicativeInstance[A[_]](implicit ap: Applicative[A]): Instance.Aux[A]

Most of the implicit defs are method enrichments (implicit def fileToRichFile(file: File): sbt.io.RichFile) or straight, non-dependant, conversions (implicit def projectToLocalProject(p: Project): LocalProject). Please correct me if I'm wrong, but I think sbt could live with a clunkier alternative for its applicativeInstance.

@smarter
Copy link
Member

smarter commented Nov 21, 2019

implicit def applicativeInstance[A[_]](implicit ap: Applicative[A]): Instance.Aux[A]

That doesn't look like a dependent method type to me, there's no parameter or result type which refers to a previous term parameter

@neko-kai
Copy link
Contributor Author

@dwijnand @smarter
Of path-dependent and only path-dependent conversions there are:

akka-http's Directive.scala has two 1 2
There's another in FieldMagnet
Another in OnSuccessMagnet
Another in ParamMagnet
Two more in akka-parsing: 1 2

For a total of 7 in akka-http.

@dwijnand

Most of the implicit defs in sbt are method enrichments

I mentioned implicit def macros for sbt. Why did I intentionally conflate dependent conversions and macro conversions here?

  1. Because whitebox macro conversions can be used to implement dependent conversions, which is what happens in shapeless.Witness implementation. (admittedly sbt's macro conversions are blackbox)
  2. Macro conversions are also hurt by Conversion, because macro methods must override a concrete method (because Conversion is now a value that can be upcast and "lose" it's macro status). Because of that to define a macro conversion you have to define two traits now:
trait Fixture0[A] extends Conversion[Int, A] {
  def a: A
  override def apply(i: Int): A = i match {
    case 0 => a
    case _ => throw new AssertionError("Runtime error: access only from 0 not any other number!")
  }
}

trait Fixture[A] extends Fixture0[A] {
  override inline def apply(i: Int): A = inline i match {
    case 0 => a
    case _ => error("Compile error: access only from 0 not any other number!")
  }
}

@nicolasstucki
Copy link
Contributor

Instead of writing a Conversion as

Conversion[T, U]

we could use

Conversion[T => U]

which also allows dependent conversion

Conversion[(x:T) => x.U]

Here, Conversion would be an opaque type

opaque type Conversion[+F <: Nothing => Any] = F

See #8523 for more details

@SethTisue
Copy link
Member

#8523 was merged — was this ticket left open on purpose (because of @neko-kai's caveat at https://contributors.scala-lang.org/t/proposal-changes-to-implicit-conversions/4101/15), or just by accident?

@smarter
Copy link
Member

smarter commented May 20, 2020

#8523 does not change the current implementation, it's a test file showing a prototype of what a changed implementation could look like.

@odersky odersky added the backlog No work planned on this by the core team for the time being. label Apr 5, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backlog No work planned on this by the core team for the time being. itype:language enhancement
Projects
None yet
Development

No branches or pull requests

9 participants