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

Synthesize a PartialFunction from function literal #8172

Merged
merged 1 commit into from Jul 11, 2019

Conversation

dwijnand
Copy link
Member

@dwijnand dwijnand commented Jun 23, 2019

Allow val pf: PartialFunction[Int, Int] = { _ => 3 }

@scala-jenkins scala-jenkins added this to the 2.13.1 milestone Jun 23, 2019
@sjrd
Copy link
Member

sjrd commented Jun 23, 2019

Hum, can I ask why?

@dwijnand
Copy link
Member Author

Sure! Because it's safe and the workaround is annoying (ironically the reverse, being able to define a function from case statements, is unsafe but already possible).

Copy link
Contributor

@adriaanm adriaanm left a comment

Choose a reason for hiding this comment

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

Looking good. Suggested a few improvements/tweaks.

src/compiler/scala/tools/nsc/typechecker/Typers.scala Outdated Show resolved Hide resolved
src/compiler/scala/tools/nsc/typechecker/Typers.scala Outdated Show resolved Hide resolved
Copy link
Contributor

@adriaanm adriaanm left a comment

Choose a reason for hiding this comment

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

Nice! Few more suggestions.

src/compiler/scala/tools/nsc/typechecker/Typers.scala Outdated Show resolved Hide resolved
src/compiler/scala/tools/nsc/typechecker/Typers.scala Outdated Show resolved Hide resolved
src/compiler/scala/tools/nsc/typechecker/Typers.scala Outdated Show resolved Hide resolved
src/compiler/scala/tools/nsc/typechecker/Typers.scala Outdated Show resolved Hide resolved
src/compiler/scala/tools/nsc/typechecker/Typers.scala Outdated Show resolved Hide resolved
src/compiler/scala/tools/nsc/typechecker/Typers.scala Outdated Show resolved Hide resolved
@dwijnand dwijnand force-pushed the pf/from-function-literal branch 2 times, most recently from d3da7eb to 64039d3 Compare June 25, 2019 13:46
Copy link
Member

@lrytz lrytz left a comment

Choose a reason for hiding this comment

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

Looks good!

@dwijnand
Copy link
Member Author

(Added some overloading and eta-expanding tests, which came up in conversation.)

@dwijnand
Copy link
Member Author

dwijnand commented Jul 4, 2019

Anyone else want to review this?

@som-snytt
Copy link
Contributor

It's not too late to add language in 6.23.1 of the spec. There's already a slot for PF because of x => x match shape case, just add "if the expected type is a PF", etc.

@dwijnand
Copy link
Member Author

dwijnand commented Jul 5, 2019

Good thinking. I actually dropped the "is of the shape", seeing as it's no longer true in 2.13, right?

Welcome to Scala 2.13.0 (OpenJDK GraalVM CE 19.0.0, Java 1.8.0_212).
Type in expressions for evaluation. Or try :help.

scala> def takeFun(f: Int => String) = f.getClass
takeFun: (f: Int => String)Class[_ <: Int => String]

scala> takeFun(x => x match { case 1 => "one"; case _ => "other" })
res0: Class[_ <: Int => String] = class $$Lambda$2053/217123986

scala> def takeFun(f: Int => String) = f.isInstanceOf[PartialFunction[Int, String]]
takeFun: (f: Int => String)Boolean

scala> takeFun(x => x match { case 1 => "one"; case _ => "other" })
res1: Boolean = false

@som-snytt
Copy link
Contributor

It's the other case you preserve in your patch:

scala> def f(pf: PartialFunction[Int, String]) = pf.isDefinedAt(27)
f: (pf: PartialFunction[Int,String])Boolean

scala> f { case 42 => "" }
res0: Boolean = false

scala> f(x => x match { case 42 => "" })
res1: Boolean = false

So the last is still false not true with this change.

A couple of paragraphs later in the spec says how the PF is constructed. I want to suggest it's the same as 8.5 for "pattern matching anonymous functions", the bare case block. The piece to add is how a default case is supplied with the function body, so that isDefinedAt is true. (Edit: maybe don't use those words.)

The list of "expected types" can now just say PF without the conditional; you were right to drop it there, but add it back in the subsequent paragraph: If it has this shape, isDefinedAt is derived from the cases; otherwise, isDefinedAt is true.

(I think of the translation as if it were written as for 8.5, which is how it's implemented, but the spec is only in terms of adding the isDefinedAt member.)

@dwijnand
Copy link
Member Author

dwijnand commented Jul 8, 2019

Thanks for your feedback, @som-snytt. Let me know if I captured everything you were indicating or if I missed or misunderstood anything - I'm happy to make further changes.

@dwijnand dwijnand requested a review from som-snytt July 8, 2019 13:49
Copy link
Contributor

@som-snytt som-snytt left a comment

Choose a reason for hiding this comment

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

Sorry, I intended to come up with some draft words this weekend. But it was the weekend.

Here is my simplification, which says that your PR is the usual case, but in this special case where the body looks like a "pattern matching anonymous function", it's handled in a similar way. I wasn't gutsy enough to say that directly, because it would require linking to that section. (Maybe this text should add, See [link to anchor for PMAFs].)

When a PartialFunction is required, an additional member isDefinedAt
is synthesized, which simply returns true.
However, if the function literal has the shape x => x match { $\ldots$ },
then isDefinedAt is derived from the pattern match in the following way:
each case from the match expression evaluates to true,
and if there is no default case,
a default case is added that evalutes to false.

Copy link
Member

@lrytz lrytz left a comment

Choose a reason for hiding this comment

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

Still LGTM :-) I think we can merge.

@lrytz lrytz merged commit 90ecd78 into scala:2.13.x Jul 11, 2019
@dwijnand dwijnand deleted the pf/from-function-literal branch July 11, 2019 12:05
@dwijnand dwijnand added the release-notes worth highlighting in next release notes label Jul 12, 2019
@nafg
Copy link
Contributor

nafg commented Sep 18, 2019

In some places it's misspelled Funtion1

@dwijnand
Copy link
Member Author

#8430 (with an unintentional typo in the commit message. English is hard.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release-notes worth highlighting in next release notes
Projects
None yet
7 participants