bug#10166 Make AssertionError fatal

[NthPortal]

Fixes scala/bug#10166

[NthPortal]

review by @SethTisue

[SethTisue]

this LGTM, but I'd like to know what @viktorklang and at least one other Scala team member think

[SethTisue]

tiny nit: missing backquotes around the added name

[NthPortal]

oops, you're right. will fix shortly

[NthPortal]

fixed now

[Ichoran]

I don't think it's a bug to catch AssertionError. Assertions are very often used for runtime checks that are no different in principle from an ArrayIndexOutOfBoundsException. Indeed, people use assert, which throws AssertionError, to do bounds checking.

I think this change will break a lot of working code for little benefit (or negative benefit). That a failed assertion is an error is itself, arguably, an error.

[SethTisue]

counterargument: AssertionError is an Error, ArrayIndexOutOfBoundsException is a RuntimeException, so I don't agree they are "no different in principle"

[NthPortal]

Shouldn't people be using require for runtime checks (especially as calls to assert may be elided)? It's my impression that require is for checks that a caller is passing something valid, whereas assert is for things that should never happen internally, but there might be a logic error which allows them.

[Ichoran]

I anticipated your counterargument with the "that it is an error is an error" comment :P

In practice, it is not used in cases where termination of the JVM is the only sane option.

[SethTisue]

okay. (I don't have a strong opinion of my own on this, actually.)

[NthPortal]

@Ichoran Do you have some examples of where people use AssertionError as described? It seems wrong to me, but if lots of people use it that way, then ¯\_(ツ)_/¯

[Ichoran]

@NthPortal - The scaladocs on assert don't state clearly that it if it fails your entire program should go down, uncatchably.

Furthermore, you find, even in the Scala library, things like

    if (idx < newlen) newelems(idx) = w
    else assert(w == 0L)

inside collection/BitSetLike.scala. Of course it's bad if you can't store your word, but it's not so bad that the JVM should terminate. If the bitset wasn't even there, it would at worst be an NPE which is catchable.

[Jasper-M]

IMO using assert for anything other than checking invariants is an error.

[NthPortal]

@Ichoran That's true, they don't. My assumption was that it was meant to have the same semantics as assert in Java, but that it shouldn't be baked into the language as a keyword. Especially given that it can be elided, which is similar to what Java does.

[Ichoran]

In case you want to check out how it's used in the Scala library, here's the output of a quick grep:

Enumeration.scala:    assert(!vmap.isDefinedAt(i), "Duplicate id: " + i)
collection/BitSetLike.scala:    else assert(w == 0L)
runtime/LambdaDeserializer.scala:    assert(targetMethodMap != null)
collection/concurrent/TrieMap.scala:          assert(inodemain ne null)
collection/concurrent/TrieMap.scala:    assert(ct.isReadOnly)
collection/immutable/HashSet.scala:    assert(Integer.bitCount(bitmap) == elems.length)
collection/immutable/RedBlackTree.scala:          assert(index >= stackOfNexts.length)
collection/immutable/TreeMap.scala:    assert(!RB.contains(tree, key))
collection/immutable/TreeSet.scala:    assert(!RB.contains(tree, elem))
collection/mutable/FlatHashTable.scala:    assert(_loadFactor > 0)
collection/mutable/FlatHashTable.scala:    assert(size >= 0)
collection/mutable/FlatHashTable.scala:        assert(assertion = false, i+" "+table(i)+" "+table.mkString)
collection/mutable/FlatHashTable.scala:    assert(lf < (loadFactorDenum / 2), "loadFactor too large; must be < 0.5")
collection/mutable/HashTable.scala:    assert(_loadFactor > 0)
collection/mutable/HashTable.scala:    assert(size >= 0)
collection/mutable/UnrolledBuffer.scala:      assert(next eq null)

I see far more things that while bad should be catchable than things that shouldn't be caught.

[NthPortal]

Looking at collection.BitSetLike, it's asserting an invariant, not checking a precondition.

if (idx >= newlen && w != 0L) newlen = idx + 1
if (idx < newlen) newelems(idx) = w
else assert(w == 0L)

If w != 0L, then newlen gets set to larger than idx, and the else is not executed. Thus, the else and the assert are only reachable if w == 0L.

I will have a look at the other uses in the library, but I suspect they all check invariants.

[SethTisue]

fwiw, I think require is the intended one for checking a precondition (it throws IllegalArgumentException)

whether, out in the wild, people know or respect the difference, I have no idea

[SethTisue]

if it fails your entire program should go down, uncatchably

@Ichoran "uncatchably" seems like hyperbole to me, changing the definition of NonFatal doesn't make things "uncatchable"

[viktorklang]

I think that there's a real risk that code will break due to this change.

[NthPortal]

Enumeration - is an invariant
runtime.LambdaDeserializer - I suspect it is considered an invariant because runtime should not be called by a library
collection.concurrent.TrieMap - looks like invariants, though I'm not an expert on its inner workings
collection.immutable.HashSet - is a precondition, not an invariant
collection.immutable.RedBlackTree - I actually don't see that one
collection.immutable.TreeMap - precondition, not invariant
collection.immutable.TreeSet - precondition, not invariant
collection.mutable.FlatHashTable - the 3 invariant, and I'm pretty sure the last one is too, but not certain
collection.mutable.HashTable - invariant
collection.mutable.UnrolledBuffer - I think invariant, but I'm really not sure

Though the majority of uses in the Scala library are as an invariant, at least 3 are not, and thus AssertionError probably should not be considered fatal right now.

[NthPortal]

I think it might be worth discussing whether or not the semantics of assert should be clarified or changed, and whether or not this is something to pursue in the future

[NthPortal]

It looks like this is not a good idea right now, possibly ever

[SethTisue]

thank you @NthPortal for the attempt. the ensuing discussion was valuable. I closed the original ticket as wontfix.