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

Map over refs of ImportTypes in TypeMap #20837

Merged
merged 3 commits into from
Jul 2, 2024
Merged

Conversation

EugeneFlesselle
Copy link
Contributor

@EugeneFlesselle EugeneFlesselle commented Jun 27, 2024

The inliner replaces references to parameters by their corresponding proxys, including in singleton types.
It did not handle the mapping over import types, the symbols of which way have depended on parameters.

Both i19493 and i19436 require mapping the type of the expr in an ImportType which is itself the info of a TermRef.
In the first issue, for the substitution of an inline def parameter proxy.
In the second issue, for the parameter of a lambda returned from an inline def.

Both can be handled in TypeMap by mapping over references to ImportTypes.
The second case also requires modifying TreeTypeMap#mapType such that the logic mapping over imports is done within a TypeMap doing the symbol substitutions.

Also note these mappings are only necessary for summonInlines (which could have just been made non-inline) resolving
post-inlining to givens imported within the inline definition.

Fix #19493
Fix #19436

@EugeneFlesselle EugeneFlesselle requested a review from odersky June 27, 2024 21:25
@EugeneFlesselle EugeneFlesselle changed the title Map over ImportTypes in inliner tree type map Map over refs of ImportTypes in TypeMap Jun 27, 2024
Copy link
Contributor

@odersky odersky left a comment

Choose a reason for hiding this comment

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

Otherwise LGTM

def apply(tp: Type): Type = tp match
case tp: TermRef if tp.symbol.isImport => mapOver(tp)
case tp => tp.substSym(substFrom, substTo)
typeMap.andThen(substMap).andThen(mapOwnerThis)
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe andThen is not inlined. So in the interest of performance I suggest the less nice

def mapType(tp: Type): Type = 
  ...
  mapOwnerThis(substMap(typeMap(tp)))

We allocate fewer closures that way, and empirically that matters (a bit)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Indeed ! turns out it's even @annotation.unspecialized

@odersky odersky assigned EugeneFlesselle and unassigned odersky Jul 2, 2024
The inliner replaces references to parameters by
their corresponding proxys, including in singleton types.
It did not, however, handle the mapping over import types,
the symbols of which way have depended on parameters.

Mapping imports correctly was necessary for i19493
since the `summonInline` resolves post inlining to
a given imported within the inline definition.

Fix scala#19493
Both i19493 and i19436 require mapping the type of
the expr in an `ImportType` which is itself the info of a `TermRef`.
In the first issue, for the substitution of an inline def parameter proxy.
In the second issue, for the parameter of a lambda returned from an inline def.

Both can be handled in `TypeMap` by mapping over references to `ImportType`s.
The second case also requires modifying `TreeTypeMap#mapType` such that
the logic mapping over imports is done within a `TypeMap` doing the symbol substitutions.

Fixes scala#19436
@odersky odersky merged commit cd8c5ed into scala:main Jul 2, 2024
22 checks passed
@odersky odersky deleted the fix-19493 branch July 2, 2024 18:56
WojciechMazur added a commit that referenced this pull request Jul 3, 2024
WojciechMazur added a commit that referenced this pull request Jul 11, 2024
Backports #20837 to the LTS branch.

PR submitted by the release tooling.
[skip ci]
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants