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

Fix test("capture value in closure") #1270

Closed
wants to merge 2 commits into from

Conversation

froth
Copy link

@froth froth commented Aug 1, 2023

The test broke with the update of scala 3.3.0 (#1225). However the old while loop used in the test as verification 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.

fixes #1225

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
Author

froth commented Aug 1, 2023

This includes an update of the scala 3 version.

Therefore it supersedes #1249

@froth
Copy link
Author

froth commented Aug 1, 2023

OK, this however fails as the scala2 macros still behave the same as before (which means "not correct" in my opinion).
I quickly tried

diff --git a/macros/src/main/scala-2/spire/macros/Syntax.scala b/macros/src/main/scala-2/spire/macros/Syntax.scala
index c765a8e4..d2a814b5 100644
--- a/macros/src/main/scala-2/spire/macros/Syntax.scala
+++ b/macros/src/main/scala-2/spire/macros/Syntax.scala
@@ -190,7 +190,8 @@ object Syntax {
       q"""
       var $index = $init
       while ($test($index)) {
-        $body($index)
+        val indexCopy = $index
+        $body(indexCopy)
         $index = $next($index)
       }
       """

which actually fixes the test. However this breaks spire.syntax.CforSuite.nested cfor.

Maybe someone more experienced in the cfor macro (or scala 2 macros in general) can have a look and give me a hint.

@armanbilge
Copy link
Member

OK, this however fails as the scala2 macros still behave the same as before (which means "not correct" in my opinion).

Whether it is "not correct" or not in any of our opinions doesn't really matter unfortunately 😕 at this point, we shouldn't really be changing the behavior of cfor. Even more so, we shouldn't be changing the behavior of something for which a test was explicitly added asserting what the intended behavior is.

@froth
Copy link
Author

froth commented Aug 1, 2023

I get you point. Does not make me happy but makes sense. I will leave it at that.

For anybody picking this up in the future:

This is the decompiled output of the relevant part of the test under scala 3.2.2(working test):

      ArrayBuffer b1 = scala.collection.mutable.ArrayBuffer..MODULE$.empty();

      int var3;
      for(IntRef index = IntRef.create(0); index.elem < 3; index.elem = var3) {
         b1.$plus$eq(() -> {
            return index.elem;
         });
         var3 = index.elem + 1;
      }

And under 3.3.0(broken tests):

      ArrayBuffer b1 = scala.collection.mutable.ArrayBuffer..MODULE$.empty();

      for(int index = 0; index < 3; ++index) {
         b1.$plus$eq(() -> {
            return index;
         });
      }

@froth froth closed this Aug 9, 2023
@mio-19
Copy link

mio-19 commented Oct 20, 2024

Is the intended behavior used in real code? If not, is introducing a breaking change an option?

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.

Use of bug in cfor macro
3 participants