types: Add assert_never for exhaustivenes checking with mypy

[ghost]

This PR addresses issue #835, and should help with exhaustiveness checking for types. Running mypy capa/rules.py showed no errors before I made these changes.

Output of mypy capa/rules.py after change:

capa/rules.py:1095: error: Argument 1 to "assert_never" has incompatible type "Statement"; expected "NoReturn"
capa/rules.py:1200: error: Argument 1 to "assert_never" has incompatible type "Scope"; expected "NoReturn"
Found 2 errors in 1 file (checked 1 source file)

Checklist

• No CHANGELOG update needed

• No new tests needed

• No documentation update needed

[williballenthin]

The code you added here is just what I had in mind for #835 - nice!

However, mypy doesn't seem to be recognizing the narrowed types. The error around line 1200 is most clear: although we've handled all cases (file, function, bb), mypy is worried about the type Scope (which is the enum container, not any of the values). Do you have any idea why this is or how we can fix it?

[williballenthin]

ahh, for the Scope errors (line 1200) there are two pre-existing mistakes:

1. cases should check against Scope.FOO rather than scope.FOO (which is a local variable), and
2. cases should use foo is enum.BAR rather than foo == enum.BAR

I found this by copying the example in the blog post and using reveal_type with changes until it worked. I was confused because the LSP/type checker in my vim editor resolved the types better than mypy was doing.

[williballenthin]

I don't have edit permissions for your repo, so @Cl3o would you update this PR with the above changes?

[williballenthin]

looking next at the other mypy error...

[ghost]

Ahh, okay. I noticed those warnings from mypy too, and I wasn't sure if it was just on my end or not. Do you want me to just do a fixup for those errors real quick?
(and also fix the linting issues, that i somehow missed)
Edit: Sorry, didn't see that comment. I can just give you edit permissions, if you'd like.

[williballenthin]

I can just give you edit permissions, if you'd like.

doesn't matter either way, I was just trying to save you a step before tagging you to request the edit.

[ghost]

Okay. I'll make the edit in a bit.
Thank you so much for all the feedback.

[williballenthin]

for the other case, the checking of Statement, i don't think we can convince mypy to do what we want.

what we want is to have mypy do exhaustiveness checking on all the subtypes of class Statement; however, i don't think this is something that mypy is capable of tracking (imagine: anyone can come along and define a new subtype of Statement so how would mypy know what is possible?). therefore, we should add a further explicit check to the if/elif/else branches like this:

here's the code as proposed above:

            elif isinstance(node, ceng.Statement):
                # unhandled type of statement.
                # this should only happen if a new subtype of `Statement`
                # has since been added to capa.
                #
                # ideally, we'd like to use mypy for exhaustiveness checking
                # for all the subtypes of `Statement`.
                # but, as far as i can tell, mypy does not support this type
                # of checking.
                #
                # in a way, this makes some intuitive sense:
                # the set of subtypes of type A is unbounded,
                # because any user might come along and create a new subtype B,
                # so mypy can't reason about this set of types.
                assert False, f'Unhandled value: {node} ({type(node).__name__})'

@Cl3o if you agree, would you add this check? then, we'll be happy to merge!

[ghost]

About the first change - I get a warning in PyCharm:
Local variable 'easy_rules_by_feature' might be referenced before assignment
at easy_rule_names = easy_rules_by_feature.get(feature) (line 1219). It suggests creating a global statement. Should I just ignore this?

[williballenthin]

About the first change - I get a warning in PyCharm:

can you introduce a default value for the local variable, like easy_rules_by_feature = {} right at the start of method match()? PyCharm must not be able to see into the assert_never to understand that this case should be impossible.

[ghost]

I think that's everything. Thank you for being so patient with me. :)

[williballenthin]

no, thank you! this looks great and it was easy to work with you.

(no pressure) would you like to find or be recommended another capa issue?

[ghost]

I'd love to work on another issue. Do you have anything specific in mind?

[williballenthin]

here are a few issues that won't require a very deep knowledge of capa's internals.

#430
#103
#331
#377
#836
#749
#756

feel free to assign yourself any of these and open a PR. also, we're happy to discuss the requirements more if its unclear what is needed. we'd like to make it as easy as possible for contributors, so let us know what we can do to make your task easier!

[williballenthin]

once you're comfortable with how capa works, bigger features would be #376 and #322

[ghost]

I think I'll start with #331 and go from there. :)

[ghost]

Sorry. I wasn't sure where to ask this, so I'm just writing it here. What does it mean to be invited as a collaborator?

[williballenthin]

i had attempted to invite you so that i could "assign" the issue to you:

feel free to reject or ignore, its just project bookkeeping

[ghost]

Ahh, okay. That's fine then.