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

Inline unapplys in the inlining phase #19382

Merged

Conversation

nicolasstucki
Copy link
Contributor

@nicolasstucki nicolasstucki commented Jan 5, 2024

These currently got inlined while typing. Therefore they used to generate code that should not be pickled. The non-transparent inline methods should be inlined in the inlining phase.

This was found while working on #19380.

@nicolasstucki nicolasstucki added the needs-minor-release This PR cannot be merged until the next minor release label Jan 5, 2024
@nicolasstucki nicolasstucki marked this pull request as ready for review January 5, 2024 16:17
@nicolasstucki nicolasstucki added this to the 3.5.0 milestone Jan 5, 2024
@nicolasstucki nicolasstucki requested a review from jchyb January 5, 2024 16:18
@jchyb
Copy link
Contributor

jchyb commented Jan 5, 2024

Won't that cause the transparent inline call to also be inlined later, at the inlining phase? Outside of the one deleted here, I can't see any other reference to inlineUnapply in Typer. Unless the idea is to first transparent inline the unapply call in typer, and later wrap the result of that with the anon class in inlineUnapply? I suppose this would also fix #19369

@nicolasstucki nicolasstucki force-pushed the avoid-inlining-before-pickling branch from b757481 to 729327b Compare January 8, 2024 07:56
@nicolasstucki
Copy link
Contributor Author

nicolasstucki commented Jan 8, 2024

Won't that cause the transparent inline call to also be inlined later, at the inlining phase?

Yes. I changed this to keep inlining the transparent ones before pickling.

@nicolasstucki nicolasstucki added backport:nominated If we agree to backport this PR, replace this tag with "backport:accepted", otherwise delete it. and removed stat:do not merge labels Jan 10, 2024
@nicolasstucki nicolasstucki modified the milestones: 3.5.0, 3.4.0 Jan 10, 2024
@nicolasstucki
Copy link
Contributor Author

We should only merge this if it is also backported to 3.4.0-RC-2. Otherwise we will need to merge for 3.5.0-RC1.

@Kordyjan Kordyjan removed the backport:nominated If we agree to backport this PR, replace this tag with "backport:accepted", otherwise delete it. label Jan 15, 2024
@Kordyjan Kordyjan modified the milestones: 3.4.0, 3.5.0 Jan 15, 2024
@nicolasstucki nicolasstucki force-pushed the avoid-inlining-before-pickling branch from 729327b to 75a9813 Compare February 8, 2024 10:54
case tree: UnApply =>
tree match
case tree1: UnApply if Inlines.needsInlining(tree1) =>
super.transform(cpy.UnApply(tree1)(fun = Inlines.inlinedUnapplyFun(tree1.fun)))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This implementation changed after I rebased.

@@ -1937,9 +1937,6 @@ class Typer(@constructorOnly nestingLevel: Int = 0) extends Namer
if (bounds != null) sym.info = bounds
}
b
case t: UnApply if t.symbol.is(Inline) =>
assert(!t.symbol.is(Transparent))
cpy.UnApply(t)(fun = Inlines.inlinedUnapplyFun(t.fun)) // TODO inline these in the inlining phase (see #19382)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was deleted after I rebased

@nicolasstucki nicolasstucki force-pushed the avoid-inlining-before-pickling branch from 75a9813 to fee0287 Compare March 6, 2024 11:07
@nicolasstucki nicolasstucki assigned nicolasstucki and unassigned jchyb Mar 8, 2024
@nicolasstucki nicolasstucki force-pushed the avoid-inlining-before-pickling branch from fee0287 to d6a621c Compare March 27, 2024 14:20
@nicolasstucki nicolasstucki assigned jchyb and unassigned nicolasstucki Mar 28, 2024
@Gedochao
Copy link
Contributor

Gedochao commented Apr 3, 2024

It has been decided to be included in the 3.5.0 release.

Comment on lines 93 to 97
case tree: UnApply =>
tree match
case tree1: UnApply if Inlines.needsInlining(tree1) =>
super.transform(cpy.UnApply(tree1)(fun = Inlines.inlinedUnapplyFun(tree1.fun)))
case tree1 => super.transform(tree1)
Copy link
Contributor

@jchyb jchyb Apr 3, 2024

Choose a reason for hiding this comment

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

Suggested change
case tree: UnApply =>
tree match
case tree1: UnApply if Inlines.needsInlining(tree1) =>
super.transform(cpy.UnApply(tree1)(fun = Inlines.inlinedUnapplyFun(tree1.fun)))
case tree1 => super.transform(tree1)
case tree: UnApply if Inlines.needsInlining(tree) =>
super.transform(cpy.UnApply(tree)(fun = Inlines.inlinedUnapplyFun(tree.fun)))

Just a NIT/question. Would this work? I see there is a case that should go through super.transform(tree) for non inline Unapply trees later as well

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I probably wanted to avoid calling needsInlining twice. I can refactor it to make it more concise.

These currently got inlined while typing. Therefore they used to generate
code that should not be pickled.
@nicolasstucki nicolasstucki force-pushed the avoid-inlining-before-pickling branch from d6a621c to 804294c Compare April 4, 2024 07:41
@nicolasstucki nicolasstucki requested a review from jchyb April 4, 2024 08:32
@nicolasstucki nicolasstucki merged commit 06066db into scala:main Apr 4, 2024
19 checks passed
@nicolasstucki nicolasstucki deleted the avoid-inlining-before-pickling branch April 4, 2024 12:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-minor-release This PR cannot be merged until the next minor release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants