Add "no-new-attributes" behavior to our models

[pradyunsg]

We should add "no-new-attributes" behavior to all our models in pip._internal.models. It'd be easy to enforce this constraint on them using the interpreter than manually in code reviews. :)

Two of the ways to achieve this using just the standard library are:

• __slots__
• collections.namedtuple
• (both together?)

In case someone (eg. a future me) wants to look up how slots work, take a look at https://stackoverflow.com/a/28059785/1931274.

Originally posted by @pradyunsg in #7310

[chrahunt]

attrs can do slots and immutability, has no install dependencies to speak of, and supports Python 2.7+ and 3.4+. A cursory test also shows no problems vendoring it.

With attrs our sdist (tar.gz) is +26KB and wheel is +30KB, both are about 1.4MB.

[xmunoz]

The attrs module might be too dynamic for mypy. Here is my work on this so far: master...xmunoz:slots-attrs

Tests pass on the first two models that I converted, but mypy linting fails with nonsense.

src/pip/_internal/models/index.py:30: error: Too many arguments for "PackageIndex"
src/pip/_internal/models/index.py:30: error: Unexpected keyword argument "file_storage_domain" for "PackageIndex"
src/pip/_internal/models/index.py:33: error: Too many arguments for "PackageIndex"
src/pip/_internal/models/index.py:33: error: Unexpected keyword argument "file_storage_domain" for "PackageIndex"
src/pip/_internal/locations.py:188: error: Unexpected keyword argument "platlib" for "Scheme"
src/pip/_internal/locations.py:188: error: Unexpected keyword argument "purelib" for "Scheme"
src/pip/_internal/locations.py:188: error: Unexpected keyword argument "headers" for "Scheme"
src/pip/_internal/locations.py:188: error: Unexpected keyword argument "scripts" for "Scheme"
src/pip/_internal/locations.py:188: error: Unexpected keyword argument "data" for "Scheme"
Found 9 errors in 2 files (checked 112 source files)

A brief peruse at mypy's issues reveal incomplete support for attrs.

[chrahunt]

Hm, it looks like an attrs plugin was added in python/mypy#4397, which should be able to recognize this case. Maybe it is not recognizing our pip._vendor.attr as attr because it is vendored?

From here it looks like it will only match if the full name (I guess the fully qualified name) matches one of the items here. I don't see any official way to override it, maybe it would work if we just assign to it.

[pradyunsg]

It might be worth filing an issue on mypy's issue tracker, linking back to here, to see if they have any suggestions for how to handle this.

I'd thought that the from attr import * in a .pyi would help avoid this, but seems like that's not the case here.

fullname = 'pip._vendor.attr.s'

[pradyunsg]

I don't see any official way to override it, maybe it would work if we just assign to it.

If this is a suggestion to assign to qualname at runtime, that won't have any effect based on my judgement -- it's not looking at __qualname__ but rather computing the qualified name from the semantic analyser's state.

[pradyunsg]

FWIW, I think we don't need attrs here, since this is a fairly well-scoped usage as such.

The code is a bit-more verbose but looking at https://github.com/pypa/packaging/pull/179/files, I doubt it'd be a major concern -- additionally, we don't need the underscore-names used there, so I don't really think we'd need to adopt attrs just yet.

[xmunoz]

Should I re-open #7388? There is one platform-specific test failure on that PR (python35, macOS, test_should_not_install_always_from_cache), but maybe it's just a flake? Otherwise, that PR seems to solve this issue and pass the test suite.

[pradyunsg]

Yea, go ahead. I've not taken a look at the PR yet tbh -- I'm still not 100% sure that's the route we'd go but I'm pretty sure that the failure was intermittent.

[pradyunsg]

The attrs module might be too dynamic for mypy.
Maybe it is not recognizing our pip._vendor.attr as attr because it is vendored?

We could attach a mypy plugin that's a no-op, while patching the mypy attrs code during this phase.

diff --git a/setup.cfg b/setup.cfg
index f0bd7a8d..169638d4 100644
--- a/setup.cfg
+++ b/setup.cfg
@@ -27,6 +27,7 @@ select = E,W,F
 ignore = W504
 
 [mypy]
+plugins = tools/mypy/vendored.py
 follow_imports = silent
 ignore_missing_imports = True
 disallow_untyped_defs = True
diff --git a/tools/mypy/vendored.py b/tools/mypy/vendored.py
new file mode 100644
index 00000000..3176bc43
--- /dev/null
+++ b/tools/mypy/vendored.py
@@ -0,0 +1,20 @@
+from mypy.plugin import Plugin
+
+
+def patch_attrs_plugin(module):
+    def add_pip_vendored_prefix(target_set):
+        for val in frozenset(target_set):
+            target_set.add("pip._vendor." + val)
+
+
+    add_pip_vendored_prefix(module.attr_class_makers)
+    add_pip_vendored_prefix(module.attr_dataclass_makers)
+    add_pip_vendored_prefix(module.attr_attrib_makers)
+
+
+def plugin(version: str):
+    import mypy.plugins.attrs
+    patch_attrs_plugin(mypy.plugins.attrs)
+
+    # Provide a no-op plugin
+    return Plugin

Yes, I know I'm evil. 😈