Skip to content

Replace is{Poly|Erased}FunctionType with {PolyOrErased,Poly,Erased}FunctionOf #18207

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 3 commits into from
Jul 18, 2023
Merged

Replace is{Poly|Erased}FunctionType with {PolyOrErased,Poly,Erased}FunctionOf #18207

merged 3 commits into from
Jul 18, 2023

Conversation

nicolasstucki
Copy link
Contributor

No description provided.

@nicolasstucki nicolasstucki force-pushed the remove-isErased-and-isPoly-functions branch 3 times, most recently from de1857c to 429886f Compare July 14, 2023 09:13
@nicolasstucki nicolasstucki self-assigned this Jul 14, 2023
@nicolasstucki nicolasstucki marked this pull request as ready for review July 14, 2023 11:12
@nicolasstucki nicolasstucki requested a review from smarter July 14, 2023 13:08
smarter
smarter requested changes Jul 14, 2023
View reviewed changes
compiler/src/dotty/tools/dotc/core/Definitions.scala Outdated
@@ -1836,7 +1838,7 @@ class Definitions {
tp.stripTypeVar.dealias match
case tp1: TypeParamRef if ctx.typerState.constraint.contains(tp1) =>
asContextFunctionType(TypeComparer.bounds(tp1).hiBound)
case tp1 @ RefinedType(parent, nme.apply, mt: MethodType) if isErasedFunctionType(parent) && mt.isContextualMethod =>
case tp1 @ PolyOrErasedFunctionOf(mt: MethodType) if mt.isContextualMethod =>
Copy link
Member

Choose a reason for hiding this comment

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

I see that this only matches ErasedFunction and not PolyFunction because it takes a MethodType and not a PolyType, but this could easily lead to confusion (especially if we have polymorphic ErasedFunctions in the future), so I would prefer to have explicit ErasedFunctionOf/PolyFunctionOf extractors.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added ErasedFunctionOf. There are no places left where we would use PolyFunctionOf.

Copy link
Member

Choose a reason for hiding this comment

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

There's a couple of places where we have check for poly functions which could benefit from an extractor:

@smarter smarter assigned nicolasstucki and unassigned smarter Jul 14, 2023
@nicolasstucki nicolasstucki force-pushed the remove-isErased-and-isPoly-functions branch from 6337c96 to 24aa05c Compare July 17, 2023 12:52
@nicolasstucki nicolasstucki force-pushed the remove-isErased-and-isPoly-functions branch from 24aa05c to 73975db Compare July 17, 2023 15:19
@smarter smarter changed the title Remove replace is{Poly|Erased}FunctionType with PolyOrErasedFunctionOf Replace is{Poly|Erased}FunctionType with {PolyOrErased,Poly,Erased}FunctionOf Jul 17, 2023
smarter
smarter reviewed Jul 17, 2023
View reviewed changes
compiler/src/dotty/tools/dotc/core/Types.scala Outdated
Comment on lines 1750 to 1753
case t if defn.isNonRefinedFunction(t) =>
t
case t if defn.isErasedFunctionType(t) =>
case t @ defn.PolyOrErasedFunctionOf(_) =>
t
Copy link
Member

Choose a reason for hiding this comment

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

Looks like this could be just case t if defn.isFunctionType(t) =>

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 would make findFunctionType identify dependent function refinements. It looks like a desirable change.

@smarter smarter merged commit da16f43 into scala:main Jul 18, 2023
@smarter smarter deleted the remove-isErased-and-isPoly-functions branch July 18, 2023 19:00
@Kordyjan Kordyjan mentioned this pull request Dec 1, 2023
Merged
Kordyjan added a commit that referenced this pull request Dec 8, 2023
@Kordyjan
Backport "Replace is{Poly|Erased}FunctionType with `{PolyOrErased,P…
f79a803 
…oly,Erased}FunctionOf`" to LTS (#19120)

Backports #18207 to the LTS branch.

PR submitted by the release tooling.
[skip ci]
@Kordyjan Kordyjan added this to the 3.3.2 milestone Dec 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@smarter smarter smarter approved these changes

Assignees

@smarter smarter

Labels
None yet
Projects
None yet
Milestone
3.3.2
Development

Successfully merging this pull request may close these issues.
3 participants
@nicolasstucki @smarter @Kordyjan