Skip to content

Conversation

@som-snytt
Copy link
Contributor

@som-snytt som-snytt commented Mar 24, 2015

catch accepts an arbitrary expression, which must conform
to Function[Throwable, ?].

The previous transform was name-based:

scala> try 42 catch new {
     | def isDefinedAt(x: Any) = false
     | def apply(x: Any) = ()
     | }
warning: there was one feature warning; re-run with -feature for details
res0: AnyVal = 42

scala> try 42 catch 22
<console>:8: error: value isDefinedAt is not a member of Int
              try 42 catch 22
                           ^

Now:

scala> try 42 catch 22
                    ^
       error: type mismatch;
        found   : Int(22)
        required: Throwable => ?

scala> def npe: Int = throw null
def npe: Int

scala> val pf: PartialFunction[Throwable, Int] = { case _: NullPointerException => 42 }
val pf: PartialFunction[Throwable,Int] = <function1>

scala> try npe catch pf
val res0: Int = 42

scala> def err: Int = throw new Error()
def err: Int

scala> try err catch pf
java.lang.Error
  at err(<console>:1)
  at liftedTree1$1(<console>:1)
  ... 33 elided

As shown, if the handler is a PartialFunction, it is invoked
selectively, as though by calling:

pf.applyOrElse(e, (t: Throwable) => throw t)

Otherwise, the handler is applied to all thrown exceptions:

scala> def f(t: Throwable) = if (t.isInstanceOf[NullPointerException]) 27 else ???
def f(t: Throwable): Int

scala> try err catch f
scala.NotImplementedError: an implementation is missing
  at scala.Predef$.$qmark$qmark$qmark(Predef.scala:345)
  at f(<console>:1)
  at $anonfun$res6$1(<console>:1)
  at $anonfun$res6$1$adapted(<console>:1)
  at liftedTree1$1(<console>:1)
  ... 33 elided

Note that applying a partial function literal in the form of a pattern-matching
anonymous function will use applyOrElse. The following example invokes
the apply method because the static type of the handler is not a
PartialFunction:

scala> val g: Throwable => Int = pf
val g: Throwable => Int = <function1>

scala> try err catch g
scala.MatchError: java.lang.Error (of class java.lang.Error)
  at scala.PartialFunction$$anon$1.apply(PartialFunction.scala:344)
  at scala.PartialFunction$$anon$1.apply(PartialFunction.scala:342)
  at $anonfun$1.applyOrElse(<console>:1)
  at $anonfun$1.applyOrElse(<console>:1)
  at scala.runtime.AbstractPartialFunction.apply(AbstractPartialFunction.scala:35)
  at liftedTree1$1(<console>:1)
  ... 33 elided

@scala-jenkins scala-jenkins added this to the 2.12.0-M1 milestone Mar 24, 2015
@som-snytt som-snytt closed this Mar 25, 2015
@SethTisue SethTisue removed this from the 2.12.0-M1 milestone Jul 20, 2015
@som-snytt som-snytt reopened this Jun 7, 2016
@scala-jenkins scala-jenkins added this to the 2.12.0-RC1 milestone Jun 7, 2016
@som-snytt som-snytt force-pushed the issue/5887-rebased branch from 805becc to 880d48e Compare June 7, 2016 06:54
@som-snytt
Copy link
Contributor Author

som-snytt commented Jun 7, 2016

Rebased. There may be review work pending around hygiene? Actually, it looks like I addressed some comments from a year ago. But there's a bootstrap issue with try relying on library code.

case Match(EmptyTree, cases) => cases
case _ =>
val binder = freshTermName()
val pat = Bind(binder, Typed(Ident(termNames.WILDCARD), Ident(typeNames.Throwable)))
Copy link
Member

Choose a reason for hiding this comment

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

This appears unhygenic, 😷

Copy link
Contributor Author

@som-snytt som-snytt Jun 7, 2016

Choose a reason for hiding this comment

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

I think you mentioned that a year ago... If I can get it to build, I'll polish it.

@retronym
Copy link
Member

retronym commented Jun 7, 2016

Would it be possible to submit the part of this that fixes parsing of try { 1; 1 } + 1 catch from the part that changes how catch expressions are translated? I can't remember the context for the latter part, and I'm a bit hesitant about having catch sometimes translating into something heavyweight (an applyOrElse call with a lambda as an argument).

@SethTisue
Copy link
Member

7-year itch, scratched.

@som-snytt
Copy link
Contributor Author

7 years later.

@som-snytt som-snytt reopened this Sep 3, 2020
@scala-jenkins scala-jenkins added this to the 2.12.13 milestone Sep 3, 2020
@som-snytt som-snytt changed the base branch from 2.12.x to 2.13.x September 3, 2020 03:25
@som-snytt som-snytt modified the milestones: 2.12.13, 2.13.4 Sep 3, 2020
@som-snytt som-snytt changed the title SI-5887: Can try any expression Can catch any expression Sep 3, 2020
@som-snytt som-snytt marked this pull request as draft September 3, 2020 03:30
@dwijnand
Copy link
Member

:head_scratch: is this addressing scala/bug#5887?

@som-snytt
Copy link
Contributor Author

@dwijnand this is the catch part. It doesn't typecheck correctly yet, mea culpa. Dotty requires a Function instead of PartialFunction, but this is an improvement over the status quo.

@dwijnand dwijnand removed this from the 2.13.4 milestone Oct 16, 2020
@som-snytt
Copy link
Contributor Author

My previous joke about Try is relevant. That is, prefer Try to try for this sort of safety.

I'll add a lint if that will assuage our collective conscience, but I think the catch function is quite niche.

The use case for linting would be, when I write the handler, they never made it easy for me to specify the type:

val handler: PartialFunction[Throwable, String] = { case _ => "hello, world" }

val danger_? = (t: Throwable) => "goodbye, cruel world"

try e catch ???

Using handler doesn't warn. Or rather, defining it does not warn.

@som-snytt
Copy link
Contributor Author

Gah, lost my comment edit when I clicked on the validate failure! which was spurious, of course.

To recap, now it issues the usual warning on catch with total function.

My comment also complained about stretching this PR into a long tail.

To answer Seth's question, the usual tractable syntax is the block of case clauses. Anyone doing something else syntactically will know what they're doing.

I wanted to make the case that in a Scalable language, I should be able to say try f() catch println in REPL. But I don't want to get into a philosophical debate about it either.

This change goes to regularity.

@som-snytt

This comment has been minimized.

@SethTisue
Copy link
Member

@lrytz care to weigh in about the design issues here?

@som-snytt
Copy link
Contributor Author

Closing because I don't care any more.

@lrytz
Copy link
Member

lrytz commented Jan 7, 2021

Allowing Function instead of requiring PartialFunction only encourages catching Throwable, which is pretty much never a good idea (because of e.g. OutOfMemoryError). What desirable code pattern does this enable?

This is a good point.

Opened scala/scala3#11019, also that Scala 3 doesn't have the "catches all Throwables" warning at all.

@som-snytt
Copy link
Contributor Author

The good point was addressed in the follow-up.

@lrytz
Copy link
Member

lrytz commented Jan 8, 2021

👍, I noticed the warning. So it would be a new, handy feature for yolo-hacking, but discouraged from the beginning for larger projects.

@SethTisue
Copy link
Member

(To be clear, I'm not opposed, as long as it warns. But can't get excited about it either.)

This commit takes an arbitrary expression to `catch`.

The expression is required to conform to `Function[Throwable, ?]`.
The previous transform was name-based.

If the handler is a `PartialFunction`, it is invoked conditionally.

More behavior tests for catch expression.
@som-snytt som-snytt reopened this Feb 5, 2021
@scala-jenkins scala-jenkins added this to the 2.13.5 milestone Feb 5, 2021
Copy link
Member

@dwijnand dwijnand left a comment

Choose a reason for hiding this comment

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

Thanks, Som!

@lrytz
Copy link
Member

lrytz commented Feb 8, 2021

I'm slightly torn; it's generally good to align Scala 2 and 3, and I also think having the warning is right. But then we introduce a new feature that gives a warning as soon and as long as you use it. Does that make sense?

Are we certain that the change in dotty was fully intentioned?

@lrytz lrytz requested a review from SethTisue February 8, 2021 15:35
@som-snytt
Copy link
Contributor Author

We can adjust later to the status quo established by the dotty ticket.

It's not like everyone will start writing try e catch f everywhere. Probably no one notices the difference.

But it's much more regular than the previous state of affairs.

I had an example of a feature that was introduced and immediately deprecated, but I don't remember it off the cuff.

For the record, now I believe the compiler should emit no warnings at all, because someone will always complain either that the warning is too strict or too lax. Leave it to 3rd party tooling to express preferences and unwarranted fears about style.

I re-opened this PR because it's an obvious good.

@SethTisue SethTisue modified the milestones: 2.13.5, 2.13.6 Feb 9, 2021
@SethTisue
Copy link
Member

there hasn't been any response on scala/scala3#11019

Lukas and I are both on the fence, but Dale likes it, and Som is still behind this even after having had six years to think about it (😬), so let's merge

@SethTisue SethTisue merged commit 4e3bd2b into scala:2.13.x Mar 9, 2021
@som-snytt
Copy link
Contributor Author

I'm going to delete this branch and go back to sleep.

I'm grateful there was no need to come up with an option name like for splain.

I'm sad Seth is not excited.

I see there were a few previous summations. My footnote would be to echo my previous comment: really, people on the fence want to say, Don't use try, use Try instead. Why don't we say that?

Because of the pun, "on the fens", I have learned from wikipedia that a fen is a type of mire, but less acidic than a bog. Please keep this in mind when saying casually, "I'm bogged down," "in a mire," etc, let alone, "on the fens." Let's use language precisely.

@som-snytt som-snytt deleted the issue/5887-rebased branch March 9, 2021 21:49
@SethTisue
Copy link
Member

a fen is a type of mire, but less acidic than a bog

as for me, just yesterday I learned what “muskeg” is

@som-snytt
Copy link
Contributor Author

This PR was definitely muskegged.

No relation to "musk egg," "Muskogee," or "beer keg." The keg hauled by moose on rescue missions would be moose keg.

@SethTisue SethTisue removed the release-notes worth highlighting in next release notes label May 14, 2021
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.

6 participants