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

fix compilation error if Repeated is included in Diagrams assertion #2037

Merged
merged 6 commits into from
Jun 21, 2021

Conversation

taisukeoe
Copy link
Contributor

I'd like to report a bug with the latest combination, Scala 3.0.0, ScalaTest 3.2.9 and sbt 1.5.2, and fix the bug by this PR.

About the bug:

Test/compile fails in Diagrams assert which takes an expression including varargs in Scala 3.0.0. It compiles in Scala 2.13.6.

[error] -- [E007] Type Mismatch Error: /home/taisukeoe/workspace/Sandbox/scala3-scalatest/src/test/scala/DiagramTest.scala:7:4 
[error] 7 |    assert(Seq("a", "b") == Seq("a"))
[error]   |    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
[error]   |    Found:    String*
[error]   |    Required: String
[error]   |
[error]   |    The following import might make progress towards fixing the problem:
[error]   |
[error]   |      import org.scalactic.Prettifier.default
[error]   |
[error]   | This location contains code that was inlined from DiagramTest.scala:9
[error] -- [E007] Type Mismatch Error: /home/taisukeoe/workspace/Sandbox/scala3-scalatest/src/test/scala/DiagramTest.scala:7:4 
[error] 7 |    assert(Seq("a", "b") == Seq("a"))
[error]   |    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
[error]   |    Found:    (11 : Int)
[error]   |    Required: String
[error]   |
[error]   |    The following import might make progress towards fixing the problem:
[error]   |
[error]   |      import org.scalactic.Prettifier.default
[error]   |
[error]   | This location contains code that was inlined from DiagramTest.scala:9

Minimal Reproducible project

https://github.com/taisukeoe/scala3-scalatest/tree/aba74a99d00cd42b1215147f62d094a2ce628488

About This PR

This PR fixes the above compilation error, and make Diagrams assertion works with varargs, which is represented by Typed(Repeated(_, _), _) term.

Note

There is still a Diagrams assertion error message corruption in Scala 3.0.0, as follows.

[info]   org.scalatest.diagrams.Diagrams.assert(Seq(a, b) != Seq(3, 5))
[info]                                        | |   |  |  || |   |  |
[info]                                        | |   3  5  || |   3  5
[info]                                        | |         || scala.collection.immutable.Seq$@2c58b1c9
[info]                                        | |         |List(3, 5)
[info]                                        | |         false
[info]                                        | scala.collection.immutable.Seq$@2c58b1c9
[info]                                        List(3, 5) (DirectDiagrammedAssertionsSpec.scala:527)

@cla-bot
Copy link

cla-bot bot commented Jun 2, 2021

Hi @taisukeoe, we require contributors to sign our Contributor License Agreement, and we don't have yours on file. In order for us to review and merge your code, please access https://www.artima.com/cla/choose-type to sign our Contributor License Agreement. Your effort is highly appreciated. Thank you.

@artimasites
Copy link

@cla-bot[bot] check

@cla-bot cla-bot bot added the cla-signed label Jun 2, 2021
@cla-bot
Copy link

cla-bot bot commented Jun 2, 2021

The cla-bot has been summoned, and re-checked this pull request!

@cheeseng
Copy link
Contributor

cheeseng commented Jun 6, 2021

@taisukeoe Sorry for getting back late to this and thanks for the fix! The fix looks good and worked great for Scala 3, but the added test for vararg seems working only under Scala 3, when built with Scala 2:

[error] /home/cheeseng/git/scalatest/jvm/diagrams-test/src/test/scala/org/scalatest/diagrams/DirectDiagrammedAssertionsSpec.scala:601:47: exception during macro expansion: 
[error] scala.MatchError: varargs (of class scala.reflect.internal.Trees$Ident)
[error] 	at org.scalatest.diagrams.DiagrammedExprMacro.traverseApply(DiagrammedExprMacro.scala:110)
[error] 	at org.scalatest.diagrams.DiagrammedExprMacro.applyExpr(DiagrammedExprMacro.scala:202)
[error] 	at org.scalatest.diagrams.DiagrammedExprMacro.transformAst(DiagrammedExprMacro.scala:387)
[error] 	at org.scalatest.diagrams.DiagrammedExprMacro.applyExpr(DiagrammedExprMacro.scala:205)
[error] 	at org.scalatest.diagrams.DiagrammedExprMacro.transformAst(DiagrammedExprMacro.scala:387)
[error] 	at org.scalatest.diagrams.DiagrammedExprMacro.genMacro(DiagrammedExprMacro.scala:421)
[error] 	at org.scalatest.diagrams.DiagramsMacro$.macroImpl(DiagramsMacro.scala:101)
[error] 	at org.scalatest.diagrams.DiagramsMacro$.assert(DiagramsMacro.scala:125)
[error] 	at sun.reflect.GeneratedMethodAccessor2.invoke(Unknown Source)
[error] 	at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
[error] 	at java.lang.reflect.Method.invoke(Method.java:498)
[error] 	at scala.reflect.macros.runtime.JavaReflectionRuntimes$JavaReflectionResolvers.$anonfun$resolveJavaReflectionRuntime$6(JavaReflectionRuntimes.scala:51)
[error] 	at scala.tools.nsc.typechecker.Macros.macroExpandWithRuntime(Macros.scala:770)
[error]         org.scalatest.diagrams.Diagrams.assert(varargs(1, "y", "z") == 1 -> Seq("y", "z"))
[error]                                               ^

From the error message may be we can enhance our Scala 2 macro to support it, if not perhaps the test can be added under dotty/diagrams-test instead?

@taisukeoe
Copy link
Contributor Author

taisukeoe commented Jun 6, 2021

@cheeseng
Thank you for confirmation!

From the error message may be we can enhance our Scala 2 macro to support it, if not perhaps the test can be added under dotty/diagrams-test instead?

As long as I quickly checked, this error is occurred only when varargs is defined as local method inside of describe method, and there seems to be a rare case if a method needs to be defined inside of describe.
So I'd move varargs method to outside of describe, because it's nicer not to have Scala 3 specific test file if possible.


Oops, this change seems to break Scala 3 side test. I'm checking this.

[error] -- Error: /home/taisukeoe/workspace/OSS/scalatest/dotty/diagrams-test/target/scala-3/src_managed/test/org/scalatest/diagrams/DirectDiagrammedAssertionsSpec.scala:541:8 
[error] 541 |        org.scalatest.diagrams.Diagrams.assert(varargs(1, "y", "z") == 1 -> Seq("y", "z"))
[error]     |        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
[error]     |wrong number of arguments at pickler for (x: Int, y: String*): (Int, Seq[String]): (x.value.varargs : (x: Int, y: String*): (Int, Seq[String])), expected: 2, found: 3
[error]     | This location contains code that was inlined from DirectDiagrammedAssertionsSpec.scala:541

@taisukeoe
Copy link
Contributor Author

Oops, this change seems to break Scala 3 side test. I'm checking this.

It is fixed now. Would you please review this again? @cheeseng

@cheeseng
Copy link
Contributor

cheeseng commented Jun 7, 2021

@taisukeoe Thanks for the latest version! The changes works great! One last thing you may want to add the tests into DiagrammedAssertionsSpec.scala also, which is a sibling tests for DiagrammedAssertions that calls the assert function through mixed in trait, instead of directly through Diagrams companion object.

@bvenners I have tested this with Scala 2.10, 2.13, 3.0, js and native.

@taisukeoe
Copy link
Contributor Author

@cheeseng
Thank you for confirmation. I have added tests to DiagramsSpec as well.

@bvenners bvenners merged commit e6e656b into scalatest:3.2.x-new Jun 21, 2021
@taisukeoe taisukeoe deleted the fix-diagrams-repeated-bug branch June 21, 2021 13:34
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.

None yet

4 participants