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

Deprecate old catch #9154

Merged
merged 5 commits into from
Jan 13, 2025
Merged

Deprecate old catch #9154

merged 5 commits into from
Jan 13, 2025

Conversation

richcarl
Copy link
Contributor

@richcarl richcarl commented Dec 8, 2024

try...catch...end has been available for a very long time now, and the old, simplistic catch Expr should be dropped from the language. In most places where it is used, it carries with it some unwanted (and often unknown) corner case behaviour, such as discarding the original error location, conflating errors, exits, and throws, and mixing up thrown values with normally returned values in a way that makes it very hard to see what the author intended, and whether the implementation really does what they hoped. Maintainance of such code is very hard.

I suggest the following schedule: 1) Add a deprecation warning, turned off by default, allowing users to start eliminating old catches from their own code as soon as possible. 2) Enable the warning by default in OTP 28 but allow it to be turned off selectively per module or application, so users do not have to tackle the entire code base at once. 3) Remove the option to disable the warning in OTP 29. 4) Remove the support for catch Expr completely in OTP 30.

This patch implements the warning in step 1 and marks up the applications kernel, stdlib, and compiler to prevent new old-style catches from being added.

Copy link
Contributor

github-actions bot commented Dec 8, 2024

CT Test Results

    4 files    476 suites   2h 16m 50s ⏱️
4 300 tests 4 011 ✅ 287 💤 2 ❌
9 717 runs  9 374 ✅ 341 💤 2 ❌

For more details on these failures, see this check.

Results for commit 7452c72.

♻️ This comment has been updated with latest results.

To speed up review, make sure that you have read Contributing to Erlang/OTP and that all checks pass.

See the TESTING and DEVELOPMENT HowTo guides for details about how to run test locally.

Artifacts

// Erlang/OTP Github Action Bot

@okeuday
Copy link
Contributor

okeuday commented Dec 8, 2024

@richcarl The old simplistic catch is useful in a few situations:

  • terminate functions that are unable to do something useful with exceptions (due to a focus on quick execution) for functions like: gen_tcp:close/1, erlang:port_close/1, file:delete/1, etc.
  • a test case expression, to match on a tuple representing the exception that should occur

Forcing people to use try ... catch in situations like that doesn't seem beneficial. Erlang doesn't have an effect system that makes the exception use explicit, so the problem can also relate to source code changing in the future and handling exceptions that are not documented.

@rickard-green rickard-green added the team:LG Assigned to OTP language group label Dec 9, 2024
@richcarl
Copy link
Contributor Author

richcarl commented Dec 9, 2024

@okeuday I agree that the old style is shorter to write for such cases, but the issue is that it almost always does the wrong thing. As a check in a test case, it's not specific enough, conflating different ways of termination. As a wrapper around calls it's usually too general, silently swallowing exceptions that you would have wanted to see and throwing away the stack trace, and just returning a term that ends up somewhere else entirely before it's detected.

I've spent so much time searching for the actual error reason in that kind of code that it's not funny. I think getting rid of old catches will be a big improvement on the error handling in a lot of old code, which will make maintenance and refactoring easier.

And if you want a future proof try that catches all exceptions forever, it's straightforward to write, with a clause like Class:Term:Trace -> ... or just _:_ -> .... But I always recommend thinking twice about catching every possible exception - if you're e.g. just catching throws for a nonlocal return, then let exits and errors pass through so crashes get reported properly.

@essen
Copy link
Contributor

essen commented Dec 9, 2024

I would say that this form is still useful:

_ = (catch m:f())

A shorthand for "it might crash and that's OK, I don't care, continue".

@richcarl richcarl force-pushed the deprecate-old-catch branch from 8d6fa24 to 1a3214e Compare December 9, 2024 09:53
@richcarl richcarl changed the base branch from master to maint December 9, 2024 09:55
@richcarl
Copy link
Contributor Author

richcarl commented Dec 9, 2024

@essen Yes, it's more convenient to write; the problem is what it produces. Maybe if that form and only that can be allowed, it would be ok. When the value is just shoved in an error term and passed on somewhere, the result is always a terrible debugging experience.

@bjorng
Copy link
Contributor

bjorng commented Jan 9, 2025

We like the first part of the plan, but we don't want this PR in OTP 27.3. We will accept this pull request for OTP 28 (with the warnings for the old catch disabled).

I personally like the idea of getting rid of old-style catches from the Kernel and STDLIB, because they hide types for the compiler and Dialyzer. With this machinery in place, we can start rewriting code in Kernel and STDLIB to eliminate old catches.

@richcarl richcarl changed the base branch from maint to master January 10, 2025 08:12
@richcarl richcarl force-pushed the deprecate-old-catch branch 2 times, most recently from bea4c29 to 5fba87b Compare January 10, 2025 09:08
@richcarl richcarl force-pushed the deprecate-old-catch branch from 5fba87b to 21471e4 Compare January 10, 2025 12:35
@bjorng bjorng self-assigned this Jan 10, 2025
@bjorng bjorng added the testing currently being tested, tag is used by OTP internal CI label Jan 10, 2025
Copy link
Contributor

@bjorng bjorng 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 to me.

The only thing missing is documentation of the new option.

@bjorng bjorng added testing currently being tested, tag is used by OTP internal CI and removed testing currently being tested, tag is used by OTP internal CI labels Jan 13, 2025
@bjorng bjorng merged commit 960dff7 into erlang:master Jan 13, 2025
24 of 26 checks passed
@richcarl richcarl deleted the deprecate-old-catch branch January 13, 2025 13:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
team:LG Assigned to OTP language group testing currently being tested, tag is used by OTP internal CI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants