Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix attrs.evolve with generics #15016

Closed
wants to merge 9 commits into from

Conversation

ikonst
Copy link
Contributor

@ikonst ikonst commented Apr 7, 2023

Fixes attrs.evolve for generic attrs classes.

@ikonst
Copy link
Contributor Author

ikonst commented Apr 7, 2023

👋 @JelleZijlstra you reviewed #14526 so you might have context

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@vfazio
Copy link

vfazio commented Apr 7, 2023

I was looking around for MRs touching attr.evolve after a piece of our code was getting flagged after upgrading to 1.2.0 and stumbled on this.

I was hoping this MR resolved our issue, but it doesn't appear to do so. Our use case is below. I made a minor tweak to make things "work" albeit maybe not in a satisfactory way. I'm willing to open an issue and discuss the proper fix for it if it doesn't fit with this MR re evolve + Generics.

import attr
from typing import Generic, TypeVar
from dataclasses import dataclass

C_T = TypeVar("C_T", bound="Config")

@attr.s
class Config():
    name: str = attr.ib()

@dataclass
class ConfigFactory(Generic[C_T]):
    _configs: list[C_T]

    def create_configs(self) -> None:
        for current_config in self._configs:
            x = attr.evolve(current_config, name="abc")
            print(x)

@dataclass
class TestCF(ConfigFactory[Config]):
    pass

def test() -> None:
    factory = TestCF(list())
    factory._configs.extend([Config(name="test")])
    factory.create_configs()

Where attr.evolve gets flagged: error: Argument 1 to "evolve" has incompatible type "C_T"; expected an attrs class [misc]

I needed to add:

diff --git a/mypy/plugins/attrs.py b/mypy/plugins/attrs.py
index 98090cbeb..668293fad 100644
--- a/mypy/plugins/attrs.py
+++ b/mypy/plugins/attrs.py
@@ -963,7 +963,10 @@ def evolve_function_sig_callback(ctx: mypy.plugin.FunctionSigContext) -> Callabl
     # </hack>
 
     inst_type = get_proper_type(inst_type)
+    if isinstance(inst_type, TypeVarType):
+        inst_type = inst_type.upper_bound
     inst_type_str = format_type_bare(inst_type)
+
(mypy-3.11.2) vfazio@vfazio4 /mnt/development/mypy $ python3 -m mypy -V
mypy 1.3.0+dev.edbcb217e6acdb1851a18d63a06f756fe28f2aa5 (compiled: no)
(mypy-3.11.2) vfazio@vfazio4 /mnt/development/mypy $ python3 -m mypy test.py 
test.py:14: error: Argument 1 to "evolve" has incompatible type "C_T"; expected an attrs class  [misc]
Found 1 error in 1 file (checked 1 source file)
(mypy-3.11.2) vfazio@vfazio4 /mnt/development/mypy $ git stash pop
HEAD detached at edbcb217e
Changes not staged for commit:
  (use "git add <file>..." to update what will be committed)
  (use "git restore <file>..." to discard changes in working directory)
        modified:   mypy/plugins/attrs.py

Untracked files:
  (use "git add <file>..." to include in what will be committed)
        test.py
        test2.py

no changes added to commit (use "git add" and/or "git commit -a")
Dropped refs/stash@{0} (3f973a5d6dda869c9e05bd358da9bfd1984d0a94)
(mypy-3.11.2) vfazio@vfazio4 /mnt/development/mypy $ python3 -m mypy -V
mypy 1.3.0+dev.edbcb217e6acdb1851a18d63a06f756fe28f2aa5.dirty (compiled: no)
(mypy-3.11.2) vfazio@vfazio4 /mnt/development/mypy $ python3 -m mypy test.py 
Success: no issues found in 1 source file

@github-actions

This comment has been minimized.

@ikonst
Copy link
Contributor Author

ikonst commented Apr 8, 2023

@vfazio tried addressing it in the next commits

I basically did what you suggested, but added an attrs_type in parallel with inst_type:

  • inst_type and inst_type_str are used for messages (so that we refer to T rather than the upper bound type)
  • inst_type (the T) is the return type
  • attrs_type is used to look up the attrs class and its fields

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@ikonst ikonst force-pushed the 2023-04-06-attrs-replace-generic branch from 8ade002 to e9f876e Compare April 8, 2023 04:04
@github-actions

This comment has been minimized.

@vfazio
Copy link

vfazio commented Apr 8, 2023

@vfazio tried addressing it in the next commits

I basically did what you suggested, but added an attrs_type in parallel with inst_type:

* `inst_type` and `inst_type_str` are used for messages (so that we refer to `T` rather than the upper bound type)

* `inst_type` (the `T`) is the return type

* `attrs_type` is used to look up the attrs class and its fields

Awesome, thanks!

ctx.api.fail(
f'Argument 1 to "evolve" has incompatible type "{inst_type_str}"; expected an attrs class',
f'Argument 1 to "evolve" has a variable type "{inst_type_str}" not bound to an attrs class'
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be more friendly, e.g.

has a variable type "T" that is not necessarily an attrs class; consider adding bounds= to your TypeVar definition

I can't tell where we stand between succinct and instructive.

@ikonst
Copy link
Contributor Author

ikonst commented Apr 9, 2023

Split the TypeVar fix into #15022, to be merged before this.

@ikonst ikonst marked this pull request as draft April 9, 2023 03:25
@ikonst ikonst marked this pull request as ready for review April 12, 2023 14:17
@ikonst
Copy link
Contributor Author

ikonst commented Apr 12, 2023

@hauntsaninja while I have your attention...

(There's a flood of comments above but it's basically around split-out #15022 which we already merged.)

@github-actions
Copy link
Contributor

According to mypy_primer, this change has no effect on the checked open source code. 🤖🎉

@ikonst
Copy link
Contributor Author

ikonst commented Apr 14, 2023

Closed in favor of #15050 which also addresses unions.

@ikonst ikonst closed this Apr 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants