A comprehension with valdef aliases is never a candidate for map elimination.

[som-snytt]

A comprehension with valdef aliases is never a candidate for map elimination. Previously, desugar would compare the body written by the user and not the body with a prefix of patdefs; that comparison must always fail.

When adding a filter, preserve FilterAlways check mode, which is used by makeLambda when detecting an IdPattern to avoid emitting (id: T) =>.

Use the previous commit that slightly simplifies the pattern match in makeFor. There is a single case for "generator followed by alias", and the body of the case handles either better fors or legacy.

Fixes #24673
Fixes #24736

[KacperFKorban]

Thanks for the PR, but I'm hesitant to merge changes that are primarily stylistic.
Readability is very subjective, and every time someone does a style refactor, other people have to re-learn the code.

[som-snytt]

In this case, I think the guard conditions are objectively hard to read. (As I learned when I had to read them.)

The guard condition says dropWhile and takeWhile which is probably less inefficient than the use of span in the commit.

The existing guard might be more conservative when the new feature is experimental (and the rest of the condition is never evaluated), but presumably the feature is actually exercised now.

I think the readability argument carries more weight, but if you don't see the value, then feel free to ignore the PR.

[som-snytt]

I wished to say, if you feel that the potential benefit is outweighed by the work to review the change, please ignore the PR.

[KacperFKorban]

Ok, sure I can be convinced that this improves readability and doesn't change it that much from the spec.

My main concerns were just that (1) the structure of the code looks less like the structure of the spec now (2) I want to see myself (and other people that already know the code) time from re-learning the same code, just to do a small change/review.

Sorry for the long reply times.

[som-snytt]

I'll look at whether it reflects the spec. It's certainly an Oderskyism that code should structurally preserve its specification (looking at parser, typer).

[som-snytt]

This PR leverages the previous PR that makes the code easier to read, since I had to read it again.

For posterity, the spurious debug test failure:

Details

[info] Test dotty.tools.debug.DebugTests.debug started Exception thrown onComplete (probably by a reporter) in tests/debug/eval-static-methods.scala: class java.io.IOException java.io.IOException: Failed getting JDI port of child JVM: got ERROR: transport error 202: send failed: Broken pipe at dotty.tools.vulpix.RunnerOrchestration$RunnerMonitor$RunnerProcess.getJdiPort(RunnerOrchestration.scala:113) at dotty.tools.vulpix.RunnerOrchestration$$anon$1.readJdiPort(RunnerOrchestration.scala:144) at dotty.tools.debug.DebugTests$DebugTest.verifyDebug$$anonfun$1(DebugTests.scala:60) at scala.runtime.function.JProcedure1.apply(JProcedure1.java:15) at scala.runtime.function.JProcedure1.apply(JProcedure1.java:10) at dotty.tools.vulpix.RunnerOrchestration$RunnerMonitor$Runner.debugMain(RunnerOrchestration.scala:149) at dotty.tools.vulpix.RunnerOrchestration.dotty$tools$vulpix$RunnerOrchestration$RunnerMonitor$$_$debugMain$$anonfun$1(RunnerOrchestration.scala:93) at scala.runtime.function.JProcedure1.apply(JProcedure1.java:15) at scala.runtime.function.JProcedure1.apply(JProcedure1.java:10) at dotty.tools.vulpix.RunnerOrchestration$RunnerMonitor.withRunner(RunnerOrchestration.scala:237) at dotty.tools.vulpix.RunnerOrchestration$RunnerMonitor.debugMain(RunnerOrchestration.scala:93) at dotty.tools.vulpix.RunnerOrchestration.debugMain(RunnerOrchestration.scala:72) at dotty.tools.vulpix.RunnerOrchestration.debugMain$(RunnerOrchestration.scala:38) at dotty.tools.debug.DebugTests$.debugMain(DebugTests.scala:24) at dotty.tools.debug.DebugTests$DebugTest.verifyDebug(DebugTests.scala:74) at dotty.tools.debug.DebugTests$DebugTest.onSuccess(DebugTests.scala:50) at dotty.tools.vulpix.ParallelTesting$CompilationLogic.dotty$tools$vulpix$ParallelTesting$CompilationLogic$$onComplete(ParallelTesting.scala:311) at dotty.tools.vulpix.ParallelTesting$$anon$3.checkTestSource$$anonfun$1(ParallelTesting.scala:298) at dotty.tools.vulpix.ParallelTesting$$anon$3.checkTestSource$$anonfun$adapted$1(ParallelTesting.scala:300) at scala.Function0.apply$mcV$sp(Function0.scala:45) at dotty.tools.vulpix.ParallelTesting$Test.tryCompile(ParallelTesting.scala:484) at dotty.tools.vulpix.ParallelTesting$$anon$3.checkTestSource(ParallelTesting.scala:300) at dotty.tools.vulpix.ParallelTesting$Test$LoggedRunnable.run(ParallelTesting.scala:380) at dotty.tools.vulpix.ParallelTesting$Test$LoggedRunnable.run$(ParallelTesting.scala:362) at dotty.tools.vulpix.ParallelTesting$$anon$3.run(ParallelTesting.scala:295) at java.base/java.util.concurrent.ForkJoinTask$AdaptedRunnableAction.exec(ForkJoinTask.java:1375) at java.base/java.util.concurrent.ForkJoinTask.doExec(ForkJoinTask.java:373) at java.base/java.util.concurrent.ForkJoinPool$WorkQueue.topLevelExec(ForkJoinPool.java:1182) at java.base/java.util.concurrent.ForkJoinPool.scan(ForkJoinPool.java:1655) at java.base/java.util.concurrent.ForkJoinPool.runWorker(ForkJoinPool.java:1622) at java.base/java.util.concurrent.ForkJoinWorkerThread.run(ForkJoinWorkerThread.java:165)

The catch in onComplete (Exception thrown onComplete (probably by a reporter)) catches other conditions such as a bad rewrite test.