Skip to content

Increment scala to 2.11.12, 2.12.4 and 2.13.0-M3 #186

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

Merged
merged 3 commits into from
Feb 2, 2018

Conversation

ashawley
Copy link
Member

@ashawley ashawley commented Feb 1, 2018

No description provided.

@ashawley
Copy link
Member Author

ashawley commented Feb 1, 2018

Failed with

sbt.ResolveException: unresolved dependency: org.scala-js#scalajs-library_2.13.0-M3;0.6.19: not found

@SethTisue
Copy link
Member

you need Scala.js 0.6.22

@ashawley
Copy link
Member Author

ashawley commented Feb 1, 2018

Ok, giving it a try.

And...

> ++2.13.0-M3
> xmlJS/testOnly scala.xml.UtilityTest
[error] Test scala.xml.UtilityTest.trim failed: expected: java.lang.Long<1> but was: java.lang.Byte<1>, took 0.001000192 sec

This doesn't ring a bell for me.

@SethTisue
Copy link
Member

SethTisue commented Feb 1, 2018

nor I. does it ring a bell for @sjrd?

  @Test
  def trim: Unit = {
    val x = <foo>
                 <toomuchws/>
              </foo>
    val y = xml.Utility.trim(x)
    assertEquals(1, y match { case <foo><toomuchws/></foo> => 1 })

    val x2 = <foo>
      <toomuchws>  a b  b a  </toomuchws>
    </foo>
    val y2 = xml.Utility.trim(x2)
    assertEquals(2, y2 match { case <foo><toomuchws>a b b a</toomuchws></foo> => 2 })
  }

@sjrd
Copy link
Member

sjrd commented Feb 1, 2018

Er ... doesn't look like anything to me :s

It is true that, according to JUnit which uses Java-s equals, a Long 1 is not equal to a Byte 1. Under Scala.js, it is also correct that it reports a Byte 1 and not an Integer 1 because its run-time value fits in the range of Byte.

What is not normal is that it expected a Long. It should have expected a Byte, given the source code of the test. Why was it converted to a Long? I have no idea.

The first thing to try to diagnose the issue would be

> xmlJS/test:scalajsp scala/xml/UtilityTest.sjsir

(the command supports auto-completion)
in order to determine if the IR is correct. There should be 1 (and not 1L) as first argument to the assertEquals.

If the IR is correct, it is probably on me. If the IR is incorrect, try compiling with -Xprint:mixin. If that one is correct, it is also probably on me. If it's incorrect, which I selfishly hope, it's scalac doing something weird and adapting that Int to a Long for some reason.

That said, if it's scalac's fault, I don't know how the test passes on the JVM :s

@sjrd
Copy link
Member

sjrd commented Feb 1, 2018

Actually, did the test pass on the JVM? I don't see it in the logs for JVM+2.13.0-M3 (but I'm on mobile, so I might have missed it).

@ashawley
Copy link
Member Author

ashawley commented Feb 1, 2018

Yes, the test works in the JVM.

Seems the test passes with 0.6.22 with 2.13.0-M2, but fails in 2.13.0-M3.

Here's how the intermediate representation changes in 2.13.0-M3 from 2.13.0-M2:

https://gist.github.com/ashawley/0b63bdcc8cd0f862cfaf0a3851f80211/revisions

@sjrd
Copy link
Member

sjrd commented Feb 1, 2018

I'll have a look when I get to a computer.

@sjrd
Copy link
Member

sjrd commented Feb 1, 2018

Looks like a Scala.js bug, in the sense that the -Xprint:mixin looks legit, and that the IR does not seem to match it (there's a missing primitive conversion). I need more time to isolate it completely.

In the meantime, you can unblock this PR with a workaround: write 1L and 2L explicitly after the => in the match (instead of 1 and 2).

@sjrd
Copy link
Member

sjrd commented Feb 1, 2018

Well ... that one was really really weird. Minimization and full analysis here: scala-js/scala-js#3281.

We will fix that in 0.6.23, but it will be at least 6 weeks away from now. I strongly recommend you work around the bug with the explicit 1L and 2L, as I explained in my previous comment.

@ashawley
Copy link
Member Author

ashawley commented Feb 1, 2018

Happy to fix the test with a workaround, since it's silly test as written, anyway. And if you're tracking down the defect upstream, I'll destroy the evidence here.

You're a real übermensch and thanks again.

Also fixes defect in scalajs 0.6.22 and scala 2.13.0-M3
@ashawley ashawley merged commit 44861f0 into scala:master Feb 2, 2018
@ashawley ashawley deleted the scala-2.13.0-M3 branch February 2, 2018 01:11
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.

3 participants