Skip to content

Dotty - #1067

Merged
cquiroz merged 78 commits into
typelevel:mainfrom
cquiroz:dotty
Oct 19, 2021
Merged

Dotty#1067
cquiroz merged 78 commits into
typelevel:mainfrom
cquiroz:dotty

Conversation

@cquiroz

@cquiroz cquiroz commented Sep 16, 2021

Copy link
Copy Markdown
Collaborator

Scala 3 port

This PR adds support for scala3 while still cross compiling to scala 2.13.
It is a fairly complex PR due to the structure of spire and its extensive use of techniques such as macros and typeclass based syntax that need to be changed for scala 3

The aim is to have a cross compiled version with no or minimal changes to the API and keep a reasonable performance
The mark of success will be passing all (or most tests) while the API remains mostly unchanged.
While this is totally usable we may need to do some further refinements after this is merged

General changes:

  • do/while constructs were converted to while
  • Implicits need type ascription
  • Some number conversions need to be explicit
  • Remove or rewrite the macros

API changes

In general we want clients not needing to change their code at all, however
in some cases it was unavoidable:

  • the method Dist.given clashes with the given keyword, it was renamed to given_
  • the class DistIterator has a next member that clashes with a next method. The field has renamed to _next
  • UnboundSyntax seems to be bogus and it is not used anywhere in spire. It was marked as deprecated in scala 2 and removed in scala 3
  • It is not in this PR yet but we should change all the empty paren methods to no-parens

Syntax changes

spire contains a fairly large set of syntaxis to allow a variety of operations. These use macros and needed to be rewritten.

As part of the process I changed them to be mostly extension methods.
However this changes the priorities as seen here:
scala/scala3#8740

I discovered this late in the process as it only has effects in some cases where all syntax is
imported and there are multiple implementations of the same operator.

I propose to test the approach in this PR but could be reverted if necessary

Macros

Spire includes a fair amount of macros many of them fairly complex.
As a first approximation I applied this policy about macros:

Only port macros that are strictly necessary to keep the semantics not the ones related to performance:

Under that criteria the action for each macro is as follow:

  • Checked: This is likely the most complicated macro. It can take an arbitrary expression and check if any of
    the int/long operations would cause an overflow. It is hard to replace this with a non macro as it is supposed
    to operate with any arbitrary expression.
    In a first run Checked.tryOrReturn was removed as it can't be expressed as a quasiquote and in general it
    is hard to reason about when used
    Uses of tryOrReturn have been rewritten with the general form of
try {
  Checked.checked(a)
} catch {
  case _: ArithmeticException => return b
}
  • PackMacro: This are fairly simple thus were converted using scala3 inline. Note that part of
    Pack is now shared with the macros being scala version specific

  • Machinist: They are no longer used in scala 3. All calls that previously used machinist now are just regular typeclasses calls

  • cFor: A base implementation was done at https://github.com/bishabosha/spire-scala3 and it was used
    as the basis for the port. See https://github.com/bishabosha/spire-scala3/blob/develop/src/main/scala/spire/syntax/macros/cforMacros.scala
    Note a cFor test is not passing thus a bit of work is still needed but it doesn't affect other uses

  • Fpf: This class contains several macros which have not been ported and it remains scala 2.x only.
    I couldn't find it used anywhere except in test thus impact should be limited.
    Still it could be ported later as the macros are not very complicated.

  • Auto: This is left as 2.x only

  • Literals: Literals are specified as failing at compilation time thus a macro was needed. The current one was ported to scala 3 and the old versions kept for scala 2.
    NOTE: radix literals are not tested and not yet ported

Scalafmt

Ideally we'd update scalafmt to 3.0.0 but that would produce an even larger PR, thus I propose it is done in a later stage

Performance

Note that performance will not be the same as in scala 2. This is in part because scala 3 doesn't
support specialization and due to the replacement of some macros.

Some quick benchmarking shows no important difference for code not using specialization

Keeping the same level of performance is a non goal of this PR but could be improved later

cquiroz and others added 30 commits September 12, 2021 16:43
Signed-off-by: Carlos Quiroz <3615303+cquiroz@users.noreply.github.com>
Signed-off-by: Carlos Quiroz <carlos.m.quiroz@gmail.com>
Signed-off-by: Carlos Quiroz <3615303+cquiroz@users.noreply.github.com>
Signed-off-by: Carlos Quiroz <3615303+cquiroz@users.noreply.github.com>
Signed-off-by: Carlos Quiroz <3615303+cquiroz@users.noreply.github.com>
Signed-off-by: Carlos Quiroz <3615303+cquiroz@users.noreply.github.com>
Signed-off-by: Carlos Quiroz <3615303+cquiroz@users.noreply.github.com>
Signed-off-by: Carlos Quiroz <3615303+cquiroz@users.noreply.github.com>
Signed-off-by: Carlos Quiroz <3615303+cquiroz@users.noreply.github.com>
Signed-off-by: Carlos Quiroz <3615303+cquiroz@users.noreply.github.com>
Signed-off-by: Carlos Quiroz <3615303+cquiroz@users.noreply.github.com>
Signed-off-by: Carlos Quiroz <3615303+cquiroz@users.noreply.github.com>
Signed-off-by: Carlos Quiroz <3615303+cquiroz@users.noreply.github.com>
Signed-off-by: Carlos Quiroz <3615303+cquiroz@users.noreply.github.com>
Signed-off-by: Carlos Quiroz <3615303+cquiroz@users.noreply.github.com>
Signed-off-by: Carlos Quiroz <3615303+cquiroz@users.noreply.github.com>
w
Signed-off-by: Carlos Quiroz <3615303+cquiroz@users.noreply.github.com>
Signed-off-by: Carlos Quiroz <3615303+cquiroz@users.noreply.github.com>
Signed-off-by: Carlos Quiroz <3615303+cquiroz@users.noreply.github.com>
Signed-off-by: Carlos Quiroz <3615303+cquiroz@users.noreply.github.com>
Signed-off-by: Carlos Quiroz <3615303+cquiroz@users.noreply.github.com>
Signed-off-by: Carlos Quiroz <3615303+cquiroz@users.noreply.github.com>
Signed-off-by: Carlos Quiroz <3615303+cquiroz@users.noreply.github.com>
Signed-off-by: Carlos Quiroz <3615303+cquiroz@users.noreply.github.com>
Signed-off-by: Carlos Quiroz <3615303+cquiroz@users.noreply.github.com>
Signed-off-by: Carlos Quiroz <3615303+cquiroz@users.noreply.github.com>
Signed-off-by: Carlos Quiroz <3615303+cquiroz@users.noreply.github.com>
Signed-off-by: Carlos Quiroz <3615303+cquiroz@users.noreply.github.com>
Signed-off-by: Carlos Quiroz <3615303+cquiroz@users.noreply.github.com>
Signed-off-by: Carlos Quiroz <3615303+cquiroz@users.noreply.github.com>
@armanbilge armanbilge mentioned this pull request Oct 18, 2021
@armanbilge

Copy link
Copy Markdown
Member

Can we bump to 3.1.0? 😅

Improve `cfor` / `fastFor` semantics, docs
package benchmark

import spire.implicits._
import spire.syntax.std.long._

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I'm very sorry if I asked before and forgot :( did all of these imports really have to change? I feel surprised, and it seems concerning.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

In the original PR there were some clashes due to the way priorities are set using implicit conversions compared to extension methods. That forced the change but in principle all could be reverted

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Yeah, if you don't mind I'm currently working on reducing the diff for these. Instead of asking a question every time, I figure I better just try reverting :)

def :*[F](rhs: Int)(implicit ev: RightModule[V, F], F: Ring[F]): V = ev.timesr(x, F.fromInt(rhs))
}

@deprecated("Unbound syntax will be removed", "spire 0.18.0")

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Sorry if I forgot, what are unbound syntax?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

It is a set of conversions classes like:
https://github.com/typelevel/spire/blob/main/core/src/main/scala/spire/syntax/Ops.scala#L549

final class ActionUnboundOps[G: ({ type F[A] = Action[_, A] })#F](lhs: G) {
  def |+|(rhs: G): G = macro Ops.binopWithScalar[G, G]
  def |-|(rhs: G): G = macro Ops.binopWithScalar[G, G]
  def inverse(): G = macro Ops.unopWithScalar[G]
}

These are not used anywhere and IMHO are bogus especially because lhs is G. IMHO they are just a mistake that compiles because it goes through the macro and they are never expanted

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I don't really understand but that's fine it can go away 😆

Comment on lines 1352 to 1353
// TODO Restore the checked call
def apply(expr: Algebraic.Expr): Bound = checked {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

we can remove this?


@tailrec final def gcdSpire[@sp(Int) A](a: A, b: A)(implicit ev1: EuclideanRing[A], ev2: Eq[A]): A = {
import spire.implicits._
import spire.syntax.euclideanRing._

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

This change was needed as there is b function in implicits as indicated here:
scala/scala3#13602

It can be fixed renaming the b param

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Yup I caught that one.

@armanbilge armanbilge left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I'm 👍 after we merge #1079.

armanbilge and others added 4 commits October 19, 2021 01:13
Signed-off-by: Carlos Quiroz <3615303+cquiroz@users.noreply.github.com>
Signed-off-by: Carlos Quiroz <3615303+cquiroz@users.noreply.github.com>
Signed-off-by: Carlos Quiroz <3615303+cquiroz@users.noreply.github.com>
@cquiroz

cquiroz commented Oct 19, 2021

Copy link
Copy Markdown
Collaborator Author

Added a few small fixes and updated scala to 3.1.0

@i10416 i10416 mentioned this pull request Oct 19, 2021

@armanbilge armanbilge left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

👍 when green!

@vasilmkd

Copy link
Copy Markdown
Member

Let's go! 🎉

@cquiroz
cquiroz merged commit 34a3787 into typelevel:main Oct 19, 2021
@cquiroz
cquiroz deleted the dotty branch October 19, 2021 16:24
@SethTisue

Copy link
Copy Markdown
Member

👏👏👏

This was referenced Oct 19, 2021
@som-snytt

Copy link
Copy Markdown

congrats! it's fun to watch people undertake daredevil stunts.

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.

6 participants