Typeddict context returned correctly when unambiguous #8212
Conversation
mypy/messages.py
Outdated
| self, | ||
| types: List[TypedDictType], | ||
| context: Context) -> None: | ||
| self.fail('Type of TypedDict is Ambiguous, could be any of {}'.format( |
There was a problem hiding this comment.
Don't capitalize Ambiguous
mypy/messages.py
Outdated
| types: List[TypedDictType], | ||
| context: Context) -> None: | ||
| self.fail('Type of TypedDict is Ambiguous, could be any of {}'.format( | ||
| ([(format_type(typ), format_item_name_list(typ.items.keys())) for typ in types]) |
There was a problem hiding this comment.
The type error message here is ugly. My inclination is to just print the types, though if you can make the rest look good and having clear meaning then that's OK. We definitely don't want to be printing out a bunch of brackets and parens and quotes that come from python's default reprs.
If multiple names are being formatted, also, we want to use format_type_distinctly to make sure they don't end up getting given the same message. Unfortunately format_type_distinctly only works on two strings, so I went and made a PR (#8270) to fix that.
There was a problem hiding this comment.
Yeah I wasn't quite sure what to do about printing the types here. Thanks a lot for that PR, I've updated to use format_type_distinctly and it's much cleaner! Also if you can think of any slightly nicer wording do let me know.
mypy/checkexpr.py
Outdated
| @@ -3294,19 +3313,25 @@ def visit_dict_expr(self, e: DictExpr) -> Type: | |||
| assert rv is not None | |||
| return rv | |||
|
|
|||
| def find_typeddict_context(self, context: Optional[Type]) -> Optional[TypedDictType]: | |||
| def find_typeddict_context(self, context: Optional[Type], | |||
| dict_expr: DictExpr) -> Optional[TypedDictType]: | |||
There was a problem hiding this comment.
(Has the linter broken??). Indent dict_expr another space
There was a problem hiding this comment.
Yeah this was weird, my vscode showed this being inline! I've added the space in but yeah weird this wasn't picked up.
|
Thanks for the review @msullivan! Any more comments would be great |
test-data/unit/check-typeddict.test
Outdated
| A = TypedDict('A', {'@type': Literal['a-type'], 'a': str}) | ||
| B = TypedDict('B', {'@type': Literal['a-type'], 'a': str}) | ||
|
|
||
| c: Union[A, B] = {'@type': 'a-type', 'a': 'Test'} # E: Type of TypedDict is ambiguous, could be any of ('"A"', '"B"') \ |
There was a problem hiding this comment.
Nit: Both single and double quotes in the error message ('"A"' etc.) look strange. It would be better to only use double quotes.
There was a problem hiding this comment.
Thanks for the review! I've updated to have double quotes although I could also have used format_type_distinctly directly with the bare argument set to true, would have had the same result except for single quotes (maybe that would have been cleaner?).
Typeddict context returned correctly when unambiguous (python#8212)
Fixes #8156
This is a suggestion as to how we could handle getting the Typeddict context from a Union with more than one Typeddict.
The idea is that we check each of the Typeddict types to find one who's keys match that of the expression, if more than one match it's then ambiguous. The slightly weird thing is that this matching is only applied in the Union case, if there is only a single Typeddict it's returned as before.
I'm not sure that I've put the tests in the most appropriate place, and suggestions would be great.