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

Changes to scalacheck to cross compile with Dotty #423

Open
wants to merge 8 commits into
base: master
from

Conversation

@allanrenucci
Copy link

commented Sep 14, 2018

I left out dotty-staging@00f01af on purpose because it is a binary incompatible change. Apart from that scalacheck cross compiles with Dotty

@allanrenucci allanrenucci force-pushed the dotty-staging:dotty-upstream branch 2 times, most recently from b71fb5f to 365a116 Sep 14, 2018

@allanrenucci

This comment has been minimized.

Copy link
Author

commented Sep 20, 2018

@rickynils Are you interested in these changes (or a subset)? Or should I close?

@allanrenucci allanrenucci force-pushed the dotty-staging:dotty-upstream branch from 365a116 to 1e1302c Sep 21, 2018

@er1c

This comment has been minimized.

Copy link

commented Apr 7, 2019

@rickynils this would be great! bump!

@smarter smarter force-pushed the dotty-staging:dotty-upstream branch from 1e1302c to d37b6c8 Jun 29, 2019

@smarter

This comment has been minimized.

Copy link
Contributor

commented Jun 29, 2019

All comments against this PR were addressed 9 months ago, and yet it wasn't merged or closed, what's the hold up ? I've now rebased this PR to fix conflicts, while I was at it I removed one change which is no longer needed (Seed#apply) and added two extra commits to let Dotty compile the code without -language:Scala2. Can I expect it to be merged this time around ?

@ashawley

This comment has been minimized.

Copy link
Collaborator

commented Jul 5, 2019

The Seed.apply change would have technically broken binary compatibility. I can't speak for anyone else, but I didn't finish reviewing the changes for that reason. Since it's now no longer needed, I suppose there's a chance that these fixes for Dotty could be included in a 1.14.1 bug fix release, IMO. With that in mind, I'll try to study the changes again and see if that's the case.

@ashawley

This comment has been minimized.

Copy link
Collaborator

commented Jul 8, 2019

Seems like all of this could be included a 1.14.1 bug fix release to me. I think the change to CmdLineParser is an internal change, so it shouldn't be of any consequence to the user.

@ashawley ashawley referenced this pull request Jul 8, 2019
16 of 18 tasks complete

@smarter smarter force-pushed the dotty-staging:dotty-upstream branch from d37b6c8 to 1194d67 Jul 9, 2019

@smarter

This comment has been minimized.

Copy link
Contributor

commented Jul 9, 2019

Are source-incompatible but binary-compatible changes allowed in the 1.14 branch ? One way to obtain the same result as dotty-staging@00f01af would be to simply remove the implicit from implicit def BooleanOperators(b: => Boolean): ExtendedBoolean, but existing code might have to replace import Prop.BooleanOperators by import Prop.propBoolean. If that's not OK, then is there a plan to create a branch for the development of 1.15 ?

@ashawley

This comment has been minimized.

Copy link
Collaborator

commented Jul 9, 2019

As described, that sounds like a change like that would be pretty disruptive to users in a 1.14.1 release.

I wonder if you should create an issue (or issues) to discuss the various Dotty changes you need in ScalaCheck. One assumes there will need to be a lot of coordination and investigation.

@smarter

This comment has been minimized.

Copy link
Contributor

commented Jul 9, 2019

Besides this PR, the BooleanOperators thing is the only necessary change I'm aware of, we switched the Functional Programming course at EPFL to Dotty last year, the course introduces students to property checking with scalacheck (like the mooc), and those changes were the only one we needed to get everything working.

@smarter

This comment has been minimized.

Copy link
Contributor

commented Jul 30, 2019

Anything else needed to get this merged ?

smarter added a commit to smarter/scalacheck that referenced this pull request Aug 7, 2019

Deprecate Prop.BooleanOperators in anticipation of Dotty
For a Boolean b and a prop p, the following is ambiguous in dotty:

    import Prop._
    b ==> p

Because Prop contains:

    implicit def BooleanOperators(b: => Boolean) = new ExtendedBoolean(b)
    implicit def propBoolean(b: Boolean): Prop = Prop(b)

And both ExtendedBoolean and Prop provide a `==>` method, the one in
`ExtendedBoolean` just forwards to the one in `Prop`. This is not
ambiguous in scalac because `Boolean` wins against `=> Boolean` but it
is in dotty (and intentionally so).

In general, it seems that all the methods in BooleanOperators are also
available on Prop, so BooleanOperators does not serve any purpose and
can be deprecated. We can then make it non-implicit in a subsequent
release of scalacheck (thus breaking source-compatiblity but not
binary-compatiblity) to finally be able to compile it with Dotty (which
also requires getting typelevel#423 in).
@smarter

This comment has been minimized.

Copy link
Contributor

commented Aug 7, 2019

One way to obtain the same result as dotty-staging/scalacheck@00f01af would be to simply remove the implicit from implicit def BooleanOperators(b: => Boolean): ExtendedBoolean

As a first step towards this, I've opened a PR to deprecate BooleanOperators: #498

@ashawley

This comment has been minimized.

Copy link
Collaborator

commented Aug 20, 2019

Unfortunately, we merged #498 first so there's a conflict. Fortunately, the conflict is just a single line. Would you be able to rebase? I also wonder if the new version of MiMa raises anything else.

smarter added some commits Jun 29, 2019

Use implicit function parameters explicitly
Implicitly available functions do not provide implicit views in Dotty,
so we need to call them explicitly.

(Their long term replacement is documented in
http://dotty.epfl.ch/docs/reference/contextual/conversions.html but
cannot be used when cross-compiling with Scala 2 currently).
Workaround bad implicit conversion
`prettyAny` defines an implicit conversion from `Any` to `Pretty`, this
is really dangerous in general. Here the problem is that `Pretty` has a
`map` member, so Dotty ends up choosing this conversion when we call
`map` on an array. We work around this by first converting the array to
a list, but the implicit conversions in this file should really be
rethought.

@smarter smarter force-pushed the dotty-staging:dotty-upstream branch from 1194d67 to b06de52 Aug 20, 2019

@smarter

This comment has been minimized.

Copy link
Contributor

commented Aug 20, 2019

Rebased.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.