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

AssertionError should be considered fatal #10166

Closed
scabug opened this issue Jan 26, 2017 · 18 comments
Closed

AssertionError should be considered fatal #10166

scabug opened this issue Jan 26, 2017 · 18 comments
Labels

Comments

@scabug
Copy link

scabug commented Jan 26, 2017

AssertionError s, being outside the list of fatal errors defined for NonFatal, are caught by code matching on NonFatal and with the Try construct - making it easy for failed assertions to be inappropriately handled or silently discarded where they should be considered fatal application failures and fail loudly to inform the developer/tester that a program assumption/assertion has been broken and requires remediation.

@scabug
Copy link
Author

scabug commented Jan 26, 2017

Imported From: https://issues.scala-lang.org/browse/SI-10166?orig=1
Reporter: Chris Paro (cparo)

@scabug
Copy link
Author

scabug commented Jan 26, 2017

@som-snytt said:
That's an interesting question. Either, if I can assert something, I can handle the complementary failure; or, I assert something because I can't handle failure of an invariant. The latter semantics is persuasive because I'm lazy and because if I understood the failure mode, I'd have thrown something more specific.

@scabug
Copy link
Author

scabug commented Feb 8, 2017

Jasper-M said:
I think in general assertions are used to test invariants or preconditions, whose failures are explicitly not handled.

@scabug scabug added the quickfix label Apr 7, 2017
NthPortal added a commit to NthPortal/scala that referenced this issue Feb 11, 2018
NthPortal added a commit to NthPortal/scala that referenced this issue Feb 13, 2018
@Ichoran
Copy link

Ichoran commented Feb 22, 2018

As I commented on the PR, I think AssertionError is misnamed. It is used for things like bounds checks, which makes it no different conceptually (in many cases, anyway) from an ArrayIndexOutOfBoundsError. Also, switching it now will probably break a bunch of code that relied on catching assertions.

@SethTisue
Copy link
Member

PR: scala/scala#6320

@SethTisue SethTisue added this to the 2.13.0-RC1 milestone Feb 22, 2018
NthPortal added a commit to NthPortal/scala that referenced this issue Feb 22, 2018
@viktorklang
Copy link

Is there enough information to be able to say that "all" AssertionErrors are fatal?

@SethTisue
Copy link
Member

@viktorklang tbh I don't really understand why any Error would be considered NonFatal. the JVM already has the Error vs non-Error distinction, I've never been especially clear on why NonFatal adds an additional level of classification.

@SethTisue
Copy link
Member

is there a canonical example (or two or three) of an Error that you consider to be definitely NonFatal?

@NthPortal
Copy link

NthPortal commented Feb 22, 2018

for AssertionError in particular, it is non-fatal in some cases - see this comment

@NthPortal
Copy link

most of the discussion for this seems to have happened at scala/scala#6320 (don't know why)

@viktorklang
Copy link

@SethTisue «An Error is a subclass of Throwable that indicates serious problems that a reasonable application should not try to catch. » - https://docs.oracle.com/javase/8/docs/api/java/lang/Error.html

The problem is that there are quite a few unreasonable applications out there, and since Error is extendable it is impossible to say that all Errors are fatal.

ThreadDeath, for instance, is a NonFatal Error.

@NthPortal
Copy link

to be fair, ThreadDeath is deprecated(ish)

@viktorklang
Copy link

@NthPortal To be fair, I only checked JavaSE ;)

@lrytz
Copy link
Member

lrytz commented Feb 22, 2018

It's a datapoint a bit far fetched, but IntelliJ shows AssertionError and OutOfMemoryError differently (red and orange). I don't know how they make the distinction.

image

@Ichoran sorry to nit-pick, but it's ArrayIndexOutOfBoundsException not -Error.

In my understanding, the errors that are excluded in NonFatal are things that an application really should not try to recover from, so my tendency is to keep AssertionError non-fatal.

@NthPortal
Copy link

@lrytz I think that's just because AssertionError is usually thrown for test failure

... which is another reason it should stay non-fatal, as a good testing harness shouldn't catch fatals (although it could specifically go out of its way to catch AssertionError and not other fatals)

@NthPortal
Copy link

Interestingly, I get a different icon for OOM (using scalatest)

image

(AssertionError, IllegalStateException, and OutOfMemoryError respectively)

@SethTisue
Copy link
Member

looks like this is not-a-bug/wontfix

perhaps there's room for a new PR that improves the Scaladoc for NonFatal, though.

@SethTisue
Copy link
Member

quoth @Ichoran on Gitter:

"The usage of AssertionError as generated by assert is governed by practical considerations of what works rather than first principles; and as people have likely come up with different conventions that work in practice, any change is likely to be breaking. Since the lack of convention extends into usage in Java (which we cannot substantively affect), it is also not practical to develop first principles. So leaving it alone is likely the best we can manage for now."

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

7 participants