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

Discussion: what should we do with DottyPredef before the release? #10294

Closed
anatoliykmetyuk opened this issue Nov 12, 2020 · 8 comments · Fixed by #10428
Closed

Discussion: what should we do with DottyPredef before the release? #10294

anatoliykmetyuk opened this issue Nov 12, 2020 · 8 comments · Fixed by #10428
Assignees
Milestone

Comments

@anatoliykmetyuk
Copy link
Contributor

anatoliykmetyuk commented Nov 12, 2020

This issue is the place to discuss what needs to be done about DottyPredef. DottyPredef is a workaround used to include stuff in the standard Predef. Standard Predef comes from the Scala 2 standard library. Obviously, it would be good to have the contents of the DottyPredef moved inside Predef. But it is not clear how we should proceed with it – now and in the future.

Given the compressed timeframe we have for the Scala 3.0.0 RC1 Release, it would be good to come up with some conclusion on this one by the end of the next week so that we have enough time to act upon the decision.

/cc @nicolasstucki @odersky @sjrd @smarter

@odersky
Copy link
Contributor

odersky commented Nov 13, 2020

See also #10302

@sjrd
Copy link
Member

sjrd commented Nov 13, 2020

And #10303.

@odersky
Copy link
Contributor

odersky commented Nov 13, 2020

So looking at the early results of these PRs, we seem to be left with the following essential methods

  • assert
  • valueOf
  • summon
  • nn

All but nn are simple inline methods. They should be relatively straightforward to add to Predef, or to special case in the compiler. I am not sure what to do about nn yet. Make it an inline method that forwards to a method in scala.runtime, maybe?

@odersky
Copy link
Contributor

odersky commented Nov 13, 2020

Btw the reason to make assert transparent is that we get better code out of it. Here's the translation of
assert(x > 0) with transparent assert:

        if x().>(0).unary_!() then assertFail() else ()

With normal inline assert:

        (if x().>(0).unary_!() then assertFail() else 
          scala.runtime.BoxedUnit$#UNIT
        ):scala.runtime.BoxedUnit

The reason for assertFail is that it's also shorter than throw new AssertionError. But that's maybe not so much of an issue. If we don't want an assertFail in Predef and do want to keep the byte-code concise, we could move it to runtime also.

@sjrd
Copy link
Member

sjrd commented Nov 13, 2020

Make it an inline method that forwards to a method in scala.runtime, maybe?

Yes, this is exactly what I had in mind.

If we're not too particular about the error message, we could also define it as:

  extension [T](x: T | Null) inline def nn: x.type & T =
    x.getClass() // throws NPE iff x is null; optimized by the JVM
    x.asInstanceOf[x.type & T]

@sjrd
Copy link
Member

sjrd commented Nov 13, 2020

The reason for assertFail is that it's also shorter than throw new AssertionError. But that's maybe not so much of an issue. If we don't want an assertFail in Predef and do want to keep the byte-code concise, we could move it to runtime also.

It's not that much more concise. For the assert(cond) (without message), it's 10 bytes instead of 7; and for the assert(cond, message), it's 10 bytes instead of 9.

@sjrd
Copy link
Member

sjrd commented Nov 13, 2020

Btw the reason to make assert transparent is that we get better code out of it. Here's the translation of
assert(x > 0) with transparent assert:

To me that looks more like a limitation of one of the transformation phases, perhaps erasure. Before erasure:

        // transparent:
        if cond.unary_! then throw new AssertionError("assertion failed") else
          ()
        // not transparent:
        (if cond.unary_! then
          throw new AssertionError("assertion failed: hello")
         else ()):Unit

After erasure:

        // transparent:
        if cond().unary_!() then throw new AssertionError("assertion failed")
           else
        ()
        // not transparent:
        (if cond().unary_!() then
          throw new AssertionError("assertion failed: hello")
         else scala.runtime.BoxedUnit$#UNIT):scala.runtime.BoxedUnit

Erasure or an earlier phase could replace Typed(expr, UnitTpe) by expr when it is already in statement position. Or simply by Block(expr, ()).

In any case, that does not seem like a good enough reason to use transparent, by far. We shouldn't use transparent just to get better code. We can improve the compilation pipeline instead.

@sjrd
Copy link
Member

sjrd commented Nov 19, 2020

Here is a PR to make nn an inline def: #10396

@odersky odersky linked a pull request Nov 23, 2020 that will close this issue
@Kordyjan Kordyjan added this to the 3.0.0 milestone Aug 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants