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

Use of bug in cfor macro #1225

Open
nicolasstucki opened this issue Nov 25, 2022 · 5 comments
Open

Use of bug in cfor macro #1225

nicolasstucki opened this issue Nov 25, 2022 · 5 comments

Comments

@nicolasstucki
Copy link

The cfor macro accidentally generated the desired code by using a bug in beta-reduction. This bug will be patched scala/scala3#16390 and cfor will need to be updated.

See dotty-staging/spire@7f630c0...99ea909#diff-9cd0f3a9df06a2589472aa8dc960fd2a65a2b65f2f8c257addda1c86ebc31469

@froth
Copy link

froth commented Aug 1, 2023

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 added a commit to froth/spire that referenced this issue Aug 1, 2023
The test broke with the update of scala 3.3.0 (typelevel#1225).
However the old while loop did *not* capture the var so always created
List(3,3,3). So it tested that the implementation is actually *wrong*.
I changed the test to now use a fixed List.
@froth
Copy link

froth commented Aug 9, 2023

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

@erikerlandson
Copy link
Contributor

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
Copy link
Member

Yes.

@mio-19
Copy link

mio-19 commented Oct 20, 2024

How was the unit test introduced in the first place?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants