Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Only typecheck immediate sources, not transitive #12

Closed
wants to merge 1 commit into from
Closed

Only typecheck immediate sources, not transitive #12

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
43 changes: 23 additions & 20 deletions mypy.bzl
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
load("@bazel_skylib//lib:shell.bzl", "shell")
load("@bazel_skylib//lib:sets.bzl", "sets")
load("//:rules.bzl", "MyPyStubsInfo")

# Switch to True only during debugging and development.
Expand Down Expand Up @@ -33,12 +34,6 @@ def _sources_to_cache_map_triples(srcs):
])
return triples_as_flat_list

def _is_external_dep(dep):
return dep.label.workspace_root.startswith("external/")

def _is_external_src(src_file):
return src_file.path.startswith("external/")

def _extract_srcs(srcs):
direct_src_files = []
for src in srcs:
Expand All @@ -50,7 +45,7 @@ def _extract_srcs(srcs):
def _extract_transitive_deps(deps):
transitive_deps = []
for dep in deps:
if MyPyStubsInfo not in dep and PyInfo in dep and not _is_external_dep(dep):
if MyPyStubsInfo not in dep and PyInfo in dep:
transitive_deps.append(dep[PyInfo].transitive_sources)
return transitive_deps

Expand Down Expand Up @@ -91,8 +86,12 @@ def _mypy_rule_impl(ctx, is_aspect = False, exe = None, out_path = None):
transitive_srcs_depsets = []
stub_files = []

if hasattr(base_rule.attr, "srcs"):
direct_src_files = _extract_srcs(base_rule.attr.srcs)
if not hasattr(base_rule.attr, "srcs"):
return None
direct_src_files = _extract_srcs(base_rule.attr.srcs)
direct_src_root_paths = sets.to_list(
Copy link
Collaborator

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

@josh-newman josh-newman Apr 9, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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 🎉

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

[mypy-protobuf.*]
ignore_missing_imports = True

sets.make([f.root.path for f in direct_src_files]),
)

if hasattr(base_rule.attr, "deps"):
transitive_srcs_depsets = _extract_transitive_deps(base_rule.attr.deps)
Expand All @@ -101,12 +100,10 @@ def _mypy_rule_impl(ctx, is_aspect = False, exe = None, out_path = None):
if hasattr(base_rule.attr, "imports"):
mypypath_parts = _extract_imports(base_rule.attr.imports, ctx.label)

final_srcs_depset = depset(transitive = transitive_srcs_depsets +
[depset(direct = direct_src_files)])
src_files = [f for f in final_srcs_depset.to_list() if not _is_external_src(f)]
if not src_files:
return None

transitive_src_files = depset(
direct = direct_src_files,
transitive = transitive_srcs_depsets,
).to_list()
mypypath_parts += [src_f.dirname for src_f in stub_files]
mypypath = ":".join(mypypath_parts)

Expand All @@ -124,20 +121,26 @@ def _mypy_rule_impl(ctx, is_aspect = False, exe = None, out_path = None):
# Compose a list of the files needed for use. Note that aspect rules can use
# the project version of mypy however, other rules should fall back on their
# relative runfiles.
runfiles = ctx.runfiles(files = src_files + stub_files + [mypy_config_file])
runfiles = ctx.runfiles(files = transitive_src_files + stub_files + [mypy_config_file])
if not is_aspect:
runfiles = runfiles.merge(ctx.attr._mypy_cli.default_runfiles)
runfiles = runfiles.merge(ctx.attr.mypy.default_runfiles)

ctx.actions.expand_template(
template = ctx.file._template,
output = exe,
substitutions = {
"{MYPY_EXE}": ctx.executable._mypy_cli.path,
"{MYPY_ROOT}": ctx.executable._mypy_cli.root.path,
"{CACHE_MAP_TRIPLES}": " ".join(_sources_to_cache_map_triples(src_files)),
"{CACHE_MAP_TRIPLES}": " ".join(
_sources_to_cache_map_triples(transitive_src_files),
),
"{SRC_ROOTS}": " ".join([
"--package-root " + shell.quote(path or ".")
for path in direct_src_root_paths
]),
"{SRCS}": " ".join([
shell.quote(f.path)
for f in src_files
shell.quote(f.path) for
f in direct_src_files
]),
"{VERBOSE_OPT}": "--verbose" if DEBUG else "",
"{VERBOSE_BASH}": "set -x" if DEBUG else "",
Expand Down
2 changes: 1 addition & 1 deletion templates/mypy.sh.tpl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ main() {
fi

set +o errexit
output=$($mypy {VERBOSE_OPT} --bazel --package-root . --config-file {MYPY_INI_PATH} --cache-map {CACHE_MAP_TRIPLES} -- {SRCS} 2>&1)
output=$({MYPY_EXE} {VERBOSE_OPT} --bazel {SRC_ROOTS} --config-file {MYPY_INI_PATH} --cache-map {CACHE_MAP_TRIPLES} -- {SRCS} 2>&1)
status=$?
set -o errexit

Expand Down