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

Handle dependent context functions #18443

Merged

Conversation

nicolasstucki
Copy link
Contributor

@nicolasstucki nicolasstucki commented Aug 22, 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 nicolasstucki force-pushed the handle-dependent-context-functions branch from 0f39f38 to 408c92c Compare August 22, 2023 15:32
@nicolasstucki nicolasstucki marked this pull request as ready for review August 24, 2023 07:53
@nicolasstucki nicolasstucki requested a review from odersky August 24, 2023 08:24
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 unapply(ft: Type)(using Context): Option[MethodOrPoly] = {
ft match
case RefinedType(parent, nme.apply, mt: MethodOrPoly)
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd try to optimize it further by inlining the FunctionOf extractor and specializing it to this use case.
Also, maybe split the RefinedType match into two, one for MethodType refinements and the other for PolyType refinements?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

maybe split the RefinedType match into two, one for MethodType refinements and the other for PolyType refinements?

We would end up with these cases

ft match
  case RefinedType(parent, nme.apply, mt: MethodType)
  if parent.derivesFrom(defn.PolyFunctionClass) || isFunctionNType(parent) =>
    Some(mt)
  case RefinedType(parent, nme.apply, pt: PolyType)
  if parent.derivesFrom(defn.PolyFunctionClass) =>
    Some(mt)
  ...

Note that in both cases we can have PolyFunctionClass because we now use PolyFunction for function types with erased parameters.

If it is for performance, we could do

ft match
  case RefinedType(parent, nme.apply, mt: MethodOrPoly)
  if parent.derivesFrom(defn.PolyFunctionClass) || (mt.isInstanceOf[MethodType] && isFunctionNType(parent)) =>
    Some(mt)
  ...

I pushed this last option.

Copy link
Contributor Author

@nicolasstucki nicolasstucki Aug 30, 2023

Choose a reason for hiding this comment

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

I'd try to optimize it further by inlining the FunctionOf extractor and specializing it to this use case.

Inlined, optimized, and aligned with #18418/#18486.

@nicolasstucki nicolasstucki force-pushed the handle-dependent-context-functions branch from 408c92c to 4df0b57 Compare August 30, 2023 08:35
@nicolasstucki nicolasstucki requested a review from odersky August 30, 2023 14:58
@nicolasstucki nicolasstucki force-pushed the handle-dependent-context-functions branch from 65f6dfa to 12f5442 Compare September 19, 2023 12:47
@odersky
Copy link
Contributor

odersky commented Oct 1, 2023

@nicolasstucki Can you have a look at the test failure? happy to review again when it is green.

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.
@nicolasstucki nicolasstucki force-pushed the handle-dependent-context-functions branch from 12f5442 to d5d8273 Compare October 12, 2023 15:18
@nicolasstucki
Copy link
Contributor Author

@odersky now the CI passed.

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.

This will be very handy to have. Sorry for dropping the ball on the reviews.

@nicolasstucki nicolasstucki merged commit 7c40fac into scala:main Nov 14, 2023
18 checks passed
@nicolasstucki nicolasstucki deleted the handle-dependent-context-functions branch November 14, 2023 09:00
@Kordyjan Kordyjan added this to the 3.4.0 milestone Dec 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants