LGTM.com - false positive: py/unreachable-statement with assert

[gpotter2]

Platform: Python

Description of the false positive

assert calls will be ignored when using the -O option. Therefore some code shouldn't be considered as dead.

This is quite low priority. -O related bugs are very common, but no one really uses it. Linters have been fighting forever to know whether this should be taken into account, or simply discarded

test.py

def hello():
    assert False
    return 1

print(hello())

Results:

• python test.py -> AssertionError
• python -O test.py -> 1

Related doc:

-O
Remove assert statements and any code conditional on the value of __debug__. Augment the filename for compiled (bytecode) files by adding .opt-1 before the .pyc extension (see PEP 488). See also PYTHONOPTIMIZE.

URL to the alert on the project page on LGTM.com

https://lgtm.com/projects/g/secdev/scapy/snapshot/33177966dbe59a9c7cbed5b086f99b40c4b1ede7/files/scapy/fields.py?sort=name&dir=ASC&mode=heatmap#x551c9066f44c0ec2:1

[tausbn]

Alas, this is a case of "damned if you do, damned if you don't". If we were to treat assert as not always raising an exception, we would get people complaining in the other direction, saying that the code following the assert obviously isn't reachable, since the assert is there!

In fact, we already have a query that warns about uses of assert that may change if optimisations are applied: https://github.com/Semmle/ql/blob/master/python/ql/src/Statements/AssertLiteralConstant.ql

I'm curious, though, for the project alert you link, is there a good reason for ignoring the error when optimisations are enabled? It seems dangerous to me to have code that exhibits different behaviour depending on whether the code was "optimised" or not. (Assuming it is not tested thoroughly both with and without the -O flag.)

[gpotter2]

I'm curious, though, for the project alert you link, is there a good reason for ignoring the error when optimisations are enabled?

In my case, I actually removed the line anyways 😄.

I personally hate this corner case because some linters (Bandit used by codacy) will throw errors claiming that this qualifies as "Security Hazard".

Thus in my original message

Linters have been fighting forever to know whether this should be taken into account, or simply discarded

I see your point: it's impossible to please everyone.

I'd personally keep it the way it currently is: asserts are well used unlike -O. Still though, technically, this code is unsafe.

Feel free to pick the side LGTM will be on 😄