Enable mypy-zope plugin #471

wsanchez · 2021-03-11T21:54:39Z

Enable Mypy-Zope so that we can properly type check the use of Zope Interface classes and remove the various hacks we have in place to make Mypy happy without the plugin.

wsanchez · 2021-03-11T22:00:39Z

Getting an INTERNAL ERROR after just enabling it:

GLOB sdist-make: /home/runner/work/klein/klein/setup.py
mypy create: /home/runner/work/klein/klein/.tox/mypy
mypy installdeps: mypy==0.812, mypy-zope==0.2.11, Twisted==20.3.0, attrs==20.3.0, Automat==20.2.0, characteristic==14.3.0, constantly==15.1.0, hyperlink==21.0.0, incremental==17.5.0, PyHamcrest==2.0.2, six==1.15.0, Tubes==0.2.0, Werkzeug==1.0.1, zope.interface==5.2.0
mypy inst: /home/runner/work/klein/klein/.tox/.tmp/package/1/klein-20.6.0.zip
mypy installed: attrs==20.3.0,Automat==20.2.0,characteristic==14.3.0,constantly==15.1.0,hyperlink==21.0.0,idna==3.1,incremental==17.5.0,klein @ file:///home/runner/work/klein/klein/.tox/.tmp/package/1/klein-20.6.0.zip,mypy==0.812,mypy-extensions==0.4.3,mypy-zope==0.2.11,PyHamcrest==2.0.2,six==1.15.0,Tubes==0.2.0,Twisted==20.3.0,typed-ast==1.4.2,typing-extensions==3.7.4.3,Werkzeug==1.0.1,zope.event==4.5.0,zope.interface==5.2.0,zope.schema==6.1.0
mypy run-test-pre: PYTHONHASHSEED='688791434'
mypy run-test: commands[0] | mypy --cache-dir=/home/runner/work/klein/klein/.tox/mypy_cache release.py setup.py src
src/klein/test/test_trial.py:33: error: INTERNAL ERROR -- Please try using mypy master on Github:
https://mypy.rtfd.io/en/latest/common_issues.html#using-a-development-mypy-build
If this issue continues with mypy master, please report a bug at https://github.com/python/mypy/issues
version: 0.812
src/klein/test/test_trial.py:33: : note: please use --show-traceback to print a traceback when reporting a bug
ERROR: InvocationError for command /home/runner/work/klein/klein/.tox/mypy/bin/mypy --cache-dir=/home/runner/work/klein/klein/.tox/mypy_cache release.py setup.py src (exited with code 2)
___________________________________ summary ____________________________________
ERROR:   mypy: commands failed
Error: Process completed with exit code 1.

Filed as Shoobx/mypy-zope#37

…y` without `mypy-zope`.

wsanchez · 2021-03-12T01:00:52Z

src/klein/_session.py

@@ -123,7 +123,7 @@ def procureSession(
            ]
            # Does it seem like this check is expensive? It sure is! Don't want
            # to do it? Turn on your dang HTTPS!
-            yield self._store.sentInsecurely(allPossibleSentTokens)
+            self._store.sentInsecurely(allPossibleSentTokens)


sentInsecurely() returns None and is not async, so mypy complains about this unnecessary yield.

wsanchez · 2021-03-12T01:04:11Z

src/klein/test/test_trial.py

+class IFrobbable(Interface):
    """
-    Tests for L{TestCase}.
+    Frobbable object.
    """

-    class IFrobbable(Interface):
+    def frob() -> None:
        """
-        Frobbable object.
+        Frob the object.
        """

-        @ifmethod
-        def frob() -> None:
-            """
-            Frob the object.
-            """
+
+class TestCaseTests(TestCase):
+    """
+    Tests for L{TestCase}.
+    """


Moved to module scope to make mypy-zope happy.

wsanchez · 2021-03-12T01:07:15Z

src/klein/_isession.py

    """
    Interface for adding hooks to the phases of a request's lifecycle.
    """

+    def addPrepareHook(


Users of IRequestLifecycle are expecting this.

wsanchez · 2021-03-12T01:08:14Z

src/klein/storage/_memory.py

@@ -55,7 +55,7 @@ class _MemoryAuthorizerFunction:
    Type shadow for function with the given attribute.
    """

-    __memoryAuthInterface__: IInterface = None
+    __memoryAuthInterface__: IInterface = None  # type: ignore[assignment]


Don't allow this to be set to None outside of init.

I think that this is fine.
Another options is to __memoryAuthInterface__ = IUndeclared or something like that

then in fromAuthorizers raise an error if IUndeclared is found.

I see that current code is not doing any checks for None - https://github.com/twisted/klein/pull/471/files#diff-02fadd59dbc54e2a28f4f2c604d476196047564bb71c63cea2e8c3f74c56b701R114

I generally agree, though I think None is fine in this context and don't see there need to define an IUndeclared.

I added an assertion above the line were __memoryAuthInterface__ is accessed to make sure it isn't still None.

wsanchez · 2021-03-12T01:08:47Z

src/klein/_form.py

@@ -458,6 +458,11 @@ class IProtoForm(Interface):
    Marker interface for L{ProtoForm}.
    """

+    def addField(field: Field) -> "FieldInjector":


Users of IProtoForm expect this.

adiroiban

Looks so much cleaner now :)

I think the interface updates might break in theory backward compatibility.
I have no idea how to implement bacward compatible interface changes.
I guess the only options is IProtoForm and then IProtoFormV2(IProtoForm)

I have no idea how many klein users are implementing their own forms.

Thanks!

adiroiban · 2021-03-12T11:51:56Z

src/klein/storage/_memory.py

@@ -55,7 +55,7 @@ class _MemoryAuthorizerFunction:
    Type shadow for function with the given attribute.
    """

-    __memoryAuthInterface__: IInterface = None
+    __memoryAuthInterface__: IInterface = None  # type: ignore[assignment]


I think that this is fine.
Another options is to __memoryAuthInterface__ = IUndeclared or something like that

then in fromAuthorizers raise an error if IUndeclared is found.

I see that current code is not doing any checks for None - https://github.com/twisted/klein/pull/471/files#diff-02fadd59dbc54e2a28f4f2c604d476196047564bb71c63cea2e8c3f74c56b701R114

wsanchez · 2021-03-12T16:34:35Z

src/klein/storage/_memory.py

@@ -55,7 +55,7 @@ class _MemoryAuthorizerFunction:
    Type shadow for function with the given attribute.
    """

-    __memoryAuthInterface__: IInterface = None
+    __memoryAuthInterface__: IInterface = None  # type: ignore[assignment]


We don't want to encourage assigning None after initialization.

Don't need to accept Any; object will do.

… mypy-zope

wsanchez · 2021-03-12T17:10:49Z

src/klein/test/test_request.py

+        # type note: This returns a FrozenHTTPRequest, which implements
+        # IHTTPRequest, which is a subclass of IHTTPMessage.
+        # Seems like a bug in mypy-zope.
+        return cls.requestFromBytes(data)  # type: ignore[return-value]


File a bug on mypy-zope and update this comment with a link

wsanchez · 2021-03-12T17:10:57Z

src/klein/test/test_response.py

+        # type note: This returns a FrozenHTTPResponse, which implements
+        # IHTTPResponse, which is a subclass of IHTTPMessage.
+        # Seems like a bug in mypy-zope.
+        return cls.responseFromBytes(data)  # type: ignore[return-value]


File a bug on mypy-zope and update this comment with a link

wsanchez · 2021-03-12T17:29:17Z

I think the interface updates might break in theory backward compatibility.
I have no idea how to implement bacward compatible interface changes.
I guess the only options is IProtoForm and then IProtoFormV2(IProtoForm)

I have no idea how many klein users are implementing their own forms.

There are still some mypy errors here, as there are a lot of these "marker interfaces" in play, and I don't understand why we have interfaces with zero defined API in them and then there's this injection magic, etc.

I don't use (or really understand) this API at all, so… shrug… it seems like the API I added to IProtoForm, for example, probably should really be there, as it's expected by the internal machinery, so I suspect that if you defined you own, that it would break at runtime when these attributes are accessed, if not due to the interface being incomplete according to Zope. But again, I don't know what this really does.

@glyph, I think this is your creation, so it would be nice if you had a few cycles to clean up the last bits here…

glyph · 2021-03-12T20:01:01Z

@glyph, I think this is your creation, so it would be nice if you had a few cycles to clean up the last bits here…

I'm (supposedly) taking the day off but filtering this kind of email is a project that is hard to bother with for less than a week's worth of vacation so I guess I might as well reply :).

As far as I can tell the interface changes that Adi raised concerns about are fine, because:

we weren't publishing any type stubs before, and
none of the code in question was actually executing at runtime.

I think I can find a couple of cycles next week to address the stuff in the implementation code but most of the stuff in test code is boring enough that I'll cast or type: ignore it away.

glyph · 2021-03-12T20:01:26Z

it seems like the API I added to IProtoForm, for example, probably should really be there, as it's expected by the internal machinery, so I suspect that if you defined you own, that it would break at runtime when these attributes are accessed, if not due to the interface being incomplete according to Zope.

I agree with this, GVOS applies

glyph · 2021-03-12T20:05:43Z

I don't understand why we have interfaces with zero defined API in them

some of them, as you observed, are just wrong, because mypy was previously providing no assistance and Any-ing us into apparent safety when it wasn't there.

However, some of them are to satisfy some mechanical requirements because they require more from the technology than it can currently provide. For example: IParsedJSONBody is intentionally Any-ish because it's a dict with certain properties which are impossible to describe using Mypy's type language (if it could be specified, it'd be some derivate of a TypedDict with higher-kinded dependent types requiring input from the application — again, in a format which currently doesn't exist — and also there's some serialization boundaries which are presently implicit). However, it needs to be an Interface to satisfy the fact that it fits into a Componentized and Componentized requires an IInterface as a key.

Discarding some of the jargon here it's like the problem with functions that modify a function signature by adding or removing an expected positional or keyword argument. There's just no way to tell Mypy "oh, it's like whatever function you got, but wiht these modifications", much as you can't say "well it's like a Dict but one which has been explicitly set up to have some keys based on this runtime data".

glyph · 2021-03-12T20:06:18Z

All of that could be more clearly described than "marker interface" so maybe I'll give that a shot as well.

glyph · 2021-03-12T20:12:54Z

IForm and IProtoForm by contrast should probably be more or less wholesale copies of the public attributes on ProtoForm and Form since those are "markers" in that the system requires an Interface but they are not publicly exported; no IForm or IProtoForm in interfaces.py, you'll note.

wsanchez · 2021-03-12T23:50:42Z

I agree with this, GVOS applies

Garden Valley Oil Services?

glyph · 2021-03-13T01:39:33Z

I agree with this, GVOS applies

Garden Valley Oil Services?

Gross Violation Of Specifications, from the twisted compat policy

…ute in IProtoForm.

…-zope-plugin

codecov · 2021-03-17T20:06:51Z

Codecov Report

Merging #471 (db1c79f) into master (39f4b79) will decrease coverage by 0.64%.
The diff coverage is 91.93%.

@@            Coverage Diff             @@
##           master     #471      +/-   ##
==========================================
- Coverage   38.38%   37.74%   -0.65%     
==========================================
  Files          45       44       -1     
  Lines        3887     3850      -37     
  Branches      249      249              
==========================================
- Hits         1492     1453      -39     
- Misses       2392     2394       +2     
  Partials        3        3

Impacted Files	Coverage Δ
src/klein/_imessage.py	`100.00% <ø> (ø)`
src/klein/interfaces.py	`100.00% <ø> (ø)`
src/klein/_session.py	`38.94% <66.66%> (-0.64%)`	⬇️
src/klein/storage/_memory.py	`60.00% <85.71%> (-1.43%)`	⬇️
src/klein/_form.py	`59.35% <90.90%> (+1.45%)`	⬆️
src/klein/_dihttp.py	`58.49% <100.00%> (-0.77%)`	⬇️
src/klein/_headers.py	`40.47% <100.00%> (-0.71%)`	⬇️
src/klein/_interfaces.py	`100.00% <100.00%> (ø)`
src/klein/_isession.py	`100.00% <100.00%> (ø)`
src/klein/_request.py	`91.30% <100.00%> (ø)`
... and 9 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 39f4b79...db1c79f. Read the comment docs.

Enable mypy-zope plugin

04a536c

wsanchez added the feature label Mar 11, 2021

wsanchez self-assigned this Mar 11, 2021

wsanchez requested a review from a team as a code owner March 11, 2021 21:54

Merge branch 'master' into mypy-zope-plugin

dd92cc4

wsanchez added 9 commits March 11, 2021 15:06

Remove the TYPE_CHECKING shenanigans we've been using to appease `myp…

6e5de4c

…y` without `mypy-zope`.

Move IFrobbable to module scope to avoid trouble with mypy-zope.

3b954f0

yield is unnecessary; ISessionStore.sentInsecurely() is not async.

0a66650

Unused ignores

eb20c06

This interface is intentionally incomplete, for testing.

bcd7bb9

url_for does not take a request.

644660c

Multiple users expect addPrepareHook() to hang off of IRequestLifecycle.

afaf266

Add addField() to IProtoForm

e46ee77

Allow __memoryAuthInterface__ to start as None, but not set to None

040c82f

wsanchez commented Mar 12, 2021

View reviewed changes

adiroiban approved these changes Mar 12, 2021

View reviewed changes

wsanchez commented Mar 12, 2021

View reviewed changes

wsanchez added 3 commits March 12, 2021 08:56

Argument is an interface type, not an interface instance.

8e557c6

Don't need to accept Any; object will do.

Returns int

0323edc

This looks correct but is being flagged as wrong; seems like a bug in…

0b2d7a0

… mypy-zope

wsanchez commented Mar 12, 2021

View reviewed changes

Make sure authorizer.__memoryAuthInterface__ isn't still None

b46383c

wsanchez added 2 commits March 16, 2021 08:25

Fill in IForm and IProtoForm

9f4f367

mypy-zope fixed the nested class issue (Shoobx/mypy-zope#37)

950e874

wsanchez mentioned this pull request Mar 16, 2021

Retuning an instance of the class being defined which provides an interface is not handled Shoobx/mypy-zope#39

Open

wsanchez added 11 commits March 16, 2021 11:06

Make IValidationFailureHandler the correct shape to fit into _call()

af42cf5

cast RequestURL for now

dfb5032

Move private _fields attribute from ProtoForm to public fields attrib…

5f65d42

…ute in IProtoForm.

Poop if self._field.pythonArgumentName is still None when we look it up

57c787d

Clean up IForm usage

6ef0b63

Merge branch 'master' into mypy-zope-plugin

f701939

cast RequestURL to IRequiredParameter for now

ca3c4dc

Merge branch 'mypy-zope-plugin' of github.com:twisted/klein into mypy…

e039449

…-zope-plugin

IInterface ➜ Type[Interface]

dda05cf

Bump incremental

aaaeaea

CI doesn't need these…

db1c79f

wsanchez merged commit 2e9c3d4 into master Mar 17, 2021

wsanchez deleted the mypy-zope-plugin branch March 17, 2021 21:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable mypy-zope plugin #471

Enable mypy-zope plugin #471

wsanchez commented Mar 11, 2021 •

edited

wsanchez commented Mar 11, 2021 •

edited

wsanchez Mar 12, 2021

wsanchez Mar 12, 2021

wsanchez Mar 12, 2021

wsanchez Mar 12, 2021

adiroiban Mar 12, 2021

wsanchez Mar 12, 2021

wsanchez Mar 12, 2021

adiroiban left a comment

adiroiban Mar 12, 2021

wsanchez Mar 12, 2021

wsanchez Mar 12, 2021

wsanchez Mar 12, 2021

wsanchez commented Mar 12, 2021

glyph commented Mar 12, 2021

glyph commented Mar 12, 2021

glyph commented Mar 12, 2021 •

edited

glyph commented Mar 12, 2021

glyph commented Mar 12, 2021

wsanchez commented Mar 12, 2021

glyph commented Mar 13, 2021

codecov bot commented Mar 17, 2021 •

edited

Enable mypy-zope plugin #471

Enable mypy-zope plugin #471

Conversation

wsanchez commented Mar 11, 2021 • edited

wsanchez commented Mar 11, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adiroiban left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wsanchez commented Mar 12, 2021

glyph commented Mar 12, 2021

glyph commented Mar 12, 2021

glyph commented Mar 12, 2021 • edited

glyph commented Mar 12, 2021

glyph commented Mar 12, 2021

wsanchez commented Mar 12, 2021

glyph commented Mar 13, 2021

codecov bot commented Mar 17, 2021 • edited

Codecov Report

wsanchez commented Mar 11, 2021 •

edited

wsanchez commented Mar 11, 2021 •

edited

glyph commented Mar 12, 2021 •

edited

codecov bot commented Mar 17, 2021 •

edited