Only typecheck immediate sources, not transitive

[josh-newman]

Hi @thundergolfer,

I found this repository after reading python/mypy#3912 (comment) and I'm also interested in typechecking Bazel-built Python code. I think I've run into a similar issue as @dmadisetti (with generated protobuf code), but I have a question and/or a different approach than #11.

If I understand Bazel aspects correctly (I'm not an expert), if I have py_library targets //py:b which depends on //py:a, and I build //py:b, Bazel will invoke the mypy aspect for both, so it's not necessary to check :a's sources while checking :bs. This PR modifies the mypy aspect to do that. I tested in an internal codebase and found that typecheck errors were still caught in transitive dependencies with this change.

This PR is incomplete because it breaks mypy_test, but if this approach seems reasonable to you, I'll go ahead and fix it so it's mergeable.

Thanks.

[thundergolfer]

if I have py_library targets //py:b which depends on //py:a, and I build //py:b, Bazel will invoke the mypy aspect for both

Yes the .bzl code currently has attr_aspects = ['deps'], so the Aspect is propagating through the deps.

so it's not necessary to check :a's sources while checking :bs

I'm not sure this is true, but I'm not an expert on MyPy's behaviour or underlying workings. The docs say this:

$ mypy file_1.py foo/file_2.py file_3.pyi some/directory
The above command tells mypy it should type check all of the provided files together.

It seems like you might want a target's srcs and the srcs of its transitive deps checked 'together', as one static type context, but this might be unnecessary as you suggest.

Dropbox's more advanced and complicated MyPy Aspect appears to pass all transitive srcs though. They also propagate along deps.

[thundergolfer]

Doing this looks right. My single --package-root looks hacky and wrong, but it wasn't breaking things from what I can see. Did you encounter problems with --package-root .?

Why not also transitive srcs as --package-roots too?

[josh-newman]

Thanks for reviewing.

I encountered the package root issue trying to depend on the Google protobuf python target using @com_google_protobuf//:protobuf_python (similar to #11). However, after fixing the source roots, I found another issue: that target includes __init__.py's under compatibility_test/v2.5.0, which is not a valid Python module name, and since the mypy invocation includes all transitive sources, mypy fails on the bad module name. I ended up switching to depending on PyPI's protobuf, since that's packaged correctly (I think @com_google_protobuf//:protobuf_python is broken, in this case).

Now, after switching to protobuf from PyPI, I have a new issue: MYPYPATH isn't populated with imports from transitive dependencies. This causes mypy to emit tons of type errors for the protobuf library because mypy sees a different import path for the external dependency (something like external.pypi__...__protobuf.google.protobuf), so it fails to match it with typeshed's definitions. I haven't figured out how to fix this yet—let me know if you see an obvious solution.

[dmadisetti]

Sounds like more of a mypy issue? The default mypi.ini is rather aggressive, if you turn off imports using

[mypy]
follow_imports=skip

You might see a solution to your problem. A less shotgun approach is to use # type: ignore. I just took a look at my code, which works with #11 and I have:

from google.protobuf import text_format  # type: ignore

In my mypy_test files.

Sorry I haven't taken the time to review this. I've been just using the tests not the aspect, so unless you make those changes- I don't have much to input.

But I'd be excited for a canonical solution that supports protobuf 🎉

[thundergolfer]

Would this also work as an alternative to follow_imports=skip?

[mypy-protobuf.*]
ignore_missing_imports = True

[thundergolfer]

@josh-newman @dmadisetti

I've created https://github.com/thundergolfer/bazel-python-mypy-protobuf as a repro of #15.

Does it also work as a repro of the problems you're experiencing with generated code?

[dmadisetti]

@thundergolfer Awesome! I was planning to do something similar. I'll apply my patch and see if it works.

This PR has some nice changes I might try to incorporate as well. However, I may not get around to it until the weekend.