Bug 12558 – try/catch allows implicit catching of Errors without specifying any Exception type

Status
RESOLVED
Resolution
FIXED
Severity
normal
Priority
P1
Component
dmd
Product
D
Version
D2
Platform
All
OS
All
Creation time
2014-04-11T10:34:00Z
Last change time
2016-10-01T11:48:02Z
Keywords
pull
Assigned to
andrej.mitrovich
Creator
andrej.mitrovich
Blocks
10247

Comments

Comment #0 by andrej.mitrovich — 2014-04-11T10:34:50Z
----- import std.stdio; void main() { try { assert(0); } catch writeln("Caught a Throwable"); } ----- $ dmd -run test.d > Caught a Throwable This is a misfeature and should be removed with prejudice. At the very least it should only ever attempt to catch Exceptions and not Errors/Throwables, but I fail to see how drowning exceptions without ever inspecting them is a good thing.
Comment #1 by andrej.mitrovich — 2014-04-11T10:37:14Z
(In reply to Andrej Mitrovic from comment #0) > $ dmd -run test.d > > Caught a Throwable Technically it caught an Error (AssertError), but anywho..
Comment #2 by yebblies — 2014-04-12T15:33:35Z
I dunno, catching asserts like that is useful. Why shouldn't the default be to catch absolutely everything?
Comment #3 by andrej.mitrovich — 2014-04-12T15:37:29Z
(In reply to yebblies from comment #2) > I dunno, catching asserts like that is useful. Why shouldn't the default be > to catch absolutely everything? Because you're potentially drowning a serious error and it's really hard to spot this in the code. But we already have library workarounds for exactly this purpose, e.g. collectException!Throwable( your_stuff_here );
Comment #4 by nick — 2014-04-19T11:09:55Z
(In reply to Andrej Mitrovic from comment #3) > (In reply to yebblies from comment #2) > > I dunno, catching asserts like that is useful. Why shouldn't the default be > > to catch absolutely everything? > > Because you're potentially drowning a serious error and it's really hard to > spot this in the code. Indeed, and we recommend *not* to catch Error. Doing it silently hides important bugs. > But we already have library workarounds for exactly this purpose, e.g. > collectException!Throwable( your_stuff_here ); or just: catch(Throwable){...} Perhaps we could add a compiler warning for catch without an explicit type.
Comment #5 by monarchdodra — 2014-04-21T20:11:45Z
"catch" should only be catching exceptions, for the same reason that "nothrow" only makes promises about exceptions. Errors (or more generally Throwables) are NOT the standard, and should always be dealt with explicitly. Besides, how hard is it to just write: "catch(Throwbale)"
Comment #6 by andrej.mitrovich — 2014-04-21T20:14:05Z
(In reply to monarchdodra from comment #5) > Besides, how hard is it to just write: "catch(Throwbale)" The general rule is: When there's a way to abuse the system, it /will/ be abused. The only reason why I found out about this feature is because it's used in the dub project. I was surprised it even compiled, and doubly-surprised that it's actually catching Throwables.
Comment #7 by briancschott — 2014-06-06T20:06:00Z
DMD can't even parse this syntax correctly. I also support removing it.
Comment #8 by andrej.mitrovich — 2014-08-23T17:01:07Z
Edited title to refer to Errors rather than Throwables, since that's the actual problematic part.
Comment #9 by code — 2014-11-27T08:42:15Z
So, anyone wants to implement the compiler warning to start deprecating this?
Comment #10 by dlang-bugzilla — 2014-11-27T08:46:05Z
The pull request exists, Andrej filed it in the URL field: https://github.com/D-Programming-Language/dmd/pull/3482
Comment #11 by dlang-bugzilla — 2015-10-08T04:12:02Z
(In reply to Vladimir Panteleev from comment #10) > The pull request exists, Andrej filed it in the URL field: > > https://github.com/D-Programming-Language/dmd/pull/3482 This pull request now has conflicts and Andrej apparently deleted his GitHub account, so I think it can only be closed now. Is anyone interested in resurrecting it?
Comment #12 by andrej.mitrovich — 2015-10-08T08:14:02Z
I'll recreate the pull today or tomorrow. I'll do the same for the other stale pulls of mine in the coming days.
Comment #13 by nick — 2016-04-25T14:15:52Z
(Linked pull 5183 now has conflicts). I discovered this with dmd 2.071.0: @safe unittest { try throw new Exception(""); catch {} // Error: can only catch class objects derived from Exception in @safe code, not 'object.Throwable' } But I think the error should occur in @system code too.
Comment #14 by andrej.mitrovich — 2016-05-06T22:01:38Z
CC Andrei for approval, then I'll update the PR.
Comment #15 by andrei — 2016-06-04T19:39:20Z
Yes, catching Errors should always be explicit.
Comment #16 by github-bugzilla — 2016-07-07T09:46:42Z
Commits pushed to master at https://github.com/dlang/dmd https://github.com/dlang/dmd/commit/9ebcbed84fcc3cb065d506180e3323d976ca266a Fix Issue 12558 - Deprecate implicit catch statements https://github.com/dlang/dmd/commit/18e6844d41e34cd67df713d31bdf350927e7d5ec Merge pull request #5183 from AndrejMitrovic/fix-12558 Issue 12558 - Deprecate implicit catch statements
Comment #17 by github-bugzilla — 2016-07-12T02:08:04Z
Commits pushed to master at https://github.com/dlang/dmd https://github.com/dlang/dmd/commit/b10643986838584198db382c63e488aab4e5b343 Issue 12558 - Document implicit catch statement deprecation in the changelog See dlang/dmd#5183 and dlang/dlang.org#1423 https://github.com/dlang/dmd/commit/24f0d1cabb2df960e84f9b2e9204d4cd7a8f4ed6 Merge pull request #5930 from Geod24/implicit-catch-statement Issue 12558 - Document implicit catch statement deprecation in the changelog
Comment #18 by github-bugzilla — 2016-07-12T08:31:53Z
Commits pushed to master at https://github.com/dlang/dlang.org https://github.com/dlang/dlang.org/commit/458ff506e385c05c8eb9f08b6915267c662780c3 Issue 12558 - Document implicit catch statement deprecation See dlang/dmd#5183 https://github.com/dlang/dlang.org/commit/51b3a5808c191dd2d04ebdf718d3f411c1468b6c Merge pull request #1423 from Geod24/throwable Issue 12558 - Document implicit catch statement deprecation
Comment #19 by github-bugzilla — 2016-10-01T11:45:40Z
Commits pushed to stable at https://github.com/dlang/dlang.org https://github.com/dlang/dlang.org/commit/458ff506e385c05c8eb9f08b6915267c662780c3 Issue 12558 - Document implicit catch statement deprecation https://github.com/dlang/dlang.org/commit/51b3a5808c191dd2d04ebdf718d3f411c1468b6c Merge pull request #1423 from Geod24/throwable
Comment #20 by github-bugzilla — 2016-10-01T11:48:02Z
Commits pushed to stable at https://github.com/dlang/dmd https://github.com/dlang/dmd/commit/9ebcbed84fcc3cb065d506180e3323d976ca266a Fix Issue 12558 - Deprecate implicit catch statements https://github.com/dlang/dmd/commit/18e6844d41e34cd67df713d31bdf350927e7d5ec Merge pull request #5183 from AndrejMitrovic/fix-12558 https://github.com/dlang/dmd/commit/b10643986838584198db382c63e488aab4e5b343 Issue 12558 - Document implicit catch statement deprecation in the changelog https://github.com/dlang/dmd/commit/24f0d1cabb2df960e84f9b2e9204d4cd7a8f4ed6 Merge pull request #5930 from Geod24/implicit-catch-statement