Better support for converter in attrs plugin. #4607
Conversation
My first pass at this only supported converters that were reachable from the root. With this code the converter can be a local function too. Also now when we can't detect the type of the converter the init type becomes Any.
| raise NotImplementedError | ||
|
|
||
| @abstractmethod | ||
| def lookup_qualified(self, name: str, ctx: Context, |
There was a problem hiding this comment.
It would be better to use only this function, rather than all four lookup functions. But if it is not easy to fix, then ignore this, I will fix this later.
| if (converter | ||
| and converter.type | ||
| and isinstance(converter.type, CallableType) | ||
| and converter.type.arg_types): | ||
| init_type = converter.type.arg_types[0] | ||
| else: | ||
| init_type = None | ||
| init_type = AnyType(TypeOfAny.from_error) |
There was a problem hiding this comment.
The rule is to show error message for Anys that are from_error. In this case you can show something like Cannot determine type of converter function, or unsupported converter.
| init_type = AnyType(TypeOfAny.from_error) | ||
| elif self.converter_name == '': | ||
| # This means we had a converter but it's not of a type we can infer. | ||
| init_type = AnyType(TypeOfAny.from_error) |
There was a problem hiding this comment.
The above comment applies also here.
| class C: | ||
| x: str = attr.ib(converter=thing.do_it) | ||
| y: str = attr.ib(converter=lambda x: x) | ||
| z: str = attr.ib(converter=factory(8)) |
There was a problem hiding this comment.
These should give errors until supported.
By the way I am thinking about how to support this. One possible way is to just accept type checker at a later stage. This would require a second part of the plugin that would be triggered on visiting this TypeInfo in type checker. We also need to be sure to mark all nodes that need this type as deferred. TBH I am sure good support for converters worth all this (including potential performance problems). So maybe we can just propose a workaround like always explicitly define your converters i.e. use
def str_first_item(seq: List[int]) -> int:
"""A docstring for why one needs this"""
return str(seq[0])
@attr.s
class C:
x: str = attr.ib(converter=str_first_item)instead of
attr.s
class C:
x: str = attr.ib(converter=lambda x: str(x[0]))There was a problem hiding this comment.
Yeah. That's what I was already thinking for my own code. Should I mention this in the error message? "only functions supported" or something like that?
There was a problem hiding this comment.
Should I mention this in the error message? "only functions supported" or something like that?
Yes, this will be helpful. Maybe like "only named functions" (to make it clear that lambdas are not fully supported).
| reveal_type(A) | ||
| [builtins fixtures/list.pyi] | ||
| [out1] | ||
| main:8: error: Revealed type is 'def (x: str?) -> __main__.A@5' |
There was a problem hiding this comment.
Why str is shown as unbound type here? This may cause problems later.
There was a problem hiding this comment.
I'll investigate
This one actually worries me most. It would be great to fix at least this one before the release.
| zz: bool = attr.ib(converter=bar.maybe_bool) | ||
| [builtins fixtures/list.pyi] | ||
| [out1] | ||
| main:2: error: Revealed type is 'def (x: Union[builtins.int, builtins.None], y: Union[builtins.str, builtins.None], z: Union[builtins.bool, builtins.None]) -> base.Base' |
There was a problem hiding this comment.
This is a bit hard to read. Could you please change this to instead checking types of things by assignments that would cause an error on the second run only? For example:
import a
[file a.py]
from b import thing
x: str = thing # OK
[file a.py.2]
from b import thing
x: int = thing # Error
[file b.py]
thing = 'thing'
There was a problem hiding this comment.
Ok. It'll be more like checking class creation though.
|
@gvanrossum I would revert this and merge later, there are some suspicious thing (like unbound types where they should be bound). |
This reverts commit f384df8.
More meesages, no unbound types, better tests. In response to comments in #4607
* master: (27 commits) Don't call --strict-optional and --incremental experimental (python#4642) Sync typeshed (python#4641) Fix callable types with inconsistent argument counts (python#4611) Fix example (add 'class A:') Make psutil an optional dependency (python#4634) mypy and mypy_extensions aren't posix only (python#3765) Documentation for attr support (python#4632) Use read_with_python_encoding in stubgen to handle file encoding (python#3790) Sync typeshed (python#4631) Add remaining core team emails to CREDITS (python#4629) Fix issues with attr code. (python#4628) Better support for converter in attrs plugin. (python#4607) Clean up credits (python#4626) Support type aliases in fine-grained incremental mode (python#4525) Fine-grained: Fix crash caused by unreachable class (python#4613) Treat divmod like a binary operator (python#4585) Sync typeshed (python#4605) Fine-grained: Don't infer partial types from multiple targets (python#4553) Fine-grained: Compare symbol table snapshots when following dependencies (python#4598) Fix type of forward reference to a decorated class method (python#4486) ...
Fixes python#4583 (and more). My first pass at this only supported converters that were reachable from the root. With this code the converter can be a local function too. Also now when we can't detect the type of the converter the init type becomes Any.
More meesages, no unbound types, better tests. In response to comments in python#4607
My first pass at this only supported converters that were reachable
from the root. With this code the converter can be a local function
too.
Also now when we can't detect the type of the converter the init type
becomes Any.
Supersedes #4583