-
Notifications
You must be signed in to change notification settings - Fork 3.1k
Can catch any expression #4400
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
Can catch any expression #4400
Conversation
805becc to
880d48e
Compare
|
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))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This appears unhygenic, 😷
There was a problem hiding this comment.
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.
|
Would it be possible to submit the part of this that fixes parsing of |
|
7-year itch, scratched. |
|
7 years later. |
880d48e to
5d8b91f
Compare
5d8b91f to
9638fc1
Compare
|
:head_scratch: is this addressing scala/bug#5887? |
|
@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. |
|
My previous joke about 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: Using |
|
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 This change goes to regularity. |
This comment has been minimized.
This comment has been minimized.
|
@lrytz care to weigh in about the design issues here? |
|
Closing because I don't care any more. |
This is a good point. Opened scala/scala3#11019, also that Scala 3 doesn't have the "catches all Throwables" warning at all. |
|
The good point was addressed in the follow-up. |
|
👍, I noticed the warning. So it would be a new, handy feature for yolo-hacking, but discouraged from the beginning for larger projects. |
|
(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.
6144f5e to
aa708e3
Compare
dwijnand
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, Som!
|
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? |
|
We can adjust later to the status quo established by the dotty ticket. It's not like everyone will start writing 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. |
|
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 |
|
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. |
as for me, just yesterday I learned what “muskeg” is |
|
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. |
catchaccepts an arbitrary expression, which must conformto
Function[Throwable, ?].The previous transform was name-based:
Now:
As shown, if the handler is a
PartialFunction, it is invokedselectively, as though by calling:
Otherwise, the handler is applied to all thrown exceptions:
Note that applying a partial function literal in the form of a pattern-matching
anonymous function will use
applyOrElse. The following example invokesthe
applymethod because the static type of the handler is not aPartialFunction: