update --warn-unreachable to respect if the code should be type checked - #10291

@JukkaL I'm not sure if there's anything I should do to move this forward

@JukkaL friendly ping, it looks like you (or someone else) allowed the CI to run so someone took the time to at least glance at this 😅

It would be nice to get this into the next release, which I'm sure you're busy with, since it's quite a small change, but right now we're disabling warn_unreachable on specific modules, and since config needs a whole module to perform a glob against we can't say *_tests,test_* which would make this issue less of a deal. We don't have many modules excluded at this point, but we're trying to increase the typing of our codebase and it would be nice to not have to silence this false positive 🙂

I can't speak for Jukka, but I think he's hesitant at this point to add new features to avoid destabilizing the release.

Based on the test case, the behavior from this PR also seems confusing: it makes reveal_type silently do nothing. It would be a better user experience if mypy emitted a message saying the reveal_type call is unreachable.

I can't speak for Jukka, but I think he's hesitant at this point to add new features to avoid destabilizing the release.

Ok, if it can't make it in that's fine. There's always the next release.

Based on the test case, the behavior from this PR also seems confusing: it makes reveal_type silently do nothing. It would be a better user experience if mypy emitted a message saying the reveal_type call is unreachable.

I could look into adjusting that, but that's actually not what the test case is doing. It's forcing the variable to be referenced which causes mypy to print the error. I just happened to use reveal_type to force the reference, but it could be anything.

This change looks good to me.

It is a little weird that we're not hitting

mypy/mypy/checkexpr.py

Line 3094 in 0964d9e

self.msg.note("'reveal_type' always outputs 'Any' in unchecked functions",

though, maybe it's something to do with the test stubs?

I looked at the stubs and refreshed my memory that the notes are also annotated in the stubs.

So I stepped through it, and while I had expected --trace to work passed to pytest, I was able to add breakpoints manually and then set breakpoints interactively in the visitors, but it looks like the reason there is no reveal_type note is because the code is unreachable and the visitor falls off when that happens:

Since the break happens before accepting the statement the visit_reveal_expr is never called for either case. I'll double check, but this probably means that using reveal_type in unreachable code already doesn't print anything.

Yup, that's correct:

def test(val: bool) -> None:
    assert val is True
    assert val is False
    reveal_type(val)

mypy --warn-unreachable test.py 
test.py:4: error: Statement is unreachable
Found 1 error in 1 file (checked 1 source file)

@JelleZijlstra I may have misunderstood your comment initially

Based on the test case, the behavior from this PR also seems confusing: it makes reveal_type silently do nothing. It would be a better user experience if mypy emitted a message saying the reveal_type call is unreachable.

That is still the case after this PR. This is the tests where a user is expecting to type their code:

And this is the case where an untyped function is expected to be ignored:

So I think the suboptimal thing here is that if you change test_untyped_fn to have a reveal_type(obj.prop) on the first line of the function, you'll get the note about the function being not checked, but you won't get it with the reveal_type(obj.prop) on the last line of the function because it's unreachable, and now that is completely silent.

Fixing that seems annoying / I think this change is still probably fine.

So I think the suboptimal thing here is that if you change test_untyped_fn to have a reveal_type(obj.prop) on the first line of the function, you'll get the note about the function being not checked, but you won't get it with the reveal_type(obj.prop) on the last line of the function because it's unreachable, and now that is completely silent.

Fixing that seems annoying / I think this change is still probably fine.

This PR doesn't change that based on this:

The break already happens either way, this PR just changes if the self.msg.unreachable_statement(s) is called or not.

Using your example I see:

def test(val):
    reveal_type(val)
    assert val is True
    assert val is False
    reveal_type(val)

by default unreachable line is missing and no warning:

mypy test.py
test.py:2: note: Revealed type is 'Any'                                       
test.py:2: note: 'reveal_type' always outputs 'Any' in unchecked functions

printing warnings, an untyped function is printing warnings this PR argues it shouldn't. The second reveal type message is still missing:

mypy --warn-unreachable test.py
test.py:2: note: Revealed type is 'Any'                                       
test.py:2: note: 'reveal_type' always outputs 'Any' in unchecked functions    
test.py:5: error: Statement is unreachable                                    
Found 1 error in 1 file (checked 1 source file)

checking all functions still doesn't warn about unreachable code by default:

mypy --check-untyped-defs  test.py
test.py:2: note: Revealed type is 'Any' 

a user who wants to everything checked still gets the message that their code is unreachable:

mypy --check-untyped-defs --warn-unreachable test.py
test.py:2: note: Revealed type is 'Any'                                       
test.py:5: error: Statement is unreachable  
Found 1 error in 1 file (checked 1 source file)

Fixing that seems annoying / I think this change is still probably fine.

I'm willing to look into this (in a separate PR if it's desired), but it would be pretty specific to reveal_type and any visitor that needs to visit every node regardless if it's unchecked or not. This will likely need to be some bit a la disabled_count that lets the code know it should still process in the "all node pass" or it would likely need to be a separate visitor that is accepted at every node and doesn't break in:

The first case would require updating all the existing visitors and making sure future editors know if they should actually be running or not, and the second case would require a 2nd full visit (I'm not sure how many passes there are against the AST already).

If the possible performance impact in traversing the tree 2x or maintaining the extra code path is worth it I'm up for trying to take a stab at it, but if either of you think it's not worth it I'll hold off until someone suggests it's something important to add.

I do run my code with --warn-unreachable because I would like to know where Mypy might be dropping off, and most of the time that's an error in our code and type information can be added/adjusted to make sure Mypy does indeed understand what type(s) an object is.

I don't think it's worth it! I'm also not aware of any other outputs mypy produces in unchecked functions that could result in false negatives with the reachability warning gone. I'm happy to merge as is, @JelleZijlstra ?

Sounds good. I'd hold off on merging though since Jukka seems to want to avoid more changes before a release.