Skip to content

SI-5887: Can try any expression#4334

Closed
som-snytt wants to merge 2 commits intoscala:2.12.xfrom
som-snytt:issue/5887
Closed

SI-5887: Can try any expression#4334
som-snytt wants to merge 2 commits intoscala:2.12.xfrom
som-snytt:issue/5887

Conversation

@som-snytt
Copy link
Contributor

Previously, parsing try was special-cased for expressions
beginning with paren or brace. Perhaps that was to avoid odd
sights such as try (1,2,3) or try { case _ => }, but it
also broke try { 1 } + 1 finally ().

This commit takes an arbitrary expression to try.

It also takes an arbitrary expression for catch. But for
improved type safety, the expression is required to conform
to PartialFunction[Throwable, ?]. The previous transform
was named-based. In addition, this commit invokes applyOrElse.

Previously:

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 new {
     | def isDefinedAt(x: Any) = false
     | def apply(x: Any) = ()
     | }
<console>:8: error: type mismatch;
 found   : AnyRef{}
 required: PartialFunction[Throwable,?]
              try 42 catch new {
                           ^

scala> try 42 catch 22
<console>:8: error: type mismatch;
 found   : Int(22)
 required: PartialFunction[Throwable,?]
              try 42 catch 22
                           ^

Previously, parsing `try` was special-cased for expressions
beginning with paren or brace. Perhaps that was to avoid odd
sights such as `try (1,2,3)` or `try { case _ => }`, but it
also broke `try { 1 } + 1 finally ()`.

This commit takes an arbitrary expression to `try`.

It also takes an arbitrary expression for `catch`. But for
improved type safety, the expression is required to conform
to `PartialFunction[Throwable, ?]`. The previous transform
was named-based. In addition, this commit invokes `applyOrElse`.

Previously:
```
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 new {
     | def isDefinedAt(x: Any) = false
     | def apply(x: Any) = ()
     | }
<console>:8: error: type mismatch;
 found   : AnyRef{}
 required: PartialFunction[Throwable,?]
              try 42 catch new {
                           ^

scala> try 42 catch 22
<console>:8: error: type mismatch;
 found   : Int(22)
 required: PartialFunction[Throwable,?]
              try 42 catch 22
                           ^
```
The syntax allows arbitrary expressions for
both try and catch. It is already specified
that the expected type of a handler is a
PartialFunction.
@retronym
Copy link
Member

This patch introduces one or two hygiene problems:

% cat sandbox/test.scala
class C {
  type PartialFunction = String
  def test = {
    try () catch new {}
  }
}

% qscalac sandbox/test.scala
sandbox/test.scala:4: error: C.this.PartialFunction does not take type parameters
    try () catch new {}
                 ^
one error found

@retronym
Copy link
Member

In the process of contrasting this translation with the status quo, I've uncovered another bug. I've assigned this to you as it will be fixed if we pursue the proposed approach in this PR (but I would ask you to add the regression test.)

@retronym
Copy link
Member

I would suggest that the inference-assisting version of identity be moved into ScalaRuntime, rather than being embedded as a local method each time.

Copy link
Member

Choose a reason for hiding this comment

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

nme.applyOrElse. Generally it is better to reuse existing names to avoid repetitive name table lookups.

@som-snytt
Copy link
Contributor Author

Thanks for taking a look. I guess there's a real incentive for slender trees going forward.

At least the "inline" was rooted. I'll refresh my memory and make another pass. That catch 22 was a good one. He makes me chuckle.

@retronym
Copy link
Member

Another motivation for avoiding introducing functions and local methods in translations is that these can sometimes be outlawed or trigger VerifyError or NullPointerException-s in shadowy contexts like super constructor calls (see #2884).

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.

4 participants