ruff icon indicating copy to clipboard operation
ruff copied to clipboard

Make flake8-type-checking's unsafe fixes not fix if a library is used in a doctest

Open mikeweltevrede opened this issue 1 year ago • 1 comments
trafficstars

Hi all, curious to hear your opinion on this :) Please let me know if this needs to be filed on flake8-type-checking instead!

Ruleset

flake8-type-checking (TCH) with unsafe fixes on.

Ruff call and settings

Ruff version

0.4.8

Command invoked

pre-commit hooks:

repos:
  - repo: local
    hooks:
      - id:       ruff
        name:     Run ruff linter
        entry:    ruff
        args:     [ "--no-cache" ]
        language: python
        types:    [ file, python ]
        files:    .*\.py$

Relevant settings in pyproject.toml:

[tool.ruff]
line-length = 120
target-version = "py39"
fix = true  # Allow for ruff linting to fix some issues. Note that we control this behaviour in [tool.ruff.lint].

[tool.ruff.lint]
extend-select = [
    "TCH",  # flake8-type-checking | Allow for imports only used for type hinting | https://docs.astral.sh/ruff/rules/#flake8-type-checking-tch
]
fixable = [
    "TCH001", "TCH002", "TCH003", "TCH004", "TCH005",   # flake8-type-checking
]
extend-safe-fixes = [
    "TCH001", "TCH002", "TCH003", "TCH004", "TCH005",   # flake8-type-checking
]

[tool.ruff.lint.flake8-type-checking]
quote-annotations = true
strict = true
exempt-modules = ["typing", "typing_extensions", "types"]

Explanation and Example

When allowing TCH to auto-fix, it will transform this...

from datetime import datetime

def my_func(dt: datetime):
    """Example function

    >>> my_func(dt=datetime(year=2024, month=7, day=2))
    'hello'
    """
    return "hello"


if __name__ == "__main__":
    import doctest
    doctest.testmod()

into ...

from typing import TYPE_CHECKING

if TYPE_CHECKING:
    from datetime import datetime

def my_func(dt: "datetime"):
    """Example function

    >>> my_func(dt=datetime(year=2024, month=7, day=2))
    'hello'
    """
    return "hello"


if __name__ == "__main__":
    import doctest
    doctest.testmod()

While this seems correct at first glance (no SyntaxError), notice that datetime is used in the doctest. It would be great if the ruff fixer can scan doctests as well for usage.

mikeweltevrede avatar Jul 02 '24 12:07 mikeweltevrede

It makes sense to me that Ruff shouldn't provide an autofix in this case because it breaks user code. Doing this would require that Ruff understands doctests and takes them into account when running the analysis. How this would work is an interesting question.

MichaReiser avatar Jul 03 '24 06:07 MichaReiser

@MichaReiser Indeed, it is relatively strange behaviour and therefore it makes sense that it is an unsafe fix.

Doctests are well-defined in Python, with >>> being included at the start of a new line in the docstring. The doctest library (or pytest with the flag --doctest-modules) will then recognize these and run them as tests.

It is interesting to think about whether there are use cases where people might use >>> at the start of a new line where it is not intended to be a doctest. Does this happen? Are these valid use cases? Is this just ignorance (I learned about doctests like this a while back but before this thought they were just examples, not meant to be runnable code per se)? Should Ruff provide an opinion here? Either way, this should remain an unsafe fix, keeping these points in mind.

I do not know how to program in Rust at the moment but if this would be implemented, I am happy to assist the person working on this and think along!

mikeweltevrede avatar Jul 09 '24 06:07 mikeweltevrede