Skip to content

Compile with dotty #7

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 5 commits into from
Apr 11, 2017

Conversation

smarter
Copy link
Member

@smarter smarter commented Mar 28, 2017

This is all that's needed to make the backend part of the dotty bootstrap, however there are many files in the backend that should not be compiled by dotty at all, in my branch of dotty I ignore them using sbt filters:

      unmanagedSourceDirectories in Compile ++=
        (((baseDirectory.value / ".." / "scala-backend" / "src" / "compiler" / "scala" / "tools" / "nsc" / "backend") *
          (GlobFilter("*.scala") - "JavaPlatform.scala" - "Platform.scala")) +++
         ((baseDirectory.value / ".." / "scala-backend" / "src" / "compiler" / "scala" / "tools" / "nsc" / "backend" / "icode") **
          (NothingFilter || "Opcodes.scala" || "Primitives.scala")) +++
         ((baseDirectory.value / ".." / "scala-backend" / "src" / "compiler" / "scala" / "tools" / "nsc" / "backend" / "jvm") *
           (GlobFilter("*.scala") - "BCodeICodeCommon.scala"
             - "GenASM.scala" - "ScalacBackendInterface.scala"))
        ).get,

An alternative would be to simply completely comment out the content of files that do not compile with dotty, this would make it easier to compile dotty without sbt since you could just pass it scala-backend/src/compiler/scala/tools/nsc/backend/**/*.scala instead of having to reproduce the filter.

`interface` needs to be accessible, otherwise Dotty will complain with
messages like:
non-private method enclosingMethodAttribute refers to private value interface
 in its type signature (classSym: BCodeAsmCommon.this.interface.Symbol, classDesc: BCodeAsmCommon.this.interface.Symbol => String, methodDesc:
  BCodeAsmCommon.this.interface.Symbol
 => String)Option[BCodeAsmCommon.this.EnclosingMethodEntry]
@smarter smarter requested a review from DarkDimius March 28, 2017 12:08
@smarter smarter force-pushed the compile-with-dotty branch from fe658aa to 4fda559 Compare March 28, 2017 12:10
@DarkDimius
Copy link

You are commenting out quite some files from this repo. I'd prefer to keep them so that we could still build a usable-scalac out of it. It used to help me a lot when debugging the difference between Dotty & scalac.

@@ -23,7 +23,7 @@ import scala.tools.nsc.backend.icode.Primitives.{NE, EQ, TestOp, ArithmeticOp}
*/
trait BCodeIdiomatic {
val int: BackendInterface
lazy val bTypes = new BTypesFromSymbols[int.type](int)
final lazy val bTypes = new BTypesFromSymbols[int.type](int)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why final?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I got that from @felixmulder I think, do you recall why this makes a difference?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, IIRC this was something fundamental to Dotty that we couldn't get around. I ended up asking @odersky who said it should be final.

Compiling without it will give you an error message - if you can compile it without final, that'd jog my memory.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My guess would be scala/scala3#50 & scala/scala3#1051

@@ -27,7 +27,8 @@ class BTypesFromSymbols[I <: BackendInterface](val int: I) extends BTypes {
import bCodeAsmCommon._

// Why the proxy, see documentation of class [[CoreBTypes]].
val coreBTypes = new CoreBTypesProxy[this.type](this)
// Dotty deviation: This type signature should nto be needed.
val coreBTypes: CoreBTypesProxy[this.type] = new CoreBTypesProxy[this.type](this)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

would you mind reporting all deviations?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, I wanted to minimize them first, but maybe that's not needed.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

@smarter
Copy link
Member Author

smarter commented Mar 28, 2017

You are commenting out quite some files from this repo. I'd prefer to keep them so that we could still build a usable-scalac out of it. It used to help me a lot when debugging the difference between Dotty & scalac.

This could be done by splitting the companion objects in separate file and not compiling the files with Global in dotty, but this would make the diff with scalac larger, I'm fine with doing it that way if you want.

@smarter smarter force-pushed the compile-with-dotty branch from 4fda559 to 81896f7 Compare March 28, 2017 12:22
Otherwise with dotty the signature of the overloaded definition will be used.
@smarter smarter force-pushed the compile-with-dotty branch from 81896f7 to 727467b Compare March 28, 2017 14:45
smarter added 2 commits April 3, 2017 18:52
Otherwise the lazy overload in BTypesFromSymbols will be rejected by
dotty without -language:Scala2.
@smarter smarter force-pushed the compile-with-dotty branch from 727467b to 1cc3419 Compare April 3, 2017 17:45
@smarter
Copy link
Member Author

smarter commented Apr 3, 2017

@DarkDimius I just updated the PR so that it still compiles with scalac (by setting a published version number in version.properties and doing "ant build"). Once this is in I'll make a second PR to fix procedure syntax warnings and this should be all that's needed for the hyper bootstrap.

Some classes cannot be compiled as part of dotty since we don't compile
Global, but we still need to compiler their companion objects because
they are referenced in other parts of the backend. We address this by
moving these objects in separate files that can be compiled by both
scalac and dotty, whereas the part that depend on Global will not be
compiled by dotty, this is achieved by using filters in sbt:

unmanagedSourceDirectories in Compile ++= {
  val backendDir = baseDirectory.value / ".." / "scala-backend" / "src" / "compiler" / "scala" / "tools" / "nsc" / "backend"
  val allScalaFiles = GlobFilter("*.scala")
  ((backendDir *
    (allScalaFiles - "JavaPlatform.scala" - "Platform.scala" - "ScalaPrimitives.scala")) +++
   (backendDir / "jvm") *
    (allScalaFiles - "BCodeICodeCommon.scala" - "GenASM.scala" - "GenBCode.scala" - "ScalacBackendInterface.scala")
  ).get
},

Summary of the changes:
- ScalaPrimitives is split into ScalaPrimitivesOps (shared) and
  ScalaPrimitives (scalac-only)
- jvm.GenBCode is split into jvm.GenBCodeOps (shared) and
  jvm.GenBCode (scalac-only)
- icode.Opcodes is split into jvm.Opcodes (shared) and
  icode.Opcodes (scalac-only)
- icode.Primitives is split into jvm.Primitives (shared) and
  icode.Primitives (scalac-only)
@smarter smarter force-pushed the compile-with-dotty branch from 1cc3419 to 6539dac Compare April 3, 2017 18:09
package tools.nsc
package backend

object ScalaPrimitivesOps extends ScalaPrimitivesOps

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you'll switch names between object that are currently named ScalaPrimitives and ScalaPrimitivesOps most of the need for api changes will be gone. While this messes up the name, I think it's worth it both for API compatibility and for binary compatibility.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is still an object ScalaPrimitives for compatibility: https://github.com/DarkDimius/scala/pull/7/files#diff-859cee54c124f77e4b12fe371a1198a6R411, what else is needed?

@@ -28,8 +28,8 @@ abstract class ConstantOptimization extends SubComponent {
import global._
import icodes._
import icodes.opcodes._
import scala.tools.nsc.backend.icode.Primitives._
import scala.tools.nsc.backend.icode.Opcodes._
import scala.tools.nsc.backend.jvm.Primitives._

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why did you move them? with the same reasoning, I believe we should keep them where they are now.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

icode.Primitives contained both a class and an object, I cannot compile the class with dotty and I need to compile the object, so I had to move the object. I decided to move it to jvm.Primitives instead of something like icode.PrimitivesOps because that way I don't need to compile anything from the icode subdirectory which makes the selection with filters of files compiled by dotty simpler.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let me think about it more.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I really really would like to avoid that, it's very weird, it may break stuff, it's evil. Also I don't see what difference it would make, you still need to split things into different files with different names.


import scala.tools.asm

object GenBCodeOps extends GenBCodeOps

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same with this refactoring.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As in the other cases, I needed to compile the object but not the class, but kept a compatibility object GenBCode extends GenBCodeOps

Copy link

@DarkDimius DarkDimius left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See above

@smarter
Copy link
Member Author

smarter commented Apr 10, 2017

Let me think about it more.

Any more thoughts on this? Would be good to know how to proceed since this is blocking the hyper bootstrap which is in turn making it harder for Martin to do the changes he wants to do in the backend.

@DarkDimius
Copy link

After followup discussion we decided that we'll get this in as is.

@DarkDimius DarkDimius merged commit d74e977 into lampepfl:sharing-backend Apr 11, 2017
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