typelevel / spire

Powerful new number types and numeric abstractions for Scala.
http://typelevel.org/spire/
MIT License
1.76k stars 242 forks source link

Use of bug in `cfor` macro #1225

Open nicolasstucki opened 1 year ago

nicolasstucki commented 1 year ago

The cfor macro accidentally generated the desired code by using a bug in beta-reduction. This bug will be patched https://github.com/lampepfl/dotty/pull/16390 and cfor will need to be updated.

See https://github.com/dotty-staging/spire/compare/7f630c0209e327bdc782ade2210d8e4b916fddcc...99ea909d086e28e85dbdf8aa78ef0a83bf873405#diff-9cd0f3a9df06a2589472aa8dc960fd2a65a2b65f2f8c257addda1c86ebc31469

froth commented 11 months ago

After staring and printlning for an hour I believe the situation is slightly different.

My opinion is, that the fix in dotty actually fixes a bug in the cfor macro. However the unit test is incorrect and therefore insists on the wrong implementation. I will create a pull request to fix the test.

froth commented 11 months ago

Turned out to be not that straight forward (see closed PR). Maybe someone comes up with a compatible solution.

erikerlandson commented 6 months ago

Out of curiosity, is this why I saw cfor unit test fail when I tried a spire build with 3.3.2?

  + functions with side effects function by-value params in cfor 0.002s
==> X spire.syntax.CforSuite.capture value in closure  0.089s munit.ComparisonFailException: /home/eje/git/typelevel/spire/tests/shared/src/test/scala/spire/syntax/CforSuite.scala:113
112:    }
113:    assertEquals(b1.map(_.apply()).toList, b2.map(_.apply()).toList)
114:  }
values are not the same
=> Obtained
List(
  0,
  1,
  2
)
=> Diff (- obtained, + expected)
 List(
-  0,
-  1,
-  2
+  3,
+  3,
+  3
 )
armanbilge commented 6 months ago

Yes.