update: use java.util.BitSet on js platform #384

i10416 · 2022-02-18T11:12:05Z

close #381
scala.js supports java.util.BitSet since 1.9.0

scala.js supports java.util.BitSet since 1.9.0 https://www.scala-js.org/news/2022/02/14/announcing-scalajs-1.9.0/

johnynek · 2022-02-18T18:08:52Z

I don't think we can merge this and #376.

Also the tests will be very slow if we don't have the switch for isScalaJs.

Can scala-native handle Java util Bitset?

Finally, this will be binary incompatible.

johnynek · 2022-02-18T18:09:24Z

But thanks for the PR!!

Hopefully we can merge some version.

i10416 · 2022-02-19T07:36:08Z

Thank you for review. I will tweak what you pointed out.

I don't think we can merge this and #376

Yes, I think too.
I planed to share source between jvm/js first, then tweak #376

Can scala-native handle Java util Bitset?

AFAIK, scala native does not support java.util.BitSet :(

i10416 · 2022-02-19T07:58:49Z

Also the tests will be very slow if we don't have the switch for isScalaJs.

I think it would be better to switch scala.js/jvm tests based on build settings (e.g. generate util object as sbt-buildinfo do ) rather than BitSetUtil fields. What do you think?

i10416 · 2022-02-19T09:08:48Z

Ah, I found portable-scala/sbt-crossproject#17. IIUC, we need to copy-paste BitSetUtil between jvm and js to add native platform impl later?

armanbilge · 2022-02-19T09:24:42Z

@i10416 I'm actually about to submit a PR to fix portable-scala/sbt-crossproject#17 :)

AFAIK, scala native does not support java.util.BitSet :(

That's true, but now that Scala.js does you can probably just copy-paste the implementation and tests and make a PR to native. That's how many things get ported to native :)

I think it would be better to switch scala.js/jvm tests based on build settings

I think if you have a dependency on cats-laws in the tests you can just use this:

https://github.com/typelevel/cats/blob/b307688c391a47dc6dd82d42c15edf273bf2471b/kernel-laws/jvm/src/main/scala/cats/platform/Platform.scala

armanbilge · 2022-02-19T12:50:39Z

Port java.util.BitSet scala-native/scala-native#2565

i10416 · 2022-02-19T17:21:11Z

tests will be very slow if we don't have the switch for isScalaJs.

Now that test iteration is switched based on js/jvm, what to do are

merge update: use java.util.BitSet on js platform #384 in future release(or is there a nice trick to keep bincompat?)
wait for Port java.util.BitSet scala-native/scala-native#2565 to be merged/released(thx @armanbilge )
then tweak and finish add native build #376.

codecov-commenter · 2022-02-20T18:39:24Z

Codecov Report

Merging #384 (e7bf32b) into main (db5c3df) will decrease coverage by 0.16%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main     #384      +/-   ##
==========================================
- Coverage   96.67%   96.51%   -0.17%     
==========================================
  Files           9        9              
  Lines        1263     1261       -2     
  Branches      122      123       +1     
==========================================
- Hits         1221     1217       -4     
- Misses         42       44       +2

Impacted Files	Coverage Δ
...hared/src/main/scala/cats/parse/BitSetCompat.scala	`100.00% <100.00%> (ø)`
...shared/src/main/scala/cats/parse/Accumulator.scala	`97.56% <0.00%> (-2.44%)`	⬇️
core/shared/src/main/scala/cats/parse/Parser.scala	`96.25% <0.00%> (-0.29%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update db5c3df...e7bf32b. Read the comment docs.

johnynek · 2022-02-20T18:44:56Z

I think it is reasonable to release 0.4.0 and break this minor binary compatibility (isScalaJs, isScalaJvm)...

Those really should have been made private [cats.parse] anyway...

armanbilge · 2022-02-20T18:50:55Z

FWIW I'd appreciate not bumping and preserving binary-compatibility here instead. It makes downstream cross-builds slightly more complicated when the different platforms are pinned on different versions of the same dependency (I'm thinking about http4s).

Apologies if I am missing something, but why is it so hard to preserve bincompat here? Or is it just undesirable?

johnynek · 2022-02-20T18:55:09Z

@armanbilge I'm confused by what you mean.

The only real break to binary compatibility is the symbols isScalaJvm and isScalaJs. I bet no one is actually using those. So, any real dependency won't be broken.

The other errors are for items inside the private object Impl which no one can see without doing some weird reflection hacks, which we don't promise to support.

Do you think there are downstream users that care about the first two?

Or is it something about using scalajs 1.9.0?

armanbilge · 2022-02-20T18:59:47Z

FTR I completely agree that its unlikely that removing those will break any downstreams.

However, it's the version bump that got my attention: it will force downstreams to also bump their versions to take on this update.

johnynek · 2022-02-20T19:16:11Z

okay, fair point.

Let's add the isScalaJvm, isScalaJs back even though it is inelegant. We can add exclusions for the changes inside private object Impl.

i10416 · 2022-02-25T07:44:02Z

Thank you for reviewing. I will put isScalaJvm and isScalaJs back.

Removing isScalaJs/isScalaJVM breaks binary compatibility. It is unlikely that anyone uses this API, but in order to avoid bumping minor version, we put platform check fields back to BitSetUtil for now.

i10416 · 2022-03-02T17:28:46Z

core/shared/src/main/scala/cats/parse/BitSet.scala

 object BitSetUtil {
  type Tpe = BitSet
-
+  @deprecated("", "0.3.7")


I changed here to isScalaJVM =false , isScalaJs = false because I think combination of isScalaJVM =true, isScalaJs = false, isScalaJVM =false, isScalaJs = true or isScalaJVM =true, isScalaJs = true does not make sense.

well, I don't want to have a value that is false. This will be very confusing and more of a wart, if you ask me.

Can we make this the same as it was?

instead we could move the BitSet code to

private[parse] abstract class BitSetImpl { type Tpe = BitSet //... }

then have the js and jvm code just extend BitSetImpl something like that would maintain binary compatibility but also not have any falsehoods in the return values.

instead we could move the BitSet code to ...

Yeah, actually, I thought my idea was not nice too😓

That seems better solution! Thanks!

i10416 · 2022-03-02T17:30:58Z

Mima reports binary compatibility issues due to change from scala.collection.mutable.BitSet to java.util.BitSet but it is ok to suppress them, isn't it?

i10416 · 2022-03-02T17:31:49Z

The other errors are for items inside the private object Impl which no one can see without doing some weird reflection hacks, which we don't promise to support.

Ah, I overlooked this.

i10416 · 2022-03-02T17:49:33Z

Hmmm, doesn't Mima support exclude rules like cats.parse.Parser#Impl.* ?

core/shared/src/test/scala/cats/parse/LocationMapTest.scala

i10416 · 2022-03-04T06:28:52Z

I removed deprecated annotations, which I added just in case though I didn't think someone uses isScalaJs and isScalaJvm api, because I assume you don't like warning on the ground that you regard warning as fatal when checking source before marge (while I felt it makes sense to show deprecated warning).

i10416 · 2022-03-04T06:34:52Z

There remain some mima warnings such as method bitSetFor(Array[Char])scala.collection.mutable.BitSet in object cats.parse.BitSetUtil has a different result type in current version, where it is java.util.BitSet rather than scala.collection.mutable.BitSet filter with: ProblemFilters.exclude[IncompatibleResultTypeProblem]("cats.parse.BitSetUtil.bitSetFor").

Should I suppress them too for now? If we would suppress them, I think we would need to remove the exclude rules in a future release as this change is from 0.3.6 to 0.3.7 only.

core/shared/src/test/scala/cats/parse/BitSetTest.scala

johnynek · 2022-03-05T18:10:12Z

yeah, we can suppress those errors.

They are only for scalajs and they are only if for some weird reason users accessed that BitSet abstraction code, and I don't know why they would. So, I would bet it will impact exactly zero users.

Also, let's add an issue to remove isScalaJs/isScalaJvm when we release a breaking change.

i10416 · 2022-03-16T08:00:06Z

@johnynek
Can I ask you to review this PR?

Just a reminder. I'm not in a hurry, of course :)

johnynek · 2022-03-16T15:20:54Z

Sorry I missed that you had updated this. It looks good to me. If CI is green I'll merge. Thanks so much for working so much to get this done within the binary compatibility constraints.

i10416 · 2022-03-16T16:10:18Z

Sooooooorry! I will fix this by tommow! 🙇

johnynek · 2022-03-16T20:58:57Z

huh... the CI does not seem to be running.

i10416 · 2022-03-17T08:49:40Z

Strang...

johnynek · 2022-03-17T15:54:43Z

Thanks for pushing this one through!

update: use java.util.BitSet on js platform

e85b165

scala.js supports java.util.BitSet since 1.9.0 https://www.scala-js.org/news/2022/02/14/announcing-scalajs-1.9.0/

i10416 force-pushed the use-java-bitset-from-scalajs-1.9.0#381 branch from 620998f to e85b165 Compare February 18, 2022 11:13

switch test iteration based on build platform

aad44b1

put platform check fields back for bincompat

7c6097e

Removing isScalaJs/isScalaJVM breaks binary compatibility. It is unlikely that anyone uses this API, but in order to avoid bumping minor version, we put platform check fields back to BitSetUtil for now.

i10416 commented Mar 2, 2022

View reviewed changes

exclude internal api from bincompat issue reports

8a4c6e6

i10416 force-pushed the use-java-bitset-from-scalajs-1.9.0#381 branch from 971905d to 8a4c6e6 Compare March 2, 2022 18:11

resolve conflict

bc9599c

i10416 force-pushed the use-java-bitset-from-scalajs-1.9.0#381 branch from c832d10 to bc9599c Compare March 2, 2022 18:24

i10416 commented Mar 2, 2022

View reviewed changes

core/shared/src/test/scala/cats/parse/LocationMapTest.scala Outdated Show resolved Hide resolved

i10416 force-pushed the use-java-bitset-from-scalajs-1.9.0#381 branch from 8b8017e to 896ad32 Compare March 3, 2022 03:47

i10416 requested a review from johnynek March 3, 2022 03:49

tweak

3168416

i10416 force-pushed the use-java-bitset-from-scalajs-1.9.0#381 branch from 896ad32 to 3168416 Compare March 4, 2022 06:16

johnynek reviewed Mar 5, 2022

View reviewed changes

core/shared/src/test/scala/cats/parse/BitSetTest.scala Show resolved Hide resolved

i10416 added 2 commits March 9, 2022 20:31

Merge branch 'main' into use-java-bitset-from-scalajs-1.9.0#381

b74a942

rebase change

cbdddc2

i10416 mentioned this pull request Mar 9, 2022

Remove isScalaJs/isScalaJvm on minor version update #391

Open

supress bincompat wornings for bitsetutils

2859f0c

i10416 requested a review from johnynek March 9, 2022 12:14

Merge branch 'main' into use-java-bitset-from-scalajs-1.9.0#381

74baf57

johnynek approved these changes Mar 16, 2022

View reviewed changes

update mima rules

e7bf32b

i10416 force-pushed the use-java-bitset-from-scalajs-1.9.0#381 branch from c755b58 to e7bf32b Compare March 17, 2022 08:52

i10416 requested a review from johnynek March 17, 2022 08:54

johnynek merged commit 2d8e31c into typelevel:main Mar 17, 2022

i10416 mentioned this pull request Mar 18, 2022

add native build #376

Closed

Uh oh!

update: use java.util.BitSet on js platform #384

update: use java.util.BitSet on js platform #384

Uh oh!

Conversation

i10416 commented Feb 18, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

johnynek commented Feb 18, 2022

Uh oh!

johnynek commented Feb 18, 2022

Uh oh!

i10416 commented Feb 19, 2022

Uh oh!

i10416 commented Feb 19, 2022

Uh oh!

i10416 commented Feb 19, 2022

Uh oh!

armanbilge commented Feb 19, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

armanbilge commented Feb 19, 2022

Uh oh!

i10416 commented Feb 19, 2022

Uh oh!

codecov-commenter commented Feb 20, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

johnynek commented Feb 20, 2022

Uh oh!

armanbilge commented Feb 20, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

johnynek commented Feb 20, 2022

Uh oh!

armanbilge commented Feb 20, 2022

Uh oh!

johnynek commented Feb 20, 2022

Uh oh!

i10416 commented Feb 25, 2022

Uh oh!

i10416 Mar 2, 2022

Choose a reason for hiding this comment

Uh oh!

johnynek Mar 2, 2022

Choose a reason for hiding this comment

Uh oh!

i10416 Mar 2, 2022

Choose a reason for hiding this comment

Uh oh!

i10416 commented Mar 2, 2022

Uh oh!

i10416 commented Mar 2, 2022

Uh oh!

i10416 commented Mar 2, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

i10416 commented Mar 4, 2022

Uh oh!

i10416 commented Mar 4, 2022

Uh oh!

Uh oh!

johnynek commented Mar 5, 2022

Uh oh!

i10416 commented Mar 16, 2022

Uh oh!

johnynek commented Mar 16, 2022

Uh oh!

i10416 commented Mar 16, 2022

Uh oh!

johnynek commented Mar 16, 2022

Uh oh!

i10416 commented Mar 17, 2022

Uh oh!

johnynek commented Mar 17, 2022

Uh oh!

Reviewers

Assignees

i10416 commented Feb 18, 2022 •

edited

Loading

armanbilge commented Feb 19, 2022 •

edited

Loading

codecov-commenter commented Feb 20, 2022 •

edited

Loading

armanbilge commented Feb 20, 2022 •

edited

Loading

i10416 commented Mar 2, 2022 •

edited

Loading