Commit 3aeef46
committed
Fix how Diffable annotates expected repo attribute
The Diffable class's essential method, diff, requires self to have
a repo attribute, which is outside Diffable's own responsibility to
provide. This was already documented in the Diffable docstring, but
the technique used to prevent mypy from treating accesses to that
attribute as errors had several disadvantages:
- Annotating `self.repo` is ambiguous, since typically *names* are
annotated. Although mypy treated this afterwards, even in code
appearing outside the implementation of Diffable.diff, to mean
that Diffable instances have repo attributes, it is not clear how
other type checkers, or possibly future version of mypy, would or
should interpret this. It is also not obvious from reading the
code that it should have an effect on static analysis of code
outside the diff method body, but this can be verified by
temporarily placing this code (unindented) after, and outside,
the Diffable class:
from git.types import Has_Repo
def f(x: Has_Repo) -> None:
pass
f(Diffable())
This produces no new type errors from mypy, but if the annotated
conditional attribute rebinding statement is commented out, then
one of the type errors is that `f(Diffable())` passes an argument
of the wrong type to `f`.
- The annotation was on an attribute binding operation (assigning
to `self.repo`), which mypy accurately reported as an error
because the Diffable class had no `repo` instance attribute
indicated in the code elsewhere and is a slotted class whose
instances have no instance dictionaries to allow that actual
binding operation to succeed. In addition to being an incorrect
annotation, this had the effect of decreasing the number of
related mypy errors only to one, instead of the hoped-for zero.
- The rebinding of self.repo to itself was written in way that, if
the statement were executed, rebinding would actually occur when
the instance has a repo attribute. (See below for why this does
not actually happen.) But this depends on the presence of a repo
attribute to read from and then write to, so any change that
allows mypy to infer that it is okay should also be enough to
avoid the original mypy errors at issue in the first place.
- If the statement were executed, it would fail at runtime if the
instance had a repo attribute that were read-only. But there is
no reason a subclass or sibling class used to provide the repo
attribute should be unable to do this, such as by defining repo
as a property with only a getter.
- The condition that guarded the self.repo (re)binding operation
was unintentionally incorrect in a way that caused it to be
entirely unrelated to the git.typing.Has_Repo protocol it tried
to refer to. The check was `hasattr(self, "Has_Repo")`, but the
goal is not to check for an attribute named `Has_Repo`, but to
check for an attribute named `repo`.
With `Has_Repo` imported, the different condition
`isinstance(self, Has_Repo)` would do that (the protocol is
runtime-checkable). Or the condition `hasattr(self, "repo")`
could be used. Instead, it worked the same as under the change:
- if hasattr(self, "Has_Repo"):
+ if hasattr(self, "Any_Other_Nonexistent_Attribute"):
The way it avoided a runtime error (and provably so, to mypy) was
that it was always False, and mypy simply treated the annotation
as usable information on both branches.
This commit removes that code and instead adds a class-level
annotation for a `repo` instance attribute, with no assignments.
That eliminates the mypy error for the attempt to conditionally
annotate `self.repo`, while also keeping the self.repo accesses
from becoming static type errors again.
As before, mypy treats Diffable as a static subclass of Has_Repo,
but if an instance `x` of Diffable lacked a `repo` attribute (which
would lead to a runtime error, as before, in realistic use, when
`diff` were called on it), then `isinstance(x, Has_Repo)` will
evaluate to `False`. Because the repo member of Has_Repo is not
written as a method, it is a runtime error to use issubclass to
check if a class is a subclass of it (even though isinstance does
work), so adding this annotation to the Diffable class does not
affect that either.
Some behavior of Has_Repo may not be as expected, because of the
different semantics of (a) the static type system checked by mypy
and other static type checkers and (b) the actual dynamic type
system of Python enforced at runtime by the implementation.
Furthermore, this protocol is not used anywhere in GitPython, and
since this commit, there is no attempt to use or reference it.
Because it is public in the public git.types module, it should not
be immediately removed, but it may make sense to deprecate it.
However, this commit does not make that change.1 parent 4083dd8 commit 3aeef46
1 file changed
Lines changed: 5 additions & 5 deletions
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
@@ -84,13 +84,16 @@ class Diffable: | |||
| 84 | 84 | compatible type. | |
| 85 | 85 | ||
| 86 | 86 | :note: | |
| 87 | - Subclasses require a repo member as it is the case for | ||
| 88 | - :class:`~git.objects.base.Object` instances, for practical reasons we do not | ||
| 87 | + Subclasses require a repo member, as it is the case for | ||
| 88 | + :class:`~git.objects.base.Object` instances. For practical reasons we do not | ||
| 89 | 89 | derive from :class:`~git.objects.base.Object`. | |
| 90 | 90 | """ | |
| 91 | 91 | ||
| 92 | 92 | __slots__ = () | |
| 93 | 93 | ||
| 94 | + repo: "Repo" | ||
| 95 | + """Repository to operate on. Must be provided by subclass or sibling class.""" | ||
| 96 | + | ||
| 94 | 97 | class Index: | |
| 95 | 98 | """Stand-in indicating you want to diff against the index.""" | |
| 96 | 99 | ||
@@ -169,9 +172,6 @@ def diff( | |||
| 169 | 172 | if paths is not None and not isinstance(paths, (tuple, list)): | |
| 170 | 173 | paths = [paths] | |
| 171 | 174 | ||
| 172 | - if hasattr(self, "Has_Repo"): | ||
| 173 | - self.repo: "Repo" = self.repo | ||
| 174 | - | ||
| 175 | 175 | diff_cmd = self.repo.git.diff | |
| 176 | 176 | if other is self.Index: | |
| 177 | 177 | args.insert(0, "--cached") | |
0 commit comments