Refactor dependent function refinement logic #18305
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
nicolasstucki
force-pushed
the
refactor-dependent-refinemet-logic
branch
7 times, most recently
from
9504f3c to
1969f39
Compare
nicolasstucki
changed the title
nicolasstucki
force-pushed
the
refactor-dependent-refinemet-logic
branch
9 times, most recently
from
934255f to
a325ff7
Compare
|
The current conflict is trivially fixable in a7f1966. I will rebase once I had some feedback on these changes. |
smarter
reviewed
Aug 15, 2023
Comment on lines
+1119
to
+1120
| val methodType = if isContextual then ContextualMethodType else MethodType | ||
| Some(methodType(argTypes, resultType)) |
There was a problem hiding this comment.
This is nice from a conceptual viewpoint, but I'm a bit worried about the performance impact of creating all these method types given that they are uncached types. Maybe the name of the extractor could make it clearer that there's a conversion, e.g. if we call it FromMethod (and we call have a corresponding apply to go in the other direction).
There was a problem hiding this comment.
Followup in #18443
There was a problem hiding this comment.
I agree it's good to make the conversion explicit in the name. The performance aspects are probably OK since the alternative is usually to take the list of AppliedType arguments and split it into inits and last, which is also expensive.
| * dependent refinements. Optionally returns a triple consisting of the argument | ||
| * types `As`, the result type `B` and a whether the type is an erased context function. | ||
| */ | ||
| object ContextFunctionType: |
There was a problem hiding this comment.
Isn't it still nice for convenience to keep an extractor just for context functions?
| approxTp.dealias.stripPoly match | ||
| case defn.FunctionOf(mt) | ||
| if mt.isContextualMethod && ( | ||
| mt.isResultDependent || // in this case `resType` is lying, gives us only the non-dependent upper bound |
There was a problem hiding this comment.
Hmm if we have an extractor that returns a MethodType I would really expect it to take dependencies into account properly
|
Test performance please |
nicolasstucki
added a commit
to dotty-staging/dotty
that referenced
this pull request
Aug 17, 2023
This provides variant to `defn.FunctionOf` that only deals with proper `FunctionN` and `ContextFunctionN` types. This avoids some overhead. A difference between the two `unapply`s is that this one does not dealias the type, it needs to be dealiased at call site. Part of scala#18305
This was referenced Aug 17, 2023
nicolasstucki
added a commit
that referenced
this pull request
Aug 21, 2023
This makes it possible to know for example that if we have a `MethodType` and we use `derivedLambdaType` we get another `MethodType`. Part of #18305
This was referenced Aug 21, 2023
nicolasstucki
force-pushed
the
refactor-dependent-refinemet-logic
branch
from
9eb04f0 to
18114f0
Compare
odersky
added a commit
that referenced
this pull request
Aug 29, 2023
Make `isJava` by default `false` and remove `dropLast` argument. Part of #18305. Note that the use of `toFunctionType` will increase, and most will not be java functions.
odersky
added a commit
that referenced
this pull request
Aug 29, 2023
nicolasstucki
added a commit
to dotty-staging/dotty
that referenced
this pull request
Aug 30, 2023
This provides variant to `defn.FunctionOf` that only deals with proper `FunctionN` and `ContextFunctionN` types. This avoids some overhead. A difference between the two `unapply`s is that this one does not dealias the type, it needs to be dealiased at call site. Part of scala#18305
nicolasstucki
force-pushed
the
refactor-dependent-refinemet-logic
branch
from
18114f0 to
47eb542
Compare
nicolasstucki
added a commit
that referenced
this pull request
Aug 31, 2023
The interpreter was dropping all arguments in the list if one of them was erased. Now we filter out only the erased arguments. Part of #18305
nicolasstucki
added a commit
that referenced
this pull request
Aug 31, 2023
nicolasstucki
added a commit
that referenced
this pull request
Nov 14, 2023
Add `FunctionTypeOfMethod` extractor that matches any kind of function
and return its method type.
We use this extractor instead of `ContextFunctionType` to all of
* `ContextFunctionN[...]`
* `ContextFunctionN[...] { def apply(using ...): R }` where `R` might be
dependent on the parameters.
* `PolyFunction { def apply(using ...): R }` where `R` might be
dependent on the parameters. Currently this one would have at least one
erased parameter.
The naming of the extractor follows the idea in
#18305 (comment).
nicolasstucki
added a commit
that referenced
this pull request
Nov 14, 2023
This provides variant to `defn.FunctionOf` that only deals with proper `FunctionN` and `ContextFunctionN` types. This avoids some overhead. A difference between the two `unapply`s is that this one does not dealias the type, it needs to be dealiased at call site. Part of #18305
WojciechMazur
added a commit
that referenced
this pull request
Jun 24, 2024
This provides variant to `defn.FunctionOf` that only deals with proper `FunctionN` and `ContextFunctionN` types. This avoids some overhead. A difference between the two `unapply`s is that this one does not dealias the type, it needs to be dealiased at call site. Part of #18305 [Cherry-picked d78c157][modified]
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This fixes inconsistencies with refined dependent function types and poly functions representing methods with erased parameters.
This PR has many commits on purpose. These changes proved to be extremely fragile. Therefore it was decided that it would be better to do smaller incremental changes to be able to pinpoint any undetected regression in the future.The first commits reflect the code to split the logic into base abstractions for function types. These aim to remove ad-hoc behavior of function-type extractors and homogenize them. Once this was done, we moved towards representing function types homogeneously using
MethodTypes as their base representation. This representation is more robust as it forces us to consider dependent function types explicitly. Finally, some fixes were made where we did not handle function types correctly. Some todos are left where we could generalize to dependent function types, but this could go out of the bounds of the current spec for dependent function types (to be followed-up).