ensure fastForInline does not make closures

[bishabosha]

make the parameters of fastForInline inline, which will beta reduce them when applied.
Currently examining produced bytecode, closures are created for test, next and body

[armanbilge]

Brilliant, thank you! I remember asking about this in #1067 (comment) but seems I forgot to create an issue to track it 😅

We're not fussed about bincompat at the moment, but for my own education: what are the bincompat implications of a change like this?

If you feel so motivated, feel free to run the FastForBenchmark :)

[armanbilge]

Hmm, this seems to have broken the semantics somehow 🤔

I only looked quickly, but it's also possible you managed to restore the cfor semantics for this test.

spire/tests/shared/src/test/scala/spire/syntax/FastForSuite.scala

Lines 102 to 113 in a667a99

	// This test distinguishes fastFor from cfor
	test("doesn't capture value in closure") {
	val b1 = collection.mutable.ArrayBuffer.empty[() => Int]
	fastFor(0)(_ < 3, _ + 1) { x =>
	b1 += (() => x)
	}
	val b2 = collection.mutable.ArrayBuffer[() => Int]()
	(0 until 3).foreach { x =>
	b2 += (() => x)
	}
	assertEquals(b1.map(_.apply()).toList, b2.map(_.apply()).toList)
	}

If that's the case, then maybe we can implement cfor on Scala 3 with identical semantics to Scala 2 cfor, which would be excellent :)

[cquiroz]

This seems great, I wonder why the tests broke

[bishabosha]

I have updated the tests to reflect the fact that any of the arguments to fastFor / fastForRange can capture the loop variable.

This does mean that the semantics of the scala 2 versions does not match, so the test will probably fail again.

[armanbilge]

🎉 hooray, all tests passing! Thank you so much 😁

I have a question below.

[bishabosha]

@armanbilge I have optimised the implementation for both fastFor and fastForRange so that side effects will not be inlined in either. I am still unsure about the status of having fastForRange for Long as well, as that is not implemented in Scala 2. Please could I also have approval for the CI?

[armanbilge]

Wow! I see you were able to fix the semantics so that they match cfor again. That's fantastic, thank you!

I think this means we can rename fastFor back to cfor, share the tests across 2/3, and remove any reference to fastFor (we only introduced it due to the semantic changes). Doesn't have to be in this PR (I can do this refactoring in a follow-up). Then maybe we can release 0.18.0 final 😁

Right now the Scala 2 fastFor reference implementation is failing the CI. To land your PR, maybe let's make the fastFor test suite Scala 3-only.

I'll take a closer look later today. Thanks again for your efforts here :)

[bishabosha]

@armanbilge I have made the FastForSuite scala 3 only :)

[armanbilge]

LGTM and not too hard to wrap my head around, although letFunc went a bit over my head 😅 Regarding the Long implementation, I think it we should definitely include it 👍

I hope @cquiroz can take a look as well :)

[armanbilge]

Would you mind writing a comment(s) to explain a little of the macro magic here? For curious users and future contributors/maintainers alike :)

[bishabosha]

I have added some doc comments, and also found a pattern extractor I can use to simplify function literal detection

[cquiroz]

Looks great, thanks a lot for this fix

[cquiroz]

I wonder why the scala part wasn't needed earlier

[bishabosha]

It's still not needed, I thought it just made it more clear where quoted comes from

[cquiroz]

Nice, in the old scala2 code there was a differentiation for literals as well that I didn't manage to port to scala 3

[bishabosha]

@armanbilge @cquiroz I have pushed another commit to simplify a bit and document

[armanbilge]

Thanks, looks great! I hope that as you find time, you may continue to contribute—I think spire has an interesting journey ahead as we figure out how to work in a language without specialization :)