Skip to content

Conversation

@som-snytt
Copy link
Contributor

@som-snytt som-snytt commented Jun 24, 2025

Fixes #23409

If map is added to a type by conversion, consider dropping the conversion when dropping a map call.

Similarly, it doesn't matter if the method is an extension or inline: drop the map and preserve the nominal receiver.

@som-snytt
Copy link
Contributor Author

som-snytt commented Jun 30, 2025

Handling inlined map at inlining works better.

Edit: working on that.

Edit: actually no, it's fine in DropForMap; at some point I just decided to lift out the reference to the binding of the receiver.

@som-snytt
Copy link
Contributor Author

The related PR because I had to read the code:

#23448

"It's not bad code, it's just a style choice."

@som-snytt som-snytt force-pushed the issue/23409-for-map branch from c121bd8 to 02fa81e Compare July 31, 2025 04:10
@som-snytt som-snytt changed the title Elide conversion of receiver in DropForMap Elide conversion of receiver, extension or inline map, trailing implicit args, in DropForMap Jul 31, 2025
@som-snytt
Copy link
Contributor Author

som-snytt commented Jul 31, 2025

I broke the one debug-preview test. This map is now eliminated: for x <- list yield x, as the sip promises IIUC.

I didn't study the debug test yet. The test run saw the SO in community_c as well. Edit: also community_b. At least it fails very fast.

Edit: Green check on test, but just missed the boat on debug-preview, as it is no longer preview.

@som-snytt som-snytt force-pushed the issue/23409-for-map branch from e9b0078 to 8691c9d Compare July 31, 2025 12:02
@som-snytt som-snytt marked this pull request as ready for review July 31, 2025 17:51
Copy link
Member

@KacperFKorban KacperFKorban left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The changes look great! I just left two minor comments/questions.

Sorry this took so long, this got lost in my notifications.

bindings.collectFirst:
case vd: ValDef if f.sameTree(vd.rhs) =>
expansion.find:
case Inlined(Thicket(Nil), Nil, Ident(ident)) => ident == vd.name
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it be safer to compare symbols instead of names?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, an excellent spot!

Comment on lines 80 to 83
// Extracts a fun from a possibly nested Apply with lambda and arbitrary implicit args.
// Specifically, an application `r.map(x => x)` is destructured into (r, map, args).
// If the receiver r was adapted, it is unwrapped.
// If `map` is an extension method, the nominal receiver is `args.head`.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// Extracts a fun from a possibly nested Apply with lambda and arbitrary implicit args.
// Specifically, an application `r.map(x => x)` is destructured into (r, map, args).
// If the receiver r was adapted, it is unwrapped.
// If `map` is an extension method, the nominal receiver is `args.head`.
/** Extracts a fun from a possibly nested Apply with lambda and arbitrary implicit args.
* Specifically, an application `r.map(x => x)` is destructured into (r, map, args).
* If the receiver r was adapted, it is unwrapped.
* If `map` is an extension method, the nominal receiver is `args.head`.
*/

Comment on lines +7 to +8
case MySome(x) => ??? //MySome(f(x))
case MyNone => ??? //MyNone
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice! This is way more elegant than my previous approach 😃

@som-snytt som-snytt force-pushed the issue/23409-for-map branch from 8691c9d to a663a2a Compare October 23, 2025 23:38
@som-snytt som-snytt merged commit a9c731c into scala:main Oct 24, 2025
51 checks passed
@som-snytt som-snytt deleted the issue/23409-for-map branch October 24, 2025 15:23
@WojciechMazur WojciechMazur added this to the 3.8.0 milestone Oct 28, 2025
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.

Regression: betterFors doesn't remove trailing map

3 participants