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

Problem with setting one TypedDict variable to another TypedDict variable #6577

Open
ckarnell opened this issue Mar 20, 2019 · 9 comments
Open

Comments

@ckarnell
Copy link
Contributor

ckarnell commented Mar 20, 2019

  • Are you reporting a bug, or opening a feature request?

Bug

  • Please insert below the code you are checking with mypy,
    or a mock-up repro if the source is private. We would appreciate
    if you try to simplify your case to a minimal repro.
TestOne = TypedDict('TestOne', {'test': int, 'another': str}, total=False)
TestTwo = TypedDict('TestTwo', {'test': int}, total=False)

var_1: TestOne = {'test': 1, 'another': 'hi'}
var_2: TestTwo = var_1  # Doesn't cause a type error... why not?? 'another' is not a valid key on a TestTwo
var_3: TestTwo = {  # Does cause a type error, correctly. "Extra key 'another' for TypedDict 'TestTwo'"
    'test': 2, 'another': 'hi'
}

TestThree = TypedDict('TestThree', {'test': int}, total=False)
TestFour = TypedDict('TestFour', {'test': int, 'another': str}, total=False)
var_4: TestThree = {'test': 3}
var_5: TestFour = var_4  # Causes a type error.... WHY???? "Incompatible types in assignment (expression has type "TestThree", variable has type "TestFour")"
  • What is the actual behavior/output?

Basically, the code above, including its inline notes, but I'll summarize:

If you assign a value who's type is a TypedDict with some keys and total=False to be the value of a variable who's type is another TypedDict with total=False and a subset of the first TypedDict's keys, it throws an error, but it should not.

If you assign a value who's type is a TypedDict with some keys and total=False to be the value of a variable who's type is another TypedDict with total=False and a superset of the first TypedDict's keys, it does not throw an error, but it should.

  • What are the versions of mypy and Python you are using?

Python version 3.6.4
Mypy version 0.670

  • Do you see the same issue after installing mypy from Git master?

After installing mypy-0.680+dev.4e0a1583aeb00b248e187054980771f1897a1d31 from github, I get the same exact errors.

  • What are the mypy flags you are using? (For example --strict-optional)

Here is my mypy.ini:

[mypy]
plugins = sqlmypy
disallow_untyped_calls = False
disallow_untyped_defs = True
ignore_missing_imports = True
warn_return_any = False
follow_imports = silent
allow_redefinition = True
  • If mypy crashed with a traceback, please paste

    It did not.

@JukkaL
Copy link
Collaborator

JukkaL commented Mar 26, 2019

Your example is not type safe, though it may not be immediately obvious why. Consider this example, which is similar to your example but with a twist:

from mypy_extensions import TypedDict

T1 = TypedDict('T1', {'x': int, 'y': int}, total=False)
T2 = TypedDict('T2', {'x': int}, total=False)
T3 = TypedDict('T3', {'x': int, 'y': str}, total=False)

t3: T3 = {'x': 0, 'y': 'foo'}
t2: T2 = t3
t1: T1 = t2  # This is currently an error
t1['y'] = 1
if 'y' in t3:
    print(t3['y'] + 'bar')  # Runtime error

If we'd allow the assignment, mypy wouldn't be able to prevent the runtime error.

I can see how allowing the t1: T1 = t2 assignment would seem like a reasonable thing. You might be able to convince me that mypy should allow it even if it's technically unsafe.

@ckarnell
Copy link
Contributor Author

@JukkaL Thank you for the response. I think I'm fundamentally misunderstanding something - in your example, why are you allowed to set t2: T2 = t3 when t3 has a super set of the values T2 allows?

It's a type error if you do this:

T2 = TypedDict('T2', {'x': int}, total=False)
t2: T2 = {'x': 5, 'y': 'hi'}

It says "Extra key 'y' for TypedDict "T2"". That makes perfect sense to me.

So why then are you allowed to do t2: T2 = t3 when t3 also might have the extra key 'y'?

The behavior I would expect is that t2: T2 = t3 does throw an error to be consistent with other errors, but t1: T1 = t2 would not because it can't have any extra keys, according to its type.

Can you let me know what's missing in my mental model here?

@JukkaL
Copy link
Collaborator

JukkaL commented Mar 26, 2019

Mypy treats creation of a TypedDict as a separate operation from assignment. This is creation (since there's a dictionary display/literal):

t: T = {'x': 0}

This is assignment (we don't construct a new instance):

t2: T2 = t

In creation we reject extra items, since this is typically a code quality issue. For example, if we remove a TypedDict item, we'd generally like mypy to help us remove the item from constructed TypedDict objects. Also, if you misspell the key of a non-total TypedDict, mypy will catch this. However, when we assign a TypedDict to a variable with a different TypedDict type, we use structural compatibility, similar to how protocols and ABCs work.

A good mental model might be to treat construction equivalent to a constructor call such as T2(x=5, y='hi'), while non-construction assignment is just an assignment. Mypy will also complain if you pass an extra argument to the constructor of a class or a named tuple.

@ckarnell
Copy link
Contributor Author

ckarnell commented Mar 26, 2019

Thanks again for the response! I understand fully now how it works currently. I guess my next question is, is that what we actually want? This still seems like basically the opposite of what I and I have to imagine most users would expect.

For two TypedDicts with total=False where one has a subset of the keys of the other, shouldn't the one with the subset of keys be the valid subtype and not the other way around? It seems like this would merit another type of compatibility similar to structural subtyping that checks if one type has at most all the attributes of the other. Do you agree? Otherwise TypedDicts seem much less usable, which is disappointing.

As an example, I have an api where the endpoint accepts some data of type TypedDict A with total=False, and I want to pass that data to a service that accepts data of shape TypedDict B with total=False, and A has a subset of B's keys. I want to be able to pass that data to the service, since data of shape A fits into B. This seems like it would be a very common use case (and would match closely with what TypedDict seems like it should do), but it sounds like the opposite is what it actually does.

I can't make TypedDict B a class that has TypedDict A as a base class as someone in gitter suggested because I can't have the type of the data my service takes be dependent on the api - the opposite should be true! I also can't just make A accept all the keys of B because then I lose a lot of type checking value and it's no longer clear what subset of B's keys A can get. Do you see the issue?

Maybe TypedDicts could accept another argument that indicates it should use the kind of checking I suggest rather than structural checking?

@ckarnell
Copy link
Contributor Author

I would also be willing to put in the work to add this feature - ability to type check in this way is important to me.

@JukkaL
Copy link
Collaborator

JukkaL commented Mar 28, 2019

I'm actually currently writing a PEP to standardize how TypedDict works, and you can bring this change up in the discussion that will follow. I'm planning to post the PEP draft to the typing-sig mailing list in the not too distant future (python/typing#590 discusses how to subscribe).

An alternative way to make this use case a bit better would to provide a custom cast function that will "widen" a TypedDict type. For example, assume A has fewer items than B (and both are non-total). Now this would be okay:

from mypy_extensions import widen

def f(b: B) -> None: ...

a: A = { ... }
f(widen(a))  # OK

@ckarnell
Copy link
Contributor Author

I'll definitely join that mailing list and take part in discussion, thanks for the link!

In the meantime, if I were to write a plugin that would create a wrapper around the current check that happens when a TypedDict variable gets set to another TypedDict value, which hook should I add? The docs don't make it explicitly clear to me. Also, can you tell me where in the source code this check happens so I can use it for reference?

If I can't figure out how to do that, I'll likely write a minimal proprietary "safely_cast_to_some_other_typed_dict" type function like you suggest.

@ilevkivskyi
Copy link
Member

I think this only appeared once before in #6522

Since mypy is correct (it flags unsafe code), I think we can live with status quo if we clearly document this behavior (plus maybe give a better error message).

JukkaL added a commit that referenced this issue Dec 14, 2021
* Typeshed cherry-pick: Relax signature of logging.config.loadConfig (#6577)

* Typeshed cherry-pick: Use AbstractSet instead of set in random and inspect (#6574)

* Partial typeshed cherry-pick: Add `SupportsRichComparison` type to `_typeshed` (#6583)

The original PR doesn't apply cleanly so I only included the changes to
builtins to avoid having to manually deal with all the conflicts.
tushar-deepsource pushed a commit to DeepSourceCorp/mypy that referenced this issue Jan 20, 2022
* Typeshed cherry-pick: Relax signature of logging.config.loadConfig (python#6577)

* Typeshed cherry-pick: Use AbstractSet instead of set in random and inspect (python#6574)

* Partial typeshed cherry-pick: Add `SupportsRichComparison` type to `_typeshed` (python#6583)

The original PR doesn't apply cleanly so I only included the changes to
builtins to avoid having to manually deal with all the conflicts.
@emmatyping
Copy link
Collaborator

When fixing this we should make sure we follow the spec: https://typing.readthedocs.io/en/latest/spec/typeddict.html#assignability

The issue boils down to mypy has the assignability backwards - I fear fixing this would have a bad primer report :/

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants