Not all new mypy type checking errors are shown by Darker

[PatrickJordanCongenica]

I've wanted to move away from pre-commit for a while and use the various Python formatters and linters directly, but didn't like how pylint takes to run as it scans your entire code base rather than just the files that have changed. Then I strolled across your execellent work here, so just wanted to thank you all for providing this library!

I've got darker up and running in my poetry managed code base using the following configuration:

[tool.poetry.dev-dependencies]
black = "^22.1.0"
isort = "^5.10.1"
mypy = "^0.931"
pylint = "^2.12.2"
taskipy = "^1.10.1"

[tool.taskipy.tasks]
black = "black ."
isort = "isort ."
mypy = "mypy ."
pylint = "pylint ./framework ./tests"
all_code = "task isort && task black && task mypy && task pylint"
changed_code = "darker ."

[tool.black]
line-length = 120

[tool.darker]
src = [
    "framework",
    "tests"
]
isort = true
lint = [
    "pylint",
    "mypy"
]

[tool.isort]
atomic = true
include_trailing_comma = true
line_length = 120
multi_line_output = 3

[tool.mypy]
no_strict_optional = true
ignore_missing_imports = true

[tool.pylint.format]
max-line-length = 120

[tool.pylint.messages_control]
disable = [
    "design",
    "duplicate-code",
    "missing-docstring",
    "invalid-name"
]

When I run poetry run task changed_code, I can see black, isort and pylint run (by manually making changes to a file that would trigger each of these tools) but mypy doesn't seem to run.

As you can see from above, I'm making use of taskipy to abbreviate some commands and when I run poetry run task mypy, I get feedback that an intentional change I made violates some of the checks mypy does:

framework/common/api_endpoint.py:157: error: Missing positional argument "temp" in call to "get" of "ApiClient"
framework/common/api_endpoint.py:158: error: Argument "url" to "get" of "ApiClient" has incompatible type "str"; expected "int"

However, running poetry run task changed_code doesn't output the same feedback, even with specifying mypy in the [tool.darker] pyproject.toml configuration.

Am I doing something wrong here?

[akaihola]

Hi @PatrickJordanCongenica, thanks for your report!

The [tool.darker] part of your pyproject.toml does look correct.

You could try to diagnose by doing the following:

• remove the [tool.darker] section temporarily
• make a deliberate type error in one of your source code files
• make sure the change is shown by git diff
• make sure mypy catches the error by running it manually: mypy .
• see if mypy catches it when running manually via Darker: darker --check --lint mypy .
• if it doesn't, try adding -v or -vv on the darker command line and share the output here
• if it does, try adding the [tool.darker] / lint = ["pylint"] configuration in pyproject.toml and run darker --check .
• if that doesn't work, let's sort the pyproject.toml problem out together
• if it does work, try to find out what is different when you run poetry run task changed_code:
  • maybe it calls darker differently?
  • maybe it does something with Git before calling darker?

I hope you get some insight by doing these steps, and please do paste any interesting findings here so we can look at it together!

[akaihola]

(btw the --check argument in my examples is there so Darker won't reformat your files or show reformatting diff output – reformatting is not what we're interested here)

[PatrickJordanCongenica]

Thanks for the reply @akaihola. Please find below the output of what you recommended.

• remove the [tool.darker] section temporarily ✔️
• make a deliberate type error in one of your source code files ✔️ (changed the input type of a param from str to int)
• make sure the change is shown by git diff ✔️

diff --git a/framework/common/api_client.py b/framework/common/api_client.py
index e29e5b0..0e96383 100644
--- a/framework/common/api_client.py
+++ b/framework/common/api_client.py
@@ -59,7 +59,7 @@ class ApiClient:
 
     def get(
         self,
-        url: str,
+        url: int,
         expected_response_type: ResponseType,
         json_schema: Optional[Dict[str, Any]] = None,
         headers: Dict[str, Any] = None,

• make sure mypy catches the error by running it manually: mypy . ✔️

➜ poetry run mypy --no-strict-optional --ignore-missing-imports .

framework/common/api_endpoint.py:158: error: Argument "url" to "get" of "ApiClient" has incompatible type "str"; expected "int"
tests/saas_dev/qa_build/functional/patient/variants/gene/transcript/snv/test_summary.py:139: error: Argument 1 to "get" of "ApiClient" has incompatible type "str"; expected "int"
Found 2 errors in 2 files (checked 232 source files)

• see if mypy catches it when running manually via Darker: darker --check --lint mypy . ❌

➜ poetry run darker --check --lint mypy .
<nothing is output when running this>

• if it doesn't, try adding -v or -vv on the darker command line and share the output here ✔️

➜ poetry run darker -vv --check --lint mypy .

# Effective configuration:

[tool.darker]
src = [
    ".",
]
revision = "HEAD"
diff = false
stdout = false
check = true
isort = false
lint = [
    "mypy",
]
log_level = "DEBUG"


# Configuration options which differ from defaults:

[tool.darker]
src = [
    ".",
]
check = true
lint = [
    "mypy",
]
log_level = "DEBUG"



DEBUG:darker.git:[/home/patrickjordan/code/api-testing]$ git rev-parse --is-inside-work-tree
DEBUG:darker.git:[/home/patrickjordan/code/api-testing]$ git diff --name-only --relative HEAD -- <massive list of files that I've omitted as it contains business sensitive information>
DEBUG:darker.__main__:Read 157 lines from edited file /home/patrickjordan/code/api-testing/framework/common/api_client.py
DEBUG:darker.__main__:Black reformat resulted in 157 lines
DEBUG:darker.diff:Diff between edited and reformatted has 1 opcode
DEBUG:darker.git:[/home/patrickjordan/code/api-testing]$ git show HEAD:./framework/common/api_client.py
DEBUG:darker.git:[/home/patrickjordan/code/api-testing]$ git log -1 --format=%ct HEAD -- framework/common/api_client.py
DEBUG:darker.diff:Diff between edited and reformatted has 3 opcodes
DEBUG:darker.chooser:Found edits on lines 1-157
DEBUG:darker.chooser:Using 157 unmodified lines at line 1
DEBUG:darker.__main__:Verifying that the 157 original edited lines and 157 reformatted lines parse into an identical abstract syntax tree
DEBUG:darker.linting:Unparseable linter output: Success: no issues found in 1 source file

Before I continue with the rest of the steps from your answer, is it worth pausing here for you to have a look at the output above? The final line does hint at a possible cause: DEBUG:darker.linting:Unparseable linter output: Success: no issues found in 1 source file?

[akaihola]

If you look closely at the output from calling mypy manually, your change caused a type checking failure in a different file!

Try changing an argument to a wrong type e.g. in a function call instead.

[akaihola]

To clarify:

• Unparseable linter output: is expected. Darker only attempts to parse linter output lines which are prefixed with a file path and a line number (and possibly a column number). All other lines trigger this DEBUG level log output but don't prevent Darker from working in any way.
• Success: no issues found in 1 source file indicates that Darker runs mypy separately for each modified file, in this case ./framework/common/api_client.py. As the change from url: str to url: int isn't in contradiction with any other static type hints in that file, the check of the file is successful.
• The change from url: str to url: int caused the call to ApiClient.get() in tests/saas_dev/qa_build/functional/patient/variants/gene/transcript/snv/test_summary.py, line 139 to become invalid since it passes a str for the url argument. But since that file wasn't modified, Darker doesn't even run mypy for it.
• So I believe mypy does run in your case for each modified file, but it just wasn't run for the unmodified file which your change rendered invalid.

I'll post further thoughts on this in a separate answer below.

[PatrickJordanCongenica]

Thanks for the explanation on what I'm seeing. I can confirm that mypy does run on the modified files as expected (by following your suggestion to change something inside a function rather than an input parameter type).

[akaihola]

This is a really good problem report, since it clarified my thoughts about Darker's applicability to filtering mypy output.

Darker only reports type violations on modified lines, but as we see, changes in code will often trigger type violations in other, unmodified parts of the file, or, as in this case, in completely different files which weren't modified at all.

To find new typing errors introduced by changes to a repository between revisions rev1 and rev2, actually a much better way to filter mypy output would be to

1. run mypy for the whole code base at rev1 (i.e. the old revision before the modifications),
   and record the file path, line number, line content and message for each typing error
2. run mypy again for the whole code base at rev2 (i.e. the modified revision or the working tree),
   and record the file path, line number, line content and message for each typing error
3. compare the recorded typing errors, and for each file at rev2, output only messages which didn't exist for exactly the same line content in rev1

I'm not sure if the line number information for typing errors is useful at all – maybe a more exact matching could be done with help from the diff and possibly assuming that the errors come in the same order. But as the order actually will change if functions or classes are reordered inside a file, maybe that's not a good idea, and matching indeed should be based only on line content.

This approach doesn't care at all about which files and lines were modified, so this isn't actually a good fit for Darker if we consider what it attempts to do. So perhaps running mypy with Darker shouldn't be recommended at all, and a different tool should be developed for that?

On the other hand, I have successfully used Darker and mypy for gradually introducing type hinting for projects which didn't have any hints to begin with. I've configured mypy to be as strict as possible, and let Darker remind me to introduce typing on lines I've modified. But regarding what was observed above, obviously this setup will have hidden from me some errors I might have caused for unmodified lines when adding wrong type hints.

[PatrickJordanCongenica]

Thanks for your time and thoughts on what is causing this @akaihola. I can see how darker is omitting files that haven't changed based on your explanation. I'm not sure what is the best solution here as I'm unaware of how darker works but given a change in one file can cause failures in unchanged files, you can end up in a bit of a catch 22 situation.

What would be the next steps? Is it worth discussing this further with others maintaining this library to see if there's a clear path to solving this issue?

[akaihola]

I'm the only maintainer currently, with a few contributors chiming in with code reviews every now and then, so realistically I'd expect the discussion to be just between you and me :)

---

On one hand I can see how it would be useful to be able to run all the linters in one go through Darker, and be able to trust that Darker does all the right dances to obtain results which make sense.

On the other hand, doing a checkout for rev1 and running mypy also there goes a bit far from what Darker currently does with all other reformatters and linters. It would be cleaner to have a completely separate tool for this.

But let's brainstorm a bit about what the options are for how to run mypy on the rev1 revision. First off, mypy must have the whole source tree available so it can check typing across modules, so we can't just feed it one historical file at a time.
I can think of:

• doing a local clone of the local repository in another directory
• forcefully checking out rev1 in the current repository clone, and somehow ensuring the original state (with possibly a mixture of staged and unstaged changes) is restored afterwards
• some other ingenious solution...?

I would also like to think about whether this solution would be better than, worse than or equivalent to the current way for other linters than mypy – so pylint and flake8.

[PatrickJordanCongenica]

That seems like a lot of work to get mypy working with darker. I'm approach this problem without any prior knowledge to the design principles of this library, so if you feel what you're suggesting falls within those principles, then it seems like a local clone might work. Obviously devil is in the detail.

For now, I can work around this by not asking darker to use mypy and simply update my changed_code task to be darker . && task mypy (as a full mypy scan of my repo isn't horrifically slow). The real MVP of darker is all the other formatters and linters. Generally, mypy runs quickly, even against large code bases, so maybe another solution would be to intentionally not support mypy for the issue I've encountered with darker and recommend that this is run against the entire code base?

[akaihola]

Actually there's no mypy specific code in Darker – mypy just happens to produce output which conforms to the same format as we see from pylint and flake8. So any command you pass to the --lint <command> option will be run by Darker and assumed to produce <path>:<linenum>: <message> or <path>:<linenum>:<column>: <message> formatted output.

So I guess we could recommend against using --lint mypy in README.rst, and possibly additionally drop other mentions of mypy.

I guess for some purposes Darker's way of filtering mypy output might be good enough, so maybe it's enough to warn about the current shortcoming?

mypy is indeed satisfyingly fast, at least when it has a previously populated .mypy_cache/ available.

[akaihola]

It seems basedmypy people have also been discussing the topic of changes on one line causing mypy failures on another (possibly in another file). They also pointed out that the cause might actually be e.g. in a configuration file (mypy.ini for instance).

See their discussion here.

[akaihola]

I just ran into flakehell which is also based on the idea of comparing two linter runs on different revisions of the code base. They call this approach "legacy first" or "baseline":

1. The first step is to create a baseline via:

flakehell baseline > .flakehell_baseline

It will contain all your current violations list with exact locations and quantity.

2. Then specify the baseline in the configuration:

# Inside your pyproject.toml
[tool.flakehell]
baseline = ".flakehell_baseline"

3. Run your linter again with flakehell lint. You will see no violations!

4. Try to add a new one into your source code. And run your linter again. It will be reported!

I tried it:

$ flakehell baseline
9da31f51c40118197ba315c9b1fcee9a
7e83108226550b5edefc5745d6d3b322
99f0dd7bf2ae35168b544256692518af

So it seems the baseline is a collection of hashes for check failures.

Also note that you need to pip install flakehell flake8==3.9.0 – newer flake8 versions break flakehell.

[akaihola]

Yes, that's my understanding – except that flakehell is a wrapper just for flake8, so no mypy support there.

I spent a while considering whether a manually generated baseline could be a good approach in Darker. That would nicely avoid having to clone a historical revision in another directory and to do an extra mypy run there.

But the flipside is that you could no longer force-merge a branch with known mypy warnings and know you won't be notified about them in the future – instead you'd have to either silence those warnings with # type: ignore directives or mypy configurations, or re-generate the baseline after merging.

Also, when mypy is updated, it will often introduce new checks and remove old false positives, but it could also change warning messages to fix typos or improve wording. All that will cause the baseline to no longer match current mypy output. So running the up-to-date mypy to create a baseline on the fly for rev1 and then run it again in rev2 would be guaranteed to give more consistent results.

We could of course support both the baseline approach and running mypy in both rev1 and rev2. In that case I'd start with the baseline approach since it's much simpler.

What do you think?

[akaihola]

I also found basedmypy which is a fork of mypy with baseline support:

$ mypy --write-baseline .
src/darker/help.py:14: error: No error code on "type: ignore" comment  [no-error-code]
release_tools/bump_version.py:315: error: TypedDict key must be a string literal; expected one of ("any_version", "old_version", "new_version", "any_milestone")  [misc]
Found 2 errors in 2 files (checked 50 source files)
Baseline successfully written to .mypy/baseline.json

$ cat .mypy/baseline.json
{
  "src/darker/help.py": [
    {
      "line": 14,
      "code": "no-error-code",
      "message": "No error code on \"type: ignore\" comment"
    }
  ],
  "release_tools/bump_version.py": [
    {
      "line": 315,
      "code": "misc",
      "message": "TypedDict key must be a string literal; expected one of (\"any_version\", \"old_version\", \"new_version\", \"any_milestone\")"
    }
  ]
}%

[akaihola]

I guess we could recreate basedmypy behavior in Darker by adding a command to convert parsed mypy output to this baseline format. darker --lint mypy could take this saved baseline JSON into account when filtering mypy output for rev2. This way we could even be compatible with basedmypy!

Although I think it's essential to have the content of the source code line as well.

[akaihola]

...and @VCTLabs is adding baseline support to the GitHub Action for the Bandit security linter.

So plenty of prior art on linter baselines in Python land!

[akaihola]

It seems that baseline support in basedmypy is rather primitive – it uses a heuristic of scanning for the same type checking error within ±50 lines, which can of course lead to false positives/negatives. We should make use of information from git diff gives about moved code, as Darker already does when filtering modifications from Black (and soon Flynt).

I suspect the same applies to flakehell baseline since the baseline file is just a list of hashes, and it doesn't use a diff to track code moves.

So in the end we'd probably be best off doing an implementation from scratch and using Darker's Git capabilities for a better baseline comparison. And instead of recording a one-time baseline, I think it's better to just run mypy twice – first for rev1 and then rev2 in --revision=<rev1>...<rev2>, and treat the first run as the baseline.

[akaihola]

• These issues will be documented in Point out shortcomings in --lint mypy #380.
• A better algorithm is being implemented. See issue Track distant side effects of changes by doing an extra baseline run for linters #383 and pull request Run linters twice: 1) baseline, 2) current; then show new messages #393.