Skip to content

Conversation

@SethTisue
Copy link
Member Author

SethTisue commented Jun 23, 2023

singleton-ops

[singleton-ops] [error] /home/jenkins/workspace/scala-2.13.x-jdk11-integrate-community-build/target-0.9.20/project-builds/singleton-ops-43ff529071c9b3e9a0869a02f971698722b8df28/src/test/scala/singleton/twoface/TwoFaceStringSpec.scala:14:27: type mismatch;
[singleton-ops] [error]  found   : ValueOf[String]
[singleton-ops] [error]  required: ValueOf[String("Something")]
[singleton-ops] [error] Note: String >: String("Something"), but class ValueOf is invariant in type T.
[singleton-ops] [error] You may wish to investigate a wildcard type such as `_ >: String("Something")`. (SLS 3.2.10)
[singleton-ops] [error]     val a = TwoFace.String[W.`"Something"`.T]
[singleton-ops] [error]                           ^

shapeless

[shapeless] [error] /home/jenkins/workspace/scala-2.13.x-jdk11-integrate-community-build/target-0.9.20/project-builds/shapeless-1448284f88e35afd260e5bd6d419e528e292758a/core/shared/src/test/scala/shapeless/product.scala:320:19: could not find implicit value for parameter toMap: shapeless.ops.product.ToMap.Aux[ProductTests.this.Foo,K,V]
[shapeless] [error]       val m = foo.toMap
[shapeless] [error]                   ^

scapegoat

[scapegoat] [error] Failed tests:
[scapegoat] [error] 	com.sksamuel.scapegoat.inspections.unnecessary.UnnecessaryConversionTest

@SethTisue
Copy link
Member Author

Scala merged PRs in this time span:

* 7db02feacd - (HEAD -> 2.13.x, origin/HEAD, origin/2.13.x) Merge pull request #10425 from RustedBones/equals-converters (3 days ago) <Lukas Rytz>
* 0f5746fd7c - Merge pull request #10352 from som-snytt/issue/12757-glblub-loop (3 days ago) <Lukas Rytz>
* 0842f23f60 - Merge pull request #10424 from som-snytt/issue/12800-enum-exhaustivity (3 days ago) <Lukas Rytz>
* 6b16203c3d - Merge pull request #10414 from som-snytt/issue/12792-complement-constant (4 days ago) <Lukas Rytz>
* ebcee622d3 - Merge pull request #10404 from som-snytt/issue/12787-inliner (9 days ago) <som-snytt>
* 6e6bf056bb - Merge pull request #10434 from SethTisue/merge-2.12-to-2.13-20230612 (9 days ago) <Seth Tisue>
* 6243e90292 - Merge pull request #10426 from lrytz/sbt1.9 (2 weeks ago) <Lukas Rytz>
* 952c36e638 - Merge pull request #10411 from som-snytt/issue/12770-string-exhaust (3 weeks ago) <Dale Wijnand>
* a2adfb3bee - Merge pull request #10413 from som-snytt/tweak/print-syms (3 weeks ago) <Seth Tisue>
* 94c8030c94 - Merge pull request #10368 from som-snytt/issue/12769-no-sources (3 weeks ago) <Seth Tisue>

@SethTisue
Copy link
Member Author

SethTisue commented Jun 23, 2023

@lrytz @som-snytt I'll take a closer look and maybe do some bisecting... but y'all feel free to look as well :-)

@SethTisue
Copy link
Member Author

I did some bisecting and it seems that shapeless and singleton-ops regressed in scala/scala#10352 and scapegoat regressed in some earlier PR. I'll do some more bisecting and investigating when next at keyboard.

@som-snytt
Copy link

Sorry, I missed this call to arms. I guess by scapegoat, you don't mean me, but a project.

@SethTisue
Copy link
Member Author

for Scapegoat, the relevant PR is scala/scala#10414 ("Fold selection of unary ops")

@SethTisue
Copy link
Member Author

SethTisue commented Jun 30, 2023

for Scapegoat, our fork was out of date, but the problem remains reproducible even after refreshing the fork. it's also reproducible outside of dbuild, but you have to publish scalameta 4.7.8 locally for the Scala version you're testing

the test in question is UnnecessaryConversionTest's "when invoking toInt on an integer literal":

      "when invoking toInt on an integer literal" in {
        val code =
          """object Example extends App {
            |  val a = 3.toInt        // NullPointerException here (v1.3.6).
            |  val b = (10 / 5).toInt // NullPointerException here (v1.3.6).
            |}""".stripMargin

        compileCodeSnippet(code)
        compiler.scapegoat.feedback.warnings.size shouldBe 2
      }

and the failure is

[info]   - when invoking toInt on an integer literal *** FAILED *** (48 milliseconds)
[info]     0 was not equal to 2 (UnnecessaryConversionTest.scala:42)

where the missing warnings are "Unnecessary toInt on instance of Int"

so, I don't think this is a regression in Scala. it's good that we now constant-fold this. as far as I can see, Scapegoat should adjust. perhaps the compiler plugin ought to run before constant folding?

I will exclude the test for now. cc @sksamuel @lolgab @saeltz

@SethTisue
Copy link
Member Author

SethTisue commented Jun 30, 2023

for singleton-ops, the failing test is

  property("Safe Creation[]") = {
    val a = TwoFace.String[W.`"Something"`.T]
    a.getValue == "Something" && a.isLiteral
  }

it is easily reproducible outside of dbuild (with ++2.13.12-bin-7db02fe!)

note that Shapeless is involved (W is Shapeless's Witness, with type member T)

not obvious to me what's going on. cc @fthomas @milessabin

@SethTisue
Copy link
Member Author

for Shapeless, the problem reproduces outside dbuild, but you'll want the changes in milessabin/shapeless#1315, not yet merged (the repo has fatal warnings enabled and 2.13.11 caused new warnings to appear)

there are a lot of "implicit not found" errors, it isn't just the sample error I showed above. the full errors are at https://scala-ci.typesafe.com/job/scala-2.13.x-jdk11-integrate-community-build/4577/artifact/logs/shapeless-build.log

I have no hypothesis about what's going on here

@milessabin
Copy link

I'm afraid I don't have time to work on this. Maybe ask @joroKr21?

Eyeballing scala/scala#10352, the changes in GlbLubs.scala and TypeConstraints.scala look like they might have introduced some unexpected semantic changes along with the intended stack-safety and performance improvements. I could easily imagine that subtly changing type inference and hence implicit resolution.

shapeless is likely to be very sensitive to this, but I wouldn't be surprised if there's more breakage in the wild.

@joroKr21
Copy link
Member

I can take a look next week, I'm out for the weekend

@som-snytt
Copy link

I guess I'm in for the weekend.

@som-snytt
Copy link

Joyfully, it seems ScalaTest does not support scalaHome setting. Whenever I have to try something that depends on ScalaTest, I find myself checking twitter more often. Please disabuse me as necessary.

[error] Caused by: java.lang.ClassNotFoundException: scala.Product
[error]         at java.base/java.net.URLClassLoader.findClass(URLClassLoader.java:445)
[error]         at java.base/java.lang.ClassLoader.loadClass(ClassLoader.java:588)
[error]         at java.base/java.lang.ClassLoader.loadClass(ClassLoader.java:521)
[error]         at java.base/java.lang.ClassLoader.defineClass1(Native Method)
[error]         at java.base/java.lang.ClassLoader.defineClass(ClassLoader.java:1013)
[error]         at java.base/java.security.SecureClassLoader.defineClass(SecureClassLoader.java:150)
[error]         at java.base/java.net.URLClassLoader.defineClass(URLClassLoader.java:524)
[error]         at java.base/java.net.URLClassLoader$1.run(URLClassLoader.java:427)
[error]         at java.base/java.net.URLClassLoader$1.run(URLClassLoader.java:421)
[error]         at java.base/java.security.AccessController.doPrivileged(AccessController.java:714)
[error]         at java.base/java.net.URLClassLoader.findClass(URLClassLoader.java:420)
[error]         at java.base/java.lang.ClassLoader.loadClass(ClassLoader.java:588)
[error]         at java.base/java.lang.ClassLoader.loadClass(ClassLoader.java:575)
[error]         at java.base/java.lang.ClassLoader.loadClass(ClassLoader.java:521)
[error]         at java.base/java.lang.Class.forName0(Native Method)
[error]         at java.base/java.lang.Class.forName(Class.java:496)
[error]         at java.base/java.lang.Class.forName(Class.java:475)
[error]         at org.scalatest.tools.Framework$ScalaTestTask.loadSuiteClass(Framework.scala:408)
[error]         at org.scalatest.tools.Framework$ScalaTestTask.suiteClass$lzycompute(Framework.scala:416)
[error]         at org.scalatest.tools.Framework$ScalaTestTask.suiteClass(Framework.scala:416)
[error]         at org.scalatest.tools.Framework$ScalaTestTask.accessible$lzycompute(Framework.scala:417)
[error]         at org.scalatest.tools.Framework$ScalaTestTask.accessible(Framework.scala:417)
[error]         at org.scalatest.tools.Framework$ScalaTestTask.shouldDiscover$lzycompute(Framework.scala:420)
[error]         at org.scalatest.tools.Framework$ScalaTestTask.shouldDiscover(Framework.scala:419)
[error]         at org.scalatest.tools.Framework$ScalaTestRunner.$anonfun$tasks$2(Framework.scala:752)
[error]         at org.scalatest.tools.Framework$ScalaTestRunner.$anonfun$tasks$2$adapted(Framework.scala:750)
[error]         at scala.collection.ArrayOps$WithFilter.map(ArrayOps.scala:90)
[error]         at org.scalatest.tools.Framework$ScalaTestRunner.tasks(Framework.scala:750)
[error]         at sbt.TestRunner.tasks(TestFramework.scala:119)

@som-snytt
Copy link

For the shapeless error, scala/scala#10452

Contrary to Miles' conjecture, it was not subtle. However, I haven't investigated yet how it breaks.

After creating a minimal test on the shapeless side, I realized reversion was quicker than analysis. But I am motivated to understand it at some future day of leisure.

@joroKr21
Copy link
Member

joroKr21 commented Jul 2, 2023

Thanks 👍 - in this case it's the reference equality on types which comes up every now and then as the source of bugs.

Screenshot 2023-07-03 at 00 15 12

@saeltz
Copy link

saeltz commented Jul 3, 2023

so, I don't think this is a regression in Scala. it's good that we now constant-fold this. as far as I can see, Scapegoat should adjust. perhaps the compiler plugin ought to run before constant folding?

@SethTisue, thanks for bringing this up! Could you remind me again in which phase the constant-folding is happening? Scapegoat currently runs after typer and before patmat. I thought it was a sub-phase of type-checking.

The other way to solve this would be for Scapegoat to only run this inspection for Scala 2.12.

@SethTisue
Copy link
Member Author

@saeltz let's discuss on a scapegoat ticket instead of here

@saeltz
Copy link

saeltz commented Jul 3, 2023

let's discuss on a scapegoat ticket instead of here

@SethTisue, I created scapegoat-scala/scapegoat#754.

@SethTisue SethTisue mentioned this pull request Jul 4, 2023
@SethTisue
Copy link
Member Author

SethTisue commented Jul 4, 2023

verified that Som's followup PR fixed shapeless and singleton-ops

there is now a scalafmt failure, but I'm going to merge this anyway and deal with that separately (issue: #1680)

@SethTisue SethTisue merged commit 57abcdb into scala:2.13.x Jul 4, 2023
@SethTisue SethTisue deleted the new-scala-2.13-sha-20230622 branch July 4, 2023 15:48
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.

5 participants