Core typing: compat/config/fields/main/packet#2671
Core typing: compat/config/fields/main/packet#2671p-l- merged 3 commits intosecdev:masterfrom gpotter2:mypy
Conversation
There was a problem hiding this comment.
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 Report
@@ Coverage Diff @@
## master #2671 +/- ##
=======================================
Coverage 88.20% 88.21%
=======================================
Files 254 254
Lines 54009 54086 +77
=======================================
+ Hits 47639 47711 +72
- Misses 6370 6375 +5
|
guedou
left a comment
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Could Any be replace by a type associated to Field?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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>
There was a problem hiding this comment.
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`?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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 =)
There was a problem hiding this comment.
Sounds good, let's do that in a future PR. See #2823
|
It seems that there is a conflict with |
|
@gpotter2 agreed! |
|
I tried the |
There was a problem hiding this comment.
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.
|
@p-l- what's your call? I am ok with merging this PR as it is. |
|
@p-l- Could you please review this? Thanks |
Kick off Hinty (#2158):
Implementations notes:
Genericclasses to make cleanFields. This means any other field that inherits fromFieldwill be able to mark whatInternalandMachinetypes it uses.SndRcvListand was moved to a parent class.TimeSignedFieldbecause it was easier than cleaning it up.Genericmakes it impossible to use the keywordcls=as a key parameter. This keyword was only used inPacketListField(in other places, it was only used as an argument which is fine). Therefore I had to rename it topkt_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:# type: ignore. I hesitated to also doutils.py, but it's honnestly to big on its own and will need a separate PR.com2annmodule to do it for us.