bug icon indicating copy to clipboard operation
bug copied to clipboard

[2.13.9, 2.13.10] Issue with zio-prelude `1.0.0-RC8-1` (zio1) macros

Open guizmaii opened this issue 3 years ago • 12 comments

There's an issue between Scala 2.13.9 and zio-prelude 1.0.0-RC8-1 (last version published supporting ZIO1) macros:

import zio.prelude.*

object LongNatural extends Subtype[Long] {
  //noinspection TypeAnnotation                            // IntelliJ annnotation
  @SuppressWarnings(Array("scalafix:ExplicitResultTypes")) // scalafix annotation
  override def assertion = assert(greaterThanOrEqualTo(0L))

  def zero: LongNatural = LongNatural(0L)
}

is failing to compile with the following error:

[error]  Newtype Assertion Failed 
[error] We were unable to read your assertion at compile-time.
[error] This is because you have annotated `def assertion` with its type signature:
[error] 
[error]     override def assertion: QuotedAssertion[Long] = assert(...)
[error]    
[error] Due to the macro machinery powering this feature, you MUST NOT ANNOTATE this method. 
[error] Try deleting the type annotation and recompiling. Something like:
[error] 
[error]     override def assertion = assert(...)
[error]     
[error]     def zero: LongNatural = LongNatural(0L)
[error]                                        ^

This was compiling fine with Scala 2.13.8. Note that we're using the -Xsource:3 flag.

guizmaii avatar Sep 22 '22 11:09 guizmaii

is the problem also reproducible without -Xsource:3?

are you able to minimize to a self-contained test, without external dependencies?

SethTisue avatar Sep 22 '22 14:09 SethTisue

@SethTisue https://scastie.scala-lang.org/EBPQHJv9TvSth8S09yaJ4Q :)

guizmaii avatar Sep 22 '22 14:09 guizmaii

the "without external dependencies" part is important

SethTisue avatar Sep 22 '22 14:09 SethTisue

I don't know how the zio-prelude macros are implemented. So, I'm not sure I can provide you this, sorry.

guizmaii avatar Sep 22 '22 14:09 guizmaii

Very likely that it's the same issue as #12645.

Jasper-M avatar Sep 22 '22 14:09 Jasper-M

do you know anyone involved with ZIO maintenance who could help with the minimization?

SethTisue avatar Sep 22 '22 14:09 SethTisue

These macros have been implemented and are maintained by Kit Langton (@kitlangton)

guizmaii avatar Sep 22 '22 14:09 guizmaii

Very likely that it's the same issue as https://github.com/scala/bug/issues/12645.

Okay, I'm tentatively closing this ticket, then. We can reopen if more information comes to light.

SethTisue avatar Sep 23 '22 18:09 SethTisue

@SethTisue @som-snytt

The fix made in Scala 2.13.10 (ie. #10160) doesn't seem to fix this issue. Here's the compilation error I have:

[info] Non-compiled module 'compiler-bridge_2.13' for Scala 2.13.10. Compiling...
[info]   Compilation completed in 8.166s.
[error] -target is deprecated: Use -release instead to compile against the correct platform API.
[error] /home/runner/work/my-project/src/main/scala/my/company/project/types/primitives.scala:
[error]  Newtype Assertion Failed 
[error] We were unable to read your assertion at compile-time.
[error] This is because you have annotated `def assertion` with its type signature:
[error] 
[error]     override def assertion: QuotedAssertion[Int] = assert(...)
[error]    
[error] Due to the macro machinery powering this feature, you MUST NOT ANNOTATE this method. 
[error] Try deleting the type annotation and recompiling. Something like:
[error] 
[error]     override def assertion = assert(...)
[error]     
[error]     val one = PosInt(1)

The corresponding code:

import zio.prelude.*

object PosInt extends Subtype[Int] {
  override def assertion = assert(greaterThan(0))

  val one = PosInt(1)
}
type PosInt = PosInt.Type

guizmaii avatar Oct 11 '22 19:10 guizmaii

Indeed.

The original change in 2.13.9 (https://github.com/scala/scala/pull/9891) is to use the overridden method's type under -Xsource:3:

scala> trait T { def f: Object }; object C extends T { def f = "" }; C.f
trait T
object C
val res0: Object = "" // type String without Xsource:3

To the macro, the tree then looks like the result type was explicit.

The fix in 2.13.10 (https://github.com/scala/scala/pull/10160) only reverts that rule within expanding / type checking a macro. However, here the type-checking of override def assertion = ... is outside a macro, so the new rule applies.

What code is the macro using to tell whether the return type was explicit or not? I believe the following should work correctly for 2.13.9/10 and also before:

val hasInferredType = defDef.tpt match { case tt: TypeTree => tt.wasEmpty; case _ => false }

lrytz avatar Oct 12 '22 08:10 lrytz

Thanks for the suggestion @lrytz.

I'll see with ZIO people if we can try this change.

guizmaii avatar Oct 12 '22 08:10 guizmaii

Possibly the improved check in the PR is viable, so I'll reopen.

som-snytt avatar Oct 13 '22 18:10 som-snytt

@som-snytt is your fix working?

@SethTisue If the fix is working, is it possible to merge it and make a new release, please? Because of this bug, we're blocked on Scala 2.13.8 which contains a severe CVE 😟

guizmaii avatar Oct 19 '22 08:10 guizmaii

The CVE can be exploited in applications that deserialize untrusted data, which is always a security risk. The fix in 2.13.9 / 10 targets a specific, known entry point in the Scala library. But it's very likely that an application's classpath contains other, similar entry points. So in that sense, it's not severe.

We're currently planning 2.13.11 on the usual schedule (3-4 months). If you'd like to use 2.13.10, it should be possible to implement a workaround in the macro.

lrytz avatar Oct 19 '22 08:10 lrytz

is your fix working?

@guizmaii It would be helpful if you gave the PR snapshot a try yourself and see if it resolves your issue. There are instructions on using PR validation snapshots at https://github.com/scala/scala/blob/2.13.x/README.md#scala-ci

SethTisue avatar Oct 19 '22 09:10 SethTisue

The PR is now merged, so it'll be available for testing in 2.13.11-bin-8fa1e7c (which CI should build within the next few hours).

SethTisue avatar Oct 26 '22 15:10 SethTisue

Thanks @SethTisue.

Sorry, I wasn't able to find time to test the PR. This release will simplify my task of testing it. I'll come back to you with the results

guizmaii avatar Oct 26 '22 16:10 guizmaii

2.13.11-bin-8fa1e7c is now published

scala-cli -S 2.13.11-bin-8fa1e7c already works (as does just scala-cli -S 2.nightly), but with sbt and other tools you'll still need to add a resolver to test the nightly; unlike Scala 3 nightlies, Scala 2 nightlies aren't published to Maven Central: https://stackoverflow.com/questions/40622878/how-do-i-tell-sbt-or-scala-cli-to-use-a-nightly-build-of-scala-2-12-or-2-13

SethTisue avatar Oct 26 '22 16:10 SethTisue

My zio-tester with the example compiles with head. (Too lazy to read the instructions again on getting a blessed nightly.)

    object LongNatural extends zio.prelude.Subtype[Long] {
      def <init>(): Report.LongNatural.type = {
        LongNatural.super.<init>();
        ()
      };
      override def assertion: zio.prelude.QuotedAssertion[Long]{def magic: Int} = {
        final class $anon extends AnyRef with zio.prelude.QuotedAssertion[Long] {
          def <init>(): <$anon: zio.prelude.QuotedAssertion[Long]> = {
            $anon.super.<init>();
            ()
          };
          @zio.prelude.assertionQuote[Long](zio.prelude.Assertion.greaterThanOrEqualTo[Long](0L)(math.this.Ordering.Long)) @zio.prelude.assertionString("greaterThanOrEqualTo(0L)") def magic: Int = 42;
          def assertion: zio.prelude.Assertion[Long] = zio.prelude.Assertion.greaterThanOrEqualTo[Long](0L)(math.this.Ordering.Long)
        };
        new $anon()
      };
      def zero: Report.LongNatural = zio.prelude.Newtype.unsafeWrap[Report.LongNatural.type](Report.this.LongNatural)(0L)
    };
    type LongNatural = Report.LongNatural.Type

For my benefit, my build.sbt

scalaVersion := "2.13.10"
//scalaVersion := "2.13.8"

scalaHome := Some(file(".../projects/scala/build/pack"))

scalacOptions ++= Seq(
  "-Xlint",
  "-Xsource:3",
  //"-Vmacro",
  "-Vprint:typer",
  //"-Wnonunit-statement",
)

//val z = "1.0.0-RC8-1+54-5eb77993-SNAPSHOT"
val z = "1.0.0-RC8-1"
//val z = "1.0.0-RC16"

libraryDependencies += "dev.zio" %% "zio-prelude" % z

I had to reread the docs to refresh my memory. I accidentally tested that it still fails without pulling in my fix.

Also, note to self, the quasi-companion works because LongNatural is a prefix of LongNatural.Type.

som-snytt avatar Oct 26 '22 17:10 som-snytt

I can confirm that my code is compiling with this 2.13.11-bin-8fa1e7c version.

Thanks everyone for your help! ❤️

guizmaii avatar Oct 27 '22 15:10 guizmaii