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

Simplify defn.FunctionOf.unapply #18486

Merged

Conversation

nicolasstucki
Copy link
Contributor

No description provided.

@nicolasstucki nicolasstucki self-assigned this Aug 30, 2023
@nicolasstucki nicolasstucki force-pushed the simplify-defn.FunctionOf.unapply branch from 91615cb to fe74fb4 Compare August 30, 2023 14:50
defn.FunctionOf(paramtpe.forceBoxStatus(true) :: Nil, restpe, isContextual)

mapArgUsing: tp =>
val defn.FunctionOf(paramtpe :: Nil, restpe, isContextual) = tp.strippedDealias: @unchecked
Copy link
Contributor Author

Choose a reason for hiding this comment

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

In the previous and current implementations, we need to strip the capture set from tp to get to the underlying function type. Is this intended, or are we losing part of the capture set that the resulting function needs?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I also noticed that previously we stripped the capture set in FunctionOf for FunctionN but not for PolyFunction. This inconsistency could have been noticed on functions with erased parameters. Now we do not strip capture sets in any of the cases in FunctionOf.

@nicolasstucki nicolasstucki marked this pull request as ready for review August 31, 2023 13:14
@nicolasstucki nicolasstucki requested a review from odersky August 31, 2023 13:14
@nicolasstucki nicolasstucki force-pushed the simplify-defn.FunctionOf.unapply branch from 3694bae to a083a7a Compare September 19, 2023 13:26
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

@@ -1116,16 +1116,13 @@ class Definitions {
else
FunctionType(args.length, isContextual).appliedTo(args ::: resultType :: Nil)
def unapply(ft: Type)(using Context): Option[(List[Type], Type, Boolean)] = {
ft.dealias match
Copy link
Contributor

Choose a reason for hiding this comment

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

What is the reason to drop the dealias in the extractor?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There are a few reasons:

  • There are few places where that type is not already dealiased
  • In a few cases we used to repeatedly dealias the type in pattern match extractors for functions. Not dealiassing here forces the use site to dealias a single time. Sometimes this happens in cases of a single pattern match and sometimes in the recursion of a TypeMap.
  • This aligns the implementation closer to RefinedFunctionOf, PolyFunctionOf and FunctionTypeOfMethod (in Handle dependent context functions #18443).
  • It brings us one step closer to being able to have a derivedFunctionTypeOf method.

Copy link
Contributor

Choose a reason for hiding this comment

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

OK, I agree it's good to align the unapply's towards no dealising.

@nicolasstucki nicolasstucki force-pushed the simplify-defn.FunctionOf.unapply branch from a083a7a to 171773d Compare November 14, 2023 09:08
@nicolasstucki nicolasstucki merged commit 312e4bb into scala:main Nov 14, 2023
15 checks passed
@nicolasstucki nicolasstucki deleted the simplify-defn.FunctionOf.unapply branch November 14, 2023 12:30
@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