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

Do not harmonize constant args if their type doesn't affect resType #13353

Merged
merged 1 commit into from
Aug 23, 2021

Conversation

dos65
Copy link
Contributor

@dos65 dos65 commented Aug 22, 2021

Fixes the following sample from #9939:

scala> f"${3.14}%.2f rounds to ${3}%d"
1 |f"${3.14}%.2f rounds to ${3}%d"
  |                          ^
  |                          type mismatch;
  |                           found   : Double
  |                           required: Int
  | This location contains code that was inlined from rs$line$2:1

At the moment when StringInterpolation transformation was performed instead
of receiving List(Constant(3.0), Constant(3)) arguments they
were: List(Constant(3.0), Constant(3.0)) (the second one was converted to double because of harmonization).
That caused the reported type mismatch.

In Scala2 harmonization doesn't happen if the resulting type is fully defined.
For f-interp it shouldn't happen too as it's resulting type is a String:

  def f[A >: Any](args: A*): String = macro ???

Fixes the following sample from scala#9939:
```scala
scala> f"${3.14}%.2f rounds to ${3}%d"
1 |f"${3.14}%.2f rounds to ${3}%d"
  |                          ^
  |                          type mismatch;
  |                           found   : Double
  |                           required: Int
  | This location contains code that was inlined from rs$line$2:1
```

At the moment when StringInterpolation transformation was performed instead
of receiving `List(Constant(3.0), Constant(3))` arguments they
were: `List(Constant(3.0), Constant(3.0))` (the second one was converted to double because of harmonization).
That caused the reported type mismatch.

In Scala2 harmonization doesn't happen if the resulting type is fully defined.
For f-interp it shouldn't happen too as it's resulting type is a `String`:
```scala
  def f[A >: Any](args: A*): String = macro ???
```
@som-snytt
Copy link
Contributor

som-snytt commented Aug 22, 2021

Thanks, I happen to have just resurrected my PR for updating the interpolator, but that test from Scala 2 is still commented out. I see on Scala 2, that was for the fix to add the type parameter; there is no harmonics. (https://github.com/scala/scala/blob/2.13.x/test/junit/scala/lang/stringinterpol/StringContextTest.scala#L103)

@som-snytt
Copy link
Contributor

A different idea is to not harmonize because of the lower bound >: Any. That is syntax for fixing or defining the A, and I guess that was part of the trick in 2014, according to the discussion on the old ticket. I linked the new ticket.

I think I still have my "I'm a lubber not a typer" t-shirt from back then.

I didn't get enough mileage from that slogan.

scala> def f[A](xs: A*) = xs.mkString
def f[A](xs: A*): String

scala> f(3, 3.14)
val res1: String = 3.03.14

@dos65
Copy link
Contributor Author

dos65 commented Aug 23, 2021

@som-snytt Thanks! I remember that I've checked if it works without >: A but it seems that I misinterpret the result. Will change the PR.

btw. Do you have any idea where this logic might be found in scala2 codebase?

@odersky odersky merged commit 4958012 into scala:master Aug 23, 2021
@dos65
Copy link
Contributor Author

dos65 commented Aug 23, 2021

@odersky @som-snytt noticed that this fix isn't fully compatible with scala2 implementation. Is it ok to leave it as it is?

@odersky
Copy link
Contributor

odersky commented Aug 23, 2021

I think we can leave it as it is.

@Kordyjan Kordyjan added this to the 3.1.0 milestone Aug 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants