-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Refactor function type logic #18193
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
Refactor function type logic #18193
Conversation
This way we are explicit on the fact that this methods does not cover `PolyFunction` and `ErasedFunction`.
This method checks if the type is any kind of function type: `FunctionN`, `ContextFunctionN`, `PolyFunction`, and `ErasedFunction`.
The erasure now supports polymorphic function types with erased parameters. We still need to add support for this in the typer.
We should double-check all uses of |
1eedc1c
to
8a202e5
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Otherwise LGTM.
case mt: MethodType if !mt.isParamDependent => | ||
assert(mt.hasErasedParams) | ||
RefinedType(defn.ErasedFunctionType, nme.apply, mt) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of adding a case, I suggest changing the existing case mt: MethodType if !mt.isParamDependent =>
case to contain an if mt.hasErasedParams then ... else ...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I also moved !mt.isParamDependent
into assertions to make it clear that this is unexpected.
@@ -1709,21 +1710,29 @@ class Definitions { | |||
* - scala.FunctionN | |||
* - scala.ContextFunctionN | |||
*/ | |||
def isFunctionType(tp: Type)(using Context): Boolean = | |||
def isFunctionNType(tp: Type)(using Context): Boolean = | |||
isNonRefinedFunction(tp.dropDependentRefinement) | |||
|
|||
/** Is `tp` a specialized, refined function type? Either an `ErasedFunction` or a `PolyFunction`. */ | |||
def isRefinedFunctionType(tp: Type)(using Context): Boolean = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"refined function type" is a misleading name because dependent function types also contain a refinement but aren't covered here, so I would call this isPolyOrErasedFunctionType if we can't find something better.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Used isPolyOrErasedFunctionType
case info1: PolyType => | ||
return isSubInfo(info1, tp2.refinedInfo) | ||
case _ => | ||
else |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure why there is a difference between the two comparisons. Would it work to always use the second one, which tests for RefinedType
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will try to merge the two cases into one (in #18200).
Backports #18193 to the LTS branch. PR submitted by the release tooling. [skip ci]
isFucntionType
toisFunctionNType
(tests for FunctionN/ContextFunctionN)
isFunctionOrPolyType
toisFunctionType
(tests for FunctionN/ContextFunctionN/PolyFunction/ErasedFunction)
isRefinedFunctionType
toisPolyOrErasedFunctionType
(tests for PolyFunction/ErasedFunction)
isPolyFunctionType
isPolyFunctionType
/isErasedFunctionType
/isPolyOrErasedFunctionType
Followup of dotty-staging@6a349b5#r121364261