Skip to content

Conversation

cdce8p
Copy link
Contributor

@cdce8p cdce8p commented Mar 9, 2023

Change Summary

Mypy 1.1.1 added support for @dataclass_transform. With the plugin active, the mypy dataclass transformer would be executed twice. As the first iteration already removes the InitVar annotation, the second one results in an error message.

The PR targets the 1.10.X-fixes branch as that is much easier to test. If excepted, I'll also open a PR for main.

Fixes: #5161

Checklist

  • Unit tests for the changes exist
  • Tests pass on CI and coverage remains at 100%
  • Documentation reflects the changes where applicable
  • changes/<pull request or issue id>-<github username>.md file added describing change
    (see changes/README.md for details)
  • My PR is ready to review, please add a comment including the phrase "please review" to assign reviewers

@dmontagu
Copy link
Contributor

dmontagu commented Mar 9, 2023

Would it be possible to add a test demonstrating the changed behavior? I recognize that we aren't actually running tests against 1.1.1 but I would still like to add some code that would be affected if we were (if only for future's sake). If such code already exists, I would appreciate if you could point me at it.

If you aren't comfortable with the mypy plugin testing infrastructure, if you can just show me how to reproduce the issue that this resolves, that might be enough for me to add it to the tests.

@cdce8p
Copy link
Contributor Author

cdce8p commented Mar 9, 2023

Would it be possible to add a test demonstrating the changed behavior? I recognize that we aren't actually running tests against 1.1.1 but I would still like to add some code that would be affected if we were (if only for future's sake).

I guess we could add the InitVar example from #5161. With this PR it should work without errors.

from dataclasses import InitVar
from pydantic.dataclasses import dataclass

@dataclass
class MyDataClass:
    foo: InitVar[str]
    bar: str

MyDataClass(foo="foo", bar="bar")

If you aren't comfortable with the mypy plugin testing infrastructure, if you can just show me how to reproduce the issue that this resolves, that might be enough for me to add it to the tests.

Tbh I've ignored them so far as they take quite long to run. Additionally my laptop isn't that powerful. So I've only tested short examples to iterate on the correct fix. If you know a good place to add it, please feel free to do so.

@dmontagu
Copy link
Contributor

dmontagu commented Mar 9, 2023

There's an issue with the added tuple[...] annotation due to the fact that this module doesn't have the __future__ import. I'll fix that and push a commit after I merge the generics PR. (Killing the CI run now since it's going to fail)

@dmontagu
Copy link
Contributor

dmontagu commented Mar 9, 2023

Just FYI -- I'm getting test failures on this branch with mypy 0.971. I'm thinking that needs to be resolved before merging.

Specifically:

FAILED tests/mypy/test_mypy.py::test_mypy_results[mypy-default.ini-plugin_success.py-plugin_success.txt] - AssertionError: 122: error: Unexpected keyword argument "name" for "AddProject"  [call-arg]

Maybe this is just something weird with my local environment considering I don't see any reason this should be happening looking at your code. I fired off a CI run with the commit I pushed so we'll see if it all just passes...

@dmontagu
Copy link
Contributor

dmontagu commented Mar 9, 2023

Seems to be some kind of real issue, unless I did something stupid without realizing it.. I'll look at this a bit later today if you don't get to it first @cdce8p

@cdce8p
Copy link
Contributor Author

cdce8p commented Mar 9, 2023

Pushed some more changes. Should be ready now

  • Replaced the PydanticPlugin.mypy_version class variable with the existing MYPY_VERSION_TUPLE.
  • Renamed PLUGIN_VERSION to __version__. After looking at the mypy code for plugins a bit more, I saw that mypy actually uses multiple different things to determine if a plugin has changed
    • report_config_data for plugin config data
    • A hash of the plugin module file contents
    • The __version__ attribute defined by the plugin module itself.
      => I.e. mypy should notice a plugin change already even without PLUGIN_VERSION. However, we can still keep it around, but don't need to write it to _plugin_data by renaming it to __version__
  • Fixed the mypy test by adding the new errors. Those are emitted with the default mypy config (without the plugin), i.e. those are expected. _This only changes once mypy 1.1.1 is tested as it supports the @dataclass_transform decorator.

@dmontagu
Copy link
Contributor

I ran this in 1.1.1 and I didn't see the errors, so I'm going to merge. Thanks @cdce8p — I was going to say I wasn't able to get to this today, but great work figuring this all out, very much appreciated!

@dmontagu dmontagu merged commit 355e6da into pydantic:1.10.X-fixes Mar 10, 2023
@samuelcolvin
Copy link
Member

Looks great, thank you both for sorting this.

@cdce8p cdce8p deleted the dataclass-initVar-1.10.X branch March 10, 2023 08:17
dmontagu added a commit that referenced this pull request Mar 23, 2023
#5167)

* Don't apply dataclass transform twice with plugin + mypy 1.1.1 (#5162)

Co-authored-by: David Montague <35119617+dmontagu@users.noreply.github.com>
(cherry picked from commit 355e6da)

* Stop handling fields without annotations

* Drop reference to warn_untyped_fields config option for plugin

* Arghhhhhh

* Fix issue with running mypy test file in python 3.7

* Try again arrghhhh

* Make work with new validator changes

---------

Co-authored-by: David Montague <35119617+dmontagu@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants