Skip to content

TypedDict typecheck fails if extra dict keys present #6223

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

Closed
zhammer opened this issue Jan 18, 2019 · 8 comments
Closed

TypedDict typecheck fails if extra dict keys present #6223

zhammer opened this issue Jan 18, 2019 · 8 comments
Labels
bug mypy got something wrong false-positive mypy gave an error on correct code priority-1-normal topic-typed-dict

Comments

@zhammer
Copy link

zhammer commented Jan 18, 2019

  • Are you reporting a bug, or opening a feature request?
    Bug (possibly, as per @ilevkivskyi on gitter)
Zach-Hammer:typedict zachhammer$ pipenv graph
mypy==0.660
  - mypy-extensions [required: >=0.4.0,<0.5.0, installed: 0.4.1]
  - typed-ast [required: >=1.2.0,<1.3.0, installed: 1.2.0]

Zach-Hammer:typedict zachhammer$ cat person.py 
from mypy_extensions import TypedDict


class Person(TypedDict):
    name: str
    age: int


def api_fetch_person() -> Person:
    return {
        'name': 'Zach',
        'age': 25,
        'meta': 'other info'
    }

Zach-Hammer:typedict zachhammer$ pipenv run mypy person.py 
person.py:10: error: Extra key 'meta' for TypedDict "Person"
  • What is the actual behavior/output?
    I have a TypedDict Person that has fields name and age
    I have a function that returns a dictionary with name, age, and meta
    mypy throws error: Extra key 'meta' for TypedDict "Person"

  • What is the behavior/output you expect?
    I was testing this out of curiosity, since it's not specified (as far as i can tell) in the TypedDict documentation. I wanted to see if typeddict allowed extra, unspecified fields. (in which case it'd behave like a typescript interface). I posted the outcome in the mypy gitter and @ilevkivskyi suggested I open an issue here.

  • Do you see the same issue after installing mypy from Git master?
    Can't do this at the moment but can try over the weekend

@zhammer
Copy link
Author

zhammer commented Jan 18, 2019

Zach Hammer @zhammer 15:07
dont have the opportunity to run with master mypy at the moment but can test soon!

Ivan Levkivskyi @ilevkivskyi 15:17
NP, I already tested, it behaves the same way on master

@zhammer
Copy link
Author

zhammer commented Jan 18, 2019

easier for repro:

mkdir mypy-6223
cd mypy-6223

pipenv --python python3 install 'mypy==0.660' 'mypy_extensions==0.4.1'

echo "
from mypy_extensions import TypedDict


class Person(TypedDict):
    name: str
    age: int


def api_fetch_person() -> Person:
    return {
        'name': 'Zach',
        'age': 25,
        'meta': 'other info'
    }
" > example.py

pipenv run mypy example.py

@ilevkivskyi
Copy link
Member

Yes, I think this should be allowed because of the structural nature of TypedDicts. @JukkaL do you have an idea why we need the check for extra keys?

@ilevkivskyi ilevkivskyi added bug mypy got something wrong topic-typed-dict priority-1-normal false-positive mypy gave an error on correct code labels Jan 19, 2019
@JukkaL
Copy link
Collaborator

JukkaL commented Jan 21, 2019

Constructors are treated specially, and this is by design. Otherwise mypy wouldn't catch misspelled (non-total TypedDict) or extra TypedDict items when using the dict literal syntax.

If we modified this to work as suggested above, we'd compromise basic type safety to better support a pretty marginal use case. (This is the first report of this issue even though TypedDict has been widely used for a long time now, so I doubt this is a common issue.)

As a workaround, you can either use a cast or declare a wider TypedDict type and perform an extra assignment to a variable annotated with the wider type.

@JukkaL JukkaL closed this as completed Jan 21, 2019
@zhammer
Copy link
Author

zhammer commented Jan 21, 2019

Ah okay. Didn't realize this was a strictly constructor case. Thanks @JukkaL

@ilevkivskyi
Copy link
Member

@JukkaL I see why prohibiting extra keys could be useful for non-total TypedDicts, but...

If we modified this to work as suggested above, we'd compromise basic type safety

Could you please give an example where allowing the original code would cause problems? Or where allowing

person: Person = {'name': 'Zach', 'age': 25, 'meta': 'other info'}

would cause problems?

@JukkaL
Copy link
Collaborator

JukkaL commented Jan 22, 2019

You are correct that in the sense that this wouldn't break type-theoretic type safety. However, I think that it would break some fairly basic use cases.

Here's one example where there would be a problem:

Person = TypedDict('Person', {'name': str, 'address': str}, total=False)

p: Person = {'name': 'Jim', 'addresss' : '1 High St'}  # Currently typo (extra 's') is an error

p2: Person = {}
p2['name'] = 'Amy'
p2['addresss'] = '1 HighSt'  # Typo is an error

The first typo would not be caught if we'd allow extra keys. Another example is renaming a key of a non-total TypedDict -- mypy wouldn't complain about using the old key, making refactoring harder. Also, the above example suggests that the two different ways of constructing a TypedDict would behave inconsistently, as a typo would only be caught using the second, longer form. Maybe we should allow the typo in the second form as well, for consistency (I'm not really suggesting this)?

For total TypedDicts the issue would be more subtle. Assume we have a TypeDict with multiple items, and we want to delete one item as part of a refactoring. Currently mypy will show places where the deleted key is used, making it easy to perform this refactoring. With the proposed change, the deleted key would still be valid in constructors.

(If we'd allow this for total TypedDicts, for consistency it should probably also be allowed for non-total TypedDicts. Currently non-total TypedDicts are strictly more flexible than total TypedDicts, and I think that this is a nice property.)

To argue for the change, we'd need evidence that the supported use case is significantly more useful that the use cases that it would break. Even a slight improvement probably wouldn't make it worthwhile, since the existing behavior wins by default (since users are already familiar with it, and any change involves some work and risk).

@ilevkivskyi
Copy link
Member

You are correct that in the sense that this wouldn't break type-theoretic type safety. However, I think that it would break some fairly basic use cases.

Thanks for the example! I am not that strongly in favor of the proposal, just wanted to double-check that I am not missing anything here. I actually agree that supporting refactorings is probably more important here, taking into account there is a simple workaround for the proposed use case.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug mypy got something wrong false-positive mypy gave an error on correct code priority-1-normal topic-typed-dict
Projects
None yet
Development

No branches or pull requests

3 participants