René's URL Explorer Experiment

{"@context":"https://schema.org","@type":"DiscussionForumPosting","headline":"Diffable.diff is misleadingly annotated regarding special `other` values","articleBody":"The `other` parameter of `Diffable.diff` is annotated this way:\r\n\r\nhttps://github.com/gitpython-developers/GitPython/blob/e880c33284f2abff55d3cddfef2302663b178005/git/diff.py#L110\r\n\r\n`Diffable.diff` accepts a few special values:\r\n\r\n- `None` for a working tree diff.\r\n- `Diffable.Index`, a `type` object that is meant to be used opaquely rather than instantiated, to compare to the index.\r\n- `NULL_TREE`, a direct instance of `object` (not to be confused with `Object`), for a diff against an empty tree.\r\n\r\n#### The type of `NULL_TREE` should be expressed better than `object`\r\n\r\nThe main problem here, which exists in the documentary effect of the annotations even if a type checker is never used, is the presence of `object` in the union. Since `object` is the root of the type hierarchy in Python, a union of anything with `object` is equivalent to just `object`. But the intent of the annotation is not to express that it is correct to pass arbitrary objects as `other`. Instead, the idea is that it is okay to pass `NULL_TREE`.\r\n\r\nThis is also the cause of the incompatible override type error on the `IndexFile.diff` override in `git.index.base`, which does not include the `object` alternative:\r\n\r\nhttps://github.com/gitpython-developers/GitPython/blob/e880c33284f2abff55d3cddfef2302663b178005/git/index/base.py#L1482\r\n\r\nThere are several places in GitPython's source code where an overridden method has an incompatible signature, violating substitutability. The reason this place is of interest is that it is hard to tell efficiently by examining the code or type errors *what* conceptually this means. It turns out it means the override does not support `NULL_TREE`:\r\n\r\n```text\r\n\u003e\u003e\u003e import git\r\n\u003e\u003e\u003e repo = git.Repo()\r\n\u003e\u003e\u003e repo.index.diff(git.NULL_TREE)\r\nTraceback (most recent call last):\r\n  File \"\u003cstdin\u003e\", line 1, in \u003cmodule\u003e\r\n  File \"C:\\Users\\ek\\source\\repos\\GitPython\\git\\index\\base.py\", line 1521, in diff\r\n    raise ValueError(\"other must be None, Diffable.Index, a Tree or Commit, was %r\" % other)\r\nValueError: other must be None, Diffable.Index, a Tree or Commit, was \u003cobject object at 0x00000265862B8A30\u003e\r\n```\r\n\r\nBut `object` does not express (to humans or tools) that what is really meant is the literal value `NULL_TREE`.\r\n\r\nWe cannot currently write `Literal[NULL_TREE]`, but if `NULL_TREE` is made into an enumeration constant, then this can be done, though cumbersomely it seems it can only be written--in annotations--as `ET.NULL_TREE` where `ET` is the declared enumeration type. Or, if `ET` has `NULL_TREE` as its only value, then writing `ET` should be sufficient, though this may be less intuitive due to not involving `Literal` in any way while expressing a literal.\r\n\r\nTo be clear, this will not cause the mypy error about an incompatible override to go away. But it will make the error make sense, as well as making the *suppression* comment on it make sense in terms of what it is expressing. *And it will allow type checkers to catch errors about incorrect values of `other`.*\r\n\r\n#### `Diffable.Index` should ideally also be improved\r\n\r\n`Diffable.Index` is used as an opaque constant, as is `NULL_TREE`. But `Diffable.Index` is a class. This is confusing because it should not be instantiated. Its static type can be expressed pretty specifically, as `Type[Diffable.Index]` (as is done; I'm omitting the quotes here for simplicity). However, this does not allow `if`-`else` logic to be type-checked exhaustively, because there is no guarantee that the `Diffable.Index` class is the only value of the static type `Type[Diffable.Index]`:\r\n\r\n```python\r\nclass Derived(Diffable.Index):\r\n    pass\r\n```\r\n\r\nHopefully no one is doing that--just as hopefully no one is instantiating it--but `mypy` and other tools, including editors, cannot tell that `Diffable.Index` is the only value of `Type[Diffable.Index]`. Humans who notice that it is intended to be used as an opaque constant *will* recognize this, however. Also, it *should* be possible to get type checkers to recognize it as the only value, and type-check `if`-`else` logic exhaustively, by decorating the definition of `Index` with `@final`. This works for some type checkers, at least `pyright` and `pylance`. But it does not work for `mypy`.\r\n\r\nThe bigger issue is that it is easy to confuse and, for example, instantiate it.\r\n\r\n`Diffable.Index` can also be made an enumeration constant, and all reasonable usage will continue to work. *At runtime.* However, if someone wants to statically annotate their own method to accept or return it, then they would need to change the annotations, since expressing it with `Type[Diffable.Index]` would no longer be able to work. (Two separate special objects cannot both be used, because existing subclasses of `Diffable` outside of GitPython would only be covering one. But if it is redefined, existing code can continue to cover it at runtime, since it should only ever have been, or be, used as an opaque constant.)\r\n\r\nIt is also possible for unreasonable usage to break at runtime, such as attempting to check if `x` is `Diffable.Index` by writing `issubclass(x, Diffable.Index)`. I am less worried about this.\r\n\r\nIt seems to me that this change is worth making for `Diffable.Index` as well, but this is a judgment call, and the new interface should perhaps be given special attention in review. When I searched for existing code outside GitPython, its forks, vendored copies of it, etc., that used these annotations in a way where they would cause a new static type error, I didn't find anything, but I only used GitHub code search, and I may not have found everything even on GitHub. My argument for making this change is to achieve greater correctness without breaking anything at runtime, and *not* that I am at all confident nobody will have to change their code to keep it passing static checks.\r\n\r\n#### Connection to #1859\r\n\r\nI've included fixes for both of these things in #1859. See [65863a2](https://github.com/gitpython-developers/GitPython/pull/1859/commits/65863a28c2bfebe084e1c86d409fbf68b0b33a76) especially, whose docstring has some more information about the specific design choices there.\r\n\r\nIf you decide you don't want the change to `Diffable.Index` there, that can be undone without sacrificing the rest. Although I am in favor of this aspect of the change, it should not be accepted on the basis of the sunk-cost fallacy. Furthermore, the sunk cost would be quite low, because it was already helpful in enabling me to figure out what needed to be done in other related parts of the code.\r\n\r\nThe purpose of this issue is twofold:\r\n\r\n- To allow the bug to be tracked if [#1859](https://github.com/gitpython-developers/GitPython/pull/1859) is changed in such a way as not to fully solve it.\r\n- To described it separately from that PR so it's easier to understand. I considered describing it in comments there, but thought opening an issue would be better.","author":{"url":"https://github.com/EliahKagan","@type":"Person","name":"EliahKagan"},"datePublished":"2024-03-14T08:06:12.000Z","interactionStatistic":{"@type":"InteractionCounter","interactionType":"https://schema.org/CommentAction","userInteractionCount":3},"url":"https://github.com/1874/GitPython/issues/1874"}