Type annotations for RDA and other analysis related code

[fmagin]

Lots of type annotations and fixups for various analysis related concepts and specifically the RDA.

Some of the annotations like 5a3b2cd might seem nearly useless, but they allow the IDE to infer that some access to this plugin is definitely of the type SimStateJNIReferences and methods/attributes that are the same as another class don't show up when searching for references to that member for the other class. All the annotations of networkx.DiGraph usage also serve this purpose, because often angr classes will have some member called successors which has a totally different meaning than DiGraph.successors.
Without annotations PyCharm can't realize that graph.successors is not a case where SimProcedure.successors is used, because graph might technically be of the type SimProcedure.

I am not sure how large the performance impact is to use exception based handling for the function handler. Instead of using hasattr. The massive issue with the hasattr/getattr approach is that it completely destroys the IDE analysis, and now it is at least possible to find every usage of FunctionHandler.handle_external_function and reason about the problem, instead of having to search the entire codebase for the string "handle_external_function"

[rhelmot]

So obvi we need to get CI passing, both lint and the testcase that seems to be failing because of the changes (I will not pretend to understand what's going on there), but beyond that, can you provide some sort of metric to indicate that the types you added are correct? Just any static analysis result that indicates the typing coverage has gone up or the number of type errors has gone down?

[fmagin]

My current solution for the FunctionHandler caused some testcases to fail still, but the issue that is left is fairly simple, I just didn't account for hasattr(obj, "attrname") also handling the case of obj being None, and forgot that I needed to add a check for that or handle that entirely different. I am going for entirely different. So the remaining testcases and lint issues aren't a problem.

A metric for the improvement is an interesting question, my main reason for writing the annotations in the first place was that it made it easier to work with the code in PyCharm, i.e. better type inference (and especially reducing false positives for some types) and fixing some lint issues PyCharm complains about by default. I'll look into setting up a mypy config that primarily detects wrong type annotations, so we can at least know if some type annotation leads to inconsistencies.

[fmagin]

@rhelmot now the only linting issue (and CI failure at all) left is:

LINT FAILURE: angr/analyses/reaching_definitions/reaching_definitions.py regressed to 9.95/10.00
... angr/analyses/reaching_definitions/reaching_definitions.py:106:8: W0233: init method from a non direct base class 'ForwardAnalysis' is called (non-parent-init-called)

which seems llike a bug in PyLint pylint-dev/pylint#3505
Though supposedly that should be addressed already, so either the version in the CI is outdated or we encountered some other issue. My guess is that it is the linked bug, and the fix didn't make it into the CI yet.

[rhelmot]

Alright - I'll take you at your word that this is an improvement. You should collaborate with @twizmwazin (kevin phoenix on slack) on getting the mypy pass set up in CI!

[ulugbekna]

Thanks a lot for the PR! I love seeing to add type annotations improving both ide experience and making closer typechecking with mypy

As side notes: Am I the only one who’s slightly concerned with

1. The PR diverging from the common style of using single quotes by using double quotes?
2. Also it seemed slightly inconsistent in using commented type annotations vs ordinary type annotations, ie f(x: ‘int’) vs f(x: int)

[fmagin]

@ulugbekna you raise some valid points:

The PR diverging from the common style of using single quotes by using double quotes?

kinda valid, I actually don't make a conscious decision for that and just assumed that the linter would complain if it is something that was deemed relevant...

Also it seemed slightly inconsistent in using commented type annotations vs ordinary type annotations, ie f(x: ‘int’) vs f(x: int)
the uncommented version isn't always possible due to possible import cycles. I am not sure if the recommendation would be to always use the quoted version, so far I basically use unquoted if possible (i.e. the types are already available at runtime) or quoted if this isn't easily possible for one of the various reasons.

Overall, how concerned are you? I am not sure if those are slight stylistic things, or if there are more severe implications I am missing. Probably not for single vs double quotes, but maybe for the quoted vs unquoted types.

[ulugbekna]

I’m afraid I’m not expert either. Development can happen incrementally so this PR is definitely a good step forward :-)

…

On Tue, 21 Dec 2021 at 22:55, Florian Magin ***@***.***> wrote: @ulugbekna <https://github.com/ulugbekna> you raise some valid points: The PR diverging from the common style of using single quotes by using double quotes? kinda valid, I actually don't make a conscious decision for that and just assumed that the linter would complain if it is something that was deemed relevant... Also it seemed slightly inconsistent in using commented type annotations vs ordinary type annotations, ie f(x: ‘int’) vs f(x: int) the uncommented version isn't always possible due to possible import cycles. I am not sure if the recommendation would be to always use the quoted version, so far I basically use unquoted if possible (i.e. the types are already available at runtime) or quoted if this isn't easily possible for one of the various reasons. Overall, how concerned are you? I am not sure if those are slight stylistic things, or if there are more severe implications I am missing. Probably not for single vs double quotes, but maybe for the quoted vs unquoted types. — Reply to this email directly, view it on GitHub <#3023 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AD4YR6YEAL5KPNASJGEFSE3USDZTZANCNFSM5KIRCIEA> . You are receiving this because you were mentioned.Message ID: ***@***.***>