Skip to content

Core typing: compat/config/fields/main/packet#2671

Merged
p-l- merged 3 commits intosecdev:masterfrom
gpotter2:mypy
Oct 8, 2020
Merged

Core typing: compat/config/fields/main/packet#2671
p-l- merged 3 commits intosecdev:masterfrom
gpotter2:mypy

Conversation

@gpotter2
Copy link
Copy Markdown
Member

@gpotter2 gpotter2 commented Jun 6, 2020

Kick off Hinty (#2158):

  • turn on Mypy strict mode (see this)
  • add typings to
    • packet.py
    • fields.py
    • config.py
    • main.py (strict)
    • __init__.py (strict)

Implementations notes:

  • this tries to be as clean and extensible as possible: we use Generic classes to make clean Fields. This means any other field that inherits from Field will be able to mark what Internal and Machine types it uses.
  • this PR fixes a few type errors that mypy triggered. For instance a function that would crash in SndRcvList and was moved to a parent class.
  • I refactored TimeSignedField because it was easier than cleaning it up.
  • This is fully compatible except one single thing: the use of Generic makes it impossible to use the keyword cls= as a key parameter. This keyword was only used in PacketListField (in other places, it was only used as an argument which is fine). Therefore I had to rename it to pkt_cls, but it's use was not that widespread because you would typically use it as an argument instead of a key argument.

To reproduce the issue with Generic:

from typing import Generic, TypeVar
I = TypeVar("I")
class A(Generic[I]):
    def __init__(self, cls=1):
        pass

A(cls=1)  # Error
  • This is a big PR, but had to be done in one go because those files just keep referring to each other. Making them one by one would mean adding a gazillion temporary # type: ignore. I hesitated to also do utils.py, but it's honnestly to big on its own and will need a separate PR.
  • python 2 compatibility was added, but it's quite a nightmare. See the code. I sometimes wish we had drop it. In any case, when will want to drop it (likely in a few years), we'll have the great com2ann module to do it for us.

@gpotter2 gpotter2 added the Hinty label Jun 6, 2020
Comment thread scapy/packet.py Outdated
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't understand the piece of code before this line (the for). Why do we use this for already? Can you get a field's value using getlayer ?!

@codecov
Copy link
Copy Markdown

codecov Bot commented Jun 6, 2020

Codecov Report

Merging #2671 into master will increase coverage by 0.00%.
The diff coverage is 89.75%.

@@           Coverage Diff           @@
##           master    #2671   +/-   ##
=======================================
  Coverage   88.20%   88.21%           
=======================================
  Files         254      254           
  Lines       54009    54086   +77     
=======================================
+ Hits        47639    47711   +72     
- Misses       6370     6375    +5     
Impacted Files Coverage Δ
scapy/__init__.py 81.63% <ø> (-0.37%) ⬇️
scapy/contrib/automotive/someip.py 93.77% <ø> (ø)
scapy/contrib/mqtt.py 97.22% <ø> (ø)
scapy/contrib/nfs.py 90.15% <ø> (ø)
scapy/contrib/portmap.py 97.14% <ø> (ø)
scapy/fields.py 91.80% <ø> (-0.72%) ⬇️
scapy/layers/sixlowpan.py 86.69% <ø> (ø)
scapy/packet.py 81.82% <85.47%> (+0.08%) ⬆️
scapy/config.py 84.55% <86.20%> (+0.04%) ⬆️
scapy/plist.py 87.46% <90.66%> (+0.19%) ⬆️
... and 14 more

Copy link
Copy Markdown
Member

@guedou guedou left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AWESOME PR. This is a huge an important work. Thanks a lot!

Did you manage to catch some bugs while typing these files?

I made comments, but most of them are related to use a type stricter than Any when possible.

Comment thread scapy/compat.py Outdated
Comment thread scapy/compat.py Outdated
Comment thread scapy/fields.py Outdated
Comment thread scapy/fields.py Outdated
Comment thread scapy/fields.py Outdated
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could Any be replace by a type associated to Field?

Copy link
Copy Markdown
Member Author

@gpotter2 gpotter2 Jun 11, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No (too bad :/) you can't for __eq__, __new__, __add__.... or anything that extends the object class.

It makes sense though, you can always try to use == on another type, so it should remain Any.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am sad =/

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it a limitation of the Python typing system? With other typed languages, we will be able to restrict the type of other. That sounds strange.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here's what mypy returns:

mypy run-test: commands[0] | python .config/mypy/mypy_check.py
scapy\fields.py:2541: error: Argument 1 of "__eq__" is incompatible with supertype "object"; supertype defines the argument type as "object"
scapy\fields.py:2541: note: This violates the Liskov substitution principle
scapy\fields.py:2541: note: See https://mypy.readthedocs.io/en/stable/common_issues.html#incompatible-overrides
scapy\fields.py:2541: note: It is recommended for "__eq__" to work with arbitrary objects, for example:
scapy\fields.py:2541: note:     def __eq__(self, other: object) -> bool:
scapy\fields.py:2541: note:         if not isinstance(other, FlagValue):
scapy\fields.py:2541: note:             return NotImplemented
scapy\fields.py:2541: note:         return <logic to compare two FlagValue instances>

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did some experiments and it seems that there is a --strict-equality mypy flag that could help detect things like Field(...) == 3. According to mypy documentation and the error message above, we should type ours __eq__() and __ne__() like:

def __eq__(self, other: object) -> bool:
    if not isinstance(other, FlagValue):
       return NotImplemented
    return <logic to compare two FlagValue instances>
```https://stackoverflow.com/questions/37557411/why-does-defining-the-argument-types-for-eq-throw-a-mypy-type-error

I will understand that you don't want to add the `if` block, but could you change `Any` to `object`?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@gpotter2 did you try this?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Except we don't want to do that, since we want to have

>>> FlagValue(1, ["a", "b", "c"])
<Flag 1 (a)>
>>> FlagValue(1, ["a", "b", "c"]) == "a"
True
>>> FlagValue(1, ["a", "b", "c"]) == 1
True

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I really believe that we want to enforce strict type checking. Otherwise, this makes typing quite unhelpful considering the work involved in annotating Scapy. The FlagValue is a valid counter example, but we should handle it separately.

I am OK with merging this PR, as soon as we move this stricter type checking discussions to a dedicated issue =)

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good, let's do that in a future PR. See #2823

Comment thread scapy/packet.py Outdated
Comment thread scapy/plist.py Outdated
Comment thread scapy/plist.py Outdated
Comment thread scapy/plist.py Outdated
Comment thread scapy/utils.py Outdated
@gpotter2 gpotter2 requested a review from guedou June 12, 2020 12:46
@guedou
Copy link
Copy Markdown
Member

guedou commented Jun 12, 2020

It seems that there is a conflict with packet.py. Could you fix before I review your last changes?

Comment thread scapy/compat.py Outdated
Comment thread scapy/fields.py Outdated
Comment thread scapy/fields.py Outdated
Comment thread scapy/fields.py Outdated
Comment thread scapy/fields.py Outdated
Comment thread scapy/fields.py Outdated
Comment thread scapy/fields.py Outdated
Comment thread scapy/fields.py Outdated
Comment thread scapy/fields.py Outdated
@gpotter2
Copy link
Copy Markdown
Member Author

I'm pretty sure I covered all your comments @guedou.
@guedou @p-l- I think it might be nice to start releasing 2.4.4 before merging this (?).

@p-l-
Copy link
Copy Markdown
Member

p-l- commented Jul 19, 2020

@gpotter2 agreed!

@guedou
Copy link
Copy Markdown
Member

guedou commented Aug 7, 2020

I tried the mypy --strict-equality option. It is able to catch things like "scapy" == 2807. but I did not manage to catch IP() == 42 yet.

Comment thread scapy/fields.py Outdated
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had quite some trouble making BitScalingField work in the first place on both Python 2 and Python 3 so its typing will be mostly ignored.

@gpotter2
Copy link
Copy Markdown
Member Author

gpotter2 commented Sep 11, 2020

@guedou @p-l- Hi, this is ready for review. I would like to have this merged as soon as possible because it's a pain to keep up to date with master. Thanks

Please squash on merge

This was referenced Sep 17, 2020
@guedou
Copy link
Copy Markdown
Member

guedou commented Sep 21, 2020

@p-l- what's your call? I am ok with merging this PR as it is.

@gpotter2 gpotter2 mentioned this pull request Sep 23, 2020
@gpotter2
Copy link
Copy Markdown
Member Author

gpotter2 commented Oct 3, 2020

@p-l- Could you please review this? Thanks

@p-l- p-l- merged commit 620dc57 into secdev:master Oct 8, 2020
@gpotter2 gpotter2 deleted the mypy branch October 8, 2020 20:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants