Skip to content

Can't convert a function to a trait using an implicit conversion and fully structural types #12609

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

Closed
hunterpayne opened this issue Jun 11, 2022 · 31 comments

Comments

@hunterpayne
Copy link

hunterpayne commented Jun 11, 2022

Reproduction steps

Scala version: 2.13.8

class RS[I]() {

  def map[O](value: ScopedValue[I, O]): RS[O] = ???
}

trait ScopedValue[I, O] { }

object Test {

  implicit def value[I, O](func: (I) => O): ScopedValue[I, O] =
    new ScopedValue[I, O] { }
  
  val exec = new RS[Long]()
  // doesn't compile but should
  val set = exec.map[Long] { l => l }
  // compiles
  val set2 = exec.map[Long] { (l: Long) => l }
}

Problem

It seems that the function needs to be fully specified to make the implicit conversion happen. Otherwise you get an 'missing parameter type' error which seems wrong. If the spec says that this is the correct behavior please let me know and I will close this.

@SethTisue SethTisue added fixed in Scala 3 This issue does not exist in the Scala 3 compiler (https://github.com/lampepfl/dotty/) infer labels Jun 11, 2022
@Jasper-M
Copy link

The tag says "fixed in Scala 3" but it's not, as far as I can see. Also, I'm no type inference expert, but it seems unlikely that this will ever work.

@joroKr21
Copy link
Member

Right - we have to know what is the type of the thing we convert before we convert it

@joroKr21
Copy link
Member

Similar issue with extension methods: lampepfl/dotty/issues/15427

@hunterpayne
Copy link
Author

But the method is requesting a ScopedValue[I, O] which can be constructed using the implicit conversion of a (I) => O to a ScopedValue[I, O] (hence the structural type). So because the I comes from exec and is specified (it is a Long), the compiler has all the information needed to determine that l => l is l: Long => l.

BTW, is this really fixed in Dotty/Scala 3? It would be a lot of work to upgrade to Scala 3 but if that is the fix then I suppose it is worth it.

@SethTisue SethTisue removed the fixed in Scala 3 This issue does not exist in the Scala 3 compiler (https://github.com/lampepfl/dotty/) label Jun 13, 2022
@SethTisue
Copy link
Member

is this really fixed in Dotty/Scala 3?

sorry about that, not sure what I was thinking, I tried it again just now and Scala 3.1.3-RC4 does give:

15 |  val set = exec.map[Long] { l => l }
   |                             ^
   |                          Missing parameter type

@hunterpayne
Copy link
Author

Ok, so next question, should it work? The compiler seems to have all the information it should need to know how to assign the type to the function argument? Or is there part of the spec that says that this shouldn't work?

@SethTisue
Copy link
Member

SethTisue commented Jun 14, 2022

The spec has relatively little to say about type inference. But have you found anything in the spec that leads you to believe that it should work?

I strongly suspect this matter is exactly as simple as @joroKr21 said above:

we have to know what is the type of the thing we convert before we convert it

Note also that implicit conversions in general are increasingly discouraged in Scala 3, anyway.

@som-snytt
Copy link

I sympathize with the OP.

The handling of under-specified function literals for overloading (https://dotty.epfl.ch/docs/reference/changed-features/overload-resolution.html) suggests (to a certain naive belief) that the same should be possible when conversions are available.

Take my literal as ? => ?, the expected type is ScopedValue[Long, Long], then the expected type for the conversion is Long => Long.

@hunterpayne
Copy link
Author

If I change (I) => O to something that isn't a function, this code compiles. So I feel strongly that it should work. It is only the raising of the passed type to a function that makes this not work.

@Jasper-M
Copy link

If I change (I) => O to something that isn't a function, this code compiles. So I feel strongly that it should work. It is only the raising of the passed type to a function that makes this not work.

Can you show a piece of code that works the way you expect?

@hunterpayne
Copy link
Author

class RS[I]() {

  def map[O](value: ScopedValue[I, O]): RS[O] = ???
}

trait ScopedValue[I, O] { }

object Test {

  implicit def value[I, O](func: (I) => O): ScopedValue[I, O] =
    new ScopedValue[I, O] { }
  implicit def value2[I, O](func: I): ScopedValue[I, O] =
    new ScopedValue[I, O] { }
  
  val exec = new RS[Long]()
  // doesn't compile but should
  val set = exec.map[Long] { l => l }
  // compiles
  val set2 = exec.map[Long] { l: Long => l }
  // compiles
  val set3 = exec.map[Long] { 7L }
}

@Jasper-M
Copy link

val set3 = exec.map[Long] { 7L }

But here you have a value of type Long while in the other you have a value of type Function1[?, ?] where there are still unresolved type variables.

I think a more similar example would be

class Foo[A, B]
implicit def value2[I, O](foo: Foo[I, O]): ScopedValue[I, O] =
    new ScopedValue[I, O] { }

//doesn't compile
exec.map[Long] { new Foo }

@hunterpayne
Copy link
Author

In that case B is never specified. Let me try again


class RS[I]() {

  def map[O](value: ScopedValue[I, O]): RS[O] = ???
}

class Foo[I, O]() {
}

trait ScopedValue[I, O] { }

object Test {

  implicit def value[I, O](func: (I) => O): ScopedValue[I, O] =
    new ScopedValue[I, O] { }
  implicit def value2[I](func: Seq[I]): ScopedValue[I, Long] =
    new ScopedValue[I, Long] { }
  implicit def value3[I](func: Foo[I, Long]): ScopedValue[I, Long] =
    new ScopedValue[I, Long] { }

  val exec = new RS[Long]()
  // doesn't compile but should
  // val set = exec.map[Long] { l => l }
  // compiles
  val set2 = exec.map[Long] { l: Long => l }
  // compiles
  val set3 = exec.map[Long] { Seq() }
  // doesn't compile
  val set4 = exec.map[Long] { new Foo }
}

So it appears that if 2 type parameters are present it doesn't work but if only 1 is present, it does?

@Jasper-M
Copy link

Something else is at play here. In exec.map[Long] { Seq() } the compiler still can't infer the type parameter to Seq.apply so he defaults to Nothing, just like with Foo. But because Seq is covariant in its type parameter, Seq[Nothing] is a subtype of Seq[Long] and value2 becomes an eligible implicit conversion.

@hunterpayne
Copy link
Author

So you are right that this compiles:

class RS[I]() {

  def map[O](value: ScopedValue[I, O]): RS[O] = ???
}

class Foo[+I, +O]() {
}

trait ScopedValue[I, O] { }

object Test {

  implicit def value[I, O](func: (I) => O): ScopedValue[I, O] =
    new ScopedValue[I, O] { }
  implicit def value2[I](func: Seq[I]): ScopedValue[I, Long] =
    new ScopedValue[I, Long] { }
  implicit def value3[I](func: Foo[I, Long]): ScopedValue[I, Long] =
    new ScopedValue[I, Long] { }

  val exec = new RS[Long]()
  // doesn't compile but should
  // val set = exec.map[Long] { l => l }
  // compiles
  val set2 = exec.map[Long] { l: Long => l }
  // compiles
  val set3 = exec.map[Long] { Seq() }
  // doesn't compile
  val set4 = exec.map[Long] { new Foo }
}

However, Function1 is contravariant in the first type parameter. So it still doesn't compile for the case I really care about. It is also weird that it requires covariance as it is increasingly difficult to use covariance and contravariance in Scala. You can't even have an implicit TypeTag of a covariant or contravariant type which makes using them very difficult. In my use case, it is a complete non-starter as I need the type tag to do what we require. So back to square 1.

@hunterpayne
Copy link
Author

hunterpayne commented Jun 16, 2022

Right - we have to know what is the type of the thing we convert before we convert it

I do want to address this before I give up hope here. The implicit conversion in question can convert any type of Function1, it has no restrictions. So no, you are wrong and you do not have to know the type of the thing first, you only have to know that it is a Function1 which it clearly is. And half of the required type is specified even before you do the implicit conversion as the argument is known to be a ScopedValue[Long, _] beforehand because exec is of type RS[Long]. If you do the implicit conversions, the compiler would find that the rest of the type is indeed specified by the return value of the function. BTW, this compiler error happens even when the return value of the function doesn't depend on the argument type (e.g. j => 7L). I used this specific example because perhaps l => l is a good corner case to cover too.

This seems like an issue that has been reported several times by others so I guess you will never fix this issue. Or maybe you just can't. It is probably a pretty hard bug to fix.

@som-snytt
Copy link

Probably no one read my previous comment, but I suggested "doable".

What you would sacrifice is some implicit scope, which would normally include the type args of the Function[A, R].

Intuitively, this is user-defined SAM conversion, where the target type is not a SAM type, but the context knows how to wire the function to the type.

My comment about overloading was intended to say that overloading used to break inference in a similar way. Both have function literals in need of an expected type.

I don't know if it would be too much complexity for whatever the use case is.

@hunterpayne
Copy link
Author

So the use case would be something like, "using an under-specified implicit conversion to infer Function type parameters". And the repo case is the original one for this bug. The type inference in this case stops when it finds an unspecified function when what it should do is pull in I from the function call and O from the return type of the Function1[I, O]. Is this correct?

@hunterpayne
Copy link
Author

hunterpayne commented Jun 20, 2022

@som-snytt I created a unit test to verify this issue. It simply tries to compile the repo case above. And I added this line to the type inference code:
Console.println(s"typed ${tree} in mode ${mode} pt ${pt}")
on line 6011 of Typers.scala.

Here is the output I get from the case that works (where the type of the anonymous function's argument is specified):

typed exec.map[Long](((l: Long) => l)) in mode EXPRmode pt ?
typed exec.map[Long] in mode APPSELmode-BYVALmode-EXPRmode-FUNmode-POLYmode pt 
typed exec.map in mode APPSELmode-BYVALmode-EXPRmode-FUNmode-POLYmode-TAPPmode pt ?
typed exec in mode APPSELmode-EXPRmode-POLYmode-QUALmode pt ?
typed Long in mode TYPEmode pt ?
typed ((l: Long) => l) in mode BYVALmode-EXPRmode pt ScopedValue[Long,Long]

And here is the output when I try the repo case (where the type of the anonymous function isn't specified):

typed exec.map[Long](((l) => l)) in mode EXPRmode pt ?
typed exec.map[Long] in mode APPSELmode-BYVALmode-EXPRmode-FUNmode-POLYmode pt 
typed exec.map in mode APPSELmode-BYVALmode-EXPRmode-FUNmode-POLYmode-TAPPmode pt ?
typed exec in mode APPSELmode-EXPRmode-POLYmode-QUALmode pt ?
typed Long in mode TYPEmode pt ?
typed ((l) => l) in mode BYVALmode-EXPRmode pt ScopedValue[Long,Long]

So the type inference engine is able to find the correct type with no changes (nice). Somehow the compiler error is thrown incorrectly further along in the process. So it seems to me that the fix is to somehow add a check to the code that throws the compiler error to avoid this case. However, I don't want to start commenting out lines that throw the compiler error farther down the line and I don't know the correct checks to add. Some help determining how to construct the proper check would be appreciated.

@hunterpayne
Copy link
Author

@som-snytt PR with the fix: scala/scala#10065

@hunterpayne
Copy link
Author

Can someone trigger the travis jobs for this PR. Been waiting for almost a week to run the CI jobs.

@som-snytt
Copy link

@hunterpayne I'd suggest squashing and push -f and maybe also add [ci: last-only] to the PR title (IIRC). That last-only command reduces the chance that a run doesn't sync. Also, there a sync command for comments which Seth has used effectively, or perhaps ineffectively.

@hunterpayne
Copy link
Author

just made a new PR: scala/scala#10067

@SethTisue
Copy link
Member

SethTisue commented Jul 6, 2022

Not sure why Travis didn't run. Making a whole new PR is the nuclear option, but there are usually measures we can take short of that, once you've alerted us to the problem. (And, no big deal, but: it's better to do that on the PR than on the bug.)

Note that Jenkins and Travis-CI are redundant with each other and we might not keep both around forever. There are some circumstances under which one can fail and the other succeed, but they're relatively rare. scala/scala-dev#751 is a firehose of history and detail on this subject. The short version is that if one is green, you can be reasonably confident the other one will be too, even if some transient problem is temporarily preventing it from running.

@hunterpayne
Copy link
Author

hunterpayne commented Jul 6, 2022

I tried 2 different ways to squash the commits and neither worked as described (even though they were Github specific instructions). So I gave up and made a new PR from a fresh fork. This is typical for git in my experience.

I do appreciate any help I can get triggering that last build.

@hunterpayne
Copy link
Author

I don't know if this matter but in TravisCI it says
We are unable to start your build at this time. You exceeded the number of users allowed for your plan. Please review your plan details and follow the steps to resolution.

@SethTisue
Copy link
Member

That's probably coming from your fork? In the main repo, scala/scala#10067 already shows a green Travis-CI run.

@hunterpayne
Copy link
Author

You are probably right but the validate-main job still hasn't run.

@hunterpayne
Copy link
Author

The builds are all complete and successful. Thank you for your help.

@SethTisue
Copy link
Member

(Jenkins had a transient disk-space issue; Lukas fixed it.)

@SethTisue
Copy link
Member

as per discussion on scala/scala#10067, we don't have consensus that there is a bug here. hence, closing

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