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

Safer exceptions #11721

Merged
merged 6 commits into from
Aug 30, 2021
Merged

Safer exceptions #11721

merged 6 commits into from
Aug 30, 2021

Conversation

odersky
Copy link
Contributor

@odersky odersky commented Mar 12, 2021

Introduce a flexible scheme for declaring and checking which exceptions can be thrown.
It relies on the effects as implicit capabilities pattern.

The scheme is not 100% safe yet since it does not track and prevent capability capture.
Nevertheless, it's already useful for declaring thrown exceptions and finding mismatches
between provided and required capabilities.

Link to Doc page

Everything is enabled under

import language.experimental.saferExceptions

Based on #11695

For Release Notes:

Safer exceptions

A new experimental feature that allows declaring and checking which exceptions can be thrown.
It relies on the effects as implicit capabilities pattern.

@odersky odersky changed the title Introduce checked exceptions Safer exceptions Mar 12, 2021
@odersky odersky force-pushed the add-safe-throws-2 branch 3 times, most recently from 0228129 to fe446b4 Compare March 14, 2021 17:57
@soronpo
Copy link
Contributor

soronpo commented Mar 14, 2021

Regarding multiple exceptions, IIUC, the current concept is that the return signature can be something like T canThrow Ex1 canThrow Ex2. Isn't that signature considered different than T canThrow Ex2 canThrow Ex1 ?

Actually, no. The rules for context function types make them mutually compatible. I am not yet sure about unions, whether there is a way to obtain them. But since we do have commutativity, maybe they are not needed.

@odersky odersky force-pushed the add-safe-throws-2 branch 4 times, most recently from c02fb16 to 1c353ab Compare March 14, 2021 22:28
project/Build.scala Outdated Show resolved Hide resolved
import language.experimental.erasedTerms
import annotation.implicitNotFound

/** A ability class that allows to throw exception `E`. When used with the
Copy link

@mr-git mr-git Mar 15, 2021

Choose a reason for hiding this comment

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

/** AN ability ..

though, in other places it is referenced as "A capability"..

@Jasper-M
Copy link
Member

Jasper-M commented Mar 16, 2021

Would it be possible for a user to implement something like this, without needing casts or other hacks?

object Try:
  def apply[E, A](body: CanThrow[E] ?=> A): Try[A] = 
    try
      Success(body)
    catch
      case NonFatal(ex) => Failure(ex)

@YulawOne
Copy link

I would like to suggest, to consider rename CanThrow and canThrow to Throws and throws, a reasoning behind is it looks quite awkward with camelCase and would be much neater with single word. Alternative names can be - raises,raise, cause, fails or failure

@odersky
Copy link
Contributor Author

odersky commented Jun 8, 2021

It fails the binary compatibility check:

scala3-library-bootstrapped: Failed binary compatibility check against org.scala-lang:scala3-library_3:3.0.0! Found 7 potential problems (filtered 26)
Error:   * interface scala.CanThrow does not have a correspondent in other version
Error:     filter with: ProblemFilters.exclude[MissingClassProblem]("scala.CanThrow")
...

How should this be addressed?

@sjrd
Copy link
Member

sjrd commented Jun 8, 2021

You'll have to mark it as experimental, and then add an exception for MiMa justified by the fact that it is experimental.

@odersky
Copy link
Contributor Author

odersky commented Jun 9, 2021

@sjrd In this case it's probably OK to mark it as experimental. But in general there could be additions that are slated for the next minor release. in this case we do want them to pass the CR without adding @experimental and without fiddling with Mima. How do we achieve that?

@julienrf
Copy link
Collaborator

julienrf commented Jun 9, 2021

But in general there could be additions that are slated for the next minor release. in this case we do want them to pass the CR without adding @experimental and without fiddling with Mima. How do we achieve that?

As I said in #12778, there should be a (simple) way to tune the build such that MiMa constraints would be relaxed. Just reading at the source code, it seems to me that setting baseVersion = "3.1.0" would be enough (no need to deal with MiMa filters).

Edit: sorry my suggestion applies only to PRs that target the next minor release, not to experimental features.

@odersky odersky force-pushed the add-safe-throws-2 branch 2 times, most recently from 1d42a72 to 471f19d Compare June 10, 2021 08:14
@odersky odersky force-pushed the add-safe-throws-2 branch 2 times, most recently from 6285368 to a2ebf00 Compare July 25, 2021 12:28
@odersky odersky added the needs-minor-release This PR cannot be merged until the next minor release label Jul 25, 2021
@odersky
Copy link
Contributor Author

odersky commented Jul 25, 2021

I have changed canThrow to throws, which is now treated as a soft keyword, so that it is still available in the @throws annotation.

@odersky odersky marked this pull request as ready for review July 25, 2021 13:15
@odersky
Copy link
Contributor Author

odersky commented Aug 26, 2021

I opened an issue #13392.

@nicolasstucki
Copy link
Contributor

Specialization by the compiler itself is an evil, ugly patch. The need for it shows that something is still very wrong with experimental. I would therefore suggest that we disable experimental checking until 3.2.

There are two distinct issues here.

  • One is the use of erased in the standard library. We should not do it until this feature is stabilized as @sjrd argued. We should special case the methods that happen to have those semantics.
  • The import of an experimental feature fails we tell the compiler to fail if we use experimental features with -Yno-experimental. This is the expected behavior. We compile the library with -Yno-experimental to avoid accidental uses of experimental features that might leak into a stable version of the library.

@nicolasstucki
Copy link
Contributor

#13394 fixes the current blocker. Tested locally by cherry-picking ea8cb80 on top of this branch.

Copy link
Contributor

@nicolasstucki nicolasstucki left a comment

Choose a reason for hiding this comment

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

If you want to make CanThrow compile before #13392 is fixed, just disable the check on the library by removing this line https://github.com/lampepfl/dotty/blob/master/project/Build.scala#L1746. Do not add #13396 to this PR as it has it too controvertial at this time.

@odersky
Copy link
Contributor Author

odersky commented Aug 27, 2021

#13392 is a blocker for 3.1. We simply cannot release another iteration of experimental with serious flaws like that.
So it's fix #13392 or disable experimental checking (which might be more prudent since other problems might come up).

@odersky
Copy link
Contributor Author

odersky commented Aug 27, 2021

Blocked by #13404

@nicolasstucki
Copy link
Contributor

If you want to make CanThrow compile before #13392 is fixed, just disable the check on the library by removing this line https://github.com/lampepfl/dotty/blob/master/project/Build.scala#L1746. Do not add #13396 to this PR as it has it too controvertial at this time.

#13392 is a blocker for 3.1. We simply cannot release another iteration of experimental with serious flaws like that.
So it's fix #13392 or disable experimental checking (which might be more prudent since other problems might come up).

What I meant is that we can disable that check for this PR to be able to merge it before #13392 is fixed. Then reenable the check when we fix #13392 (before 3.1).

@odersky
Copy link
Contributor Author

odersky commented Aug 29, 2021

What I meant is that we can disable that check for this PR to be able to merge it before #13392 is fixed. Then reenable the check when we fix #13392 (before 3.1).

I thought we'd merge the fix for #13392 first and then merge this one. Would that not work?

@nicolasstucki
Copy link
Contributor

Not if it is the wrong fix

Introduce a flexible scheme for declaring and checking which exceptions can be thrown.
It relies on the effects as implicit capabilities pattern.

The scheme is not 100% safe yet since it does not track and prevent capability capture.
Nevertheless, it's already useful for declaring thrown exceptions and finding mismatches
between provided and required capabilities.
... instead of "ability". Two reasons:

 - it's more standard
 - it's also more correct. According to
   https://writingexplained.org/capability-vs-ability-difference#When_to_Use_Capability
   a capability is a yes or no proposition, whereas an ability is a matter of degree.
 - Move $throws to scala.runtime
 - Add comment
@odersky
Copy link
Contributor Author

odersky commented Aug 30, 2021

Rebased to make use of #13394 for allowing erased classes

@odersky odersky merged commit 0da9afd into scala:master Aug 30, 2021
@odersky odersky deleted the add-safe-throws-2 branch August 30, 2021 19:30
@ysthakur
Copy link
Contributor

ysthakur commented Sep 7, 2021

Is there any chance of easing throwing multiple exceptions with a type takes a tuple of exceptions? Maybe something like

infix type throwsMulti[R, T <: Tuple] = T match {
  case EmptyTuple => R
  case e *: t => throwsMulti[CanThrow[e] ?=> R, t]
}

?

@bjornregnell
Copy link
Contributor

bjornregnell commented Sep 10, 2021

Why is the example in the docs infinite recursion?
https://github.com/lampepfl/dotty/blame/b65958f139a0a818d36ee04468ada93b8fb472ac/docs/docs/reference/experimental/canthrow.md#L119
Would perhaps this be a more appropriate example? (sorry if I'm missing something?)

def f(x: Double): Double =
  if x < limit then x * x else throw LimitExceeded())

x * x instead of f(x) - or something else less infinite? :)

@bjornregnell
Copy link
Contributor

bjornregnell commented Sep 10, 2021

sorry; forget my comment above -- I just realized that I was looking at the old version.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-minor-release This PR cannot be merged until the next minor release release-notes Should be mentioned in the release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet