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

Disallow x["foo"] for NotRequired TypedDict access #12094

Open
bsmedberg-xometry opened this issue Jan 28, 2022 · 8 comments · May be fixed by #14717
Open

Disallow x["foo"] for NotRequired TypedDict access #12094

bsmedberg-xometry opened this issue Jan 28, 2022 · 8 comments · May be fixed by #14717

Comments

@bsmedberg-xometry
Copy link

Feature

After PEP 655 was implemented, mypy now checks initialization of typed dicts. Access to these dicts however is not checked for required/optional.

Pitch

We have lots of code which makes use of TypedDict with optional parameters, and one of the most common coding errors is to call dict_value["param"]. Because these are not required keys, a sound type checker should disallow this access and instead require dict_value.get("param").

This problem was mentioned near the end of #2632 but no subsequent ticket was filed.

I intend to have a PR for this up in the next few minutes.

bsmedberg-xometry added a commit to xometry/mypy that referenced this issue Jan 28, 2022
…e should

always be accessed through .get() because the keys may not be present.

Fixes python#12094
@chuckwondo
Copy link

I just ran across this problem as well. I wouldn't have realized that this was something that mypy wasn't identifying if Pylance hadn't properly identified it with an error message of the form:

"name" is not a required key in "MyTypedDict", so access may result in runtime exception.

I thought that I was perhaps just not configuring mypy correctly for it to identify the problem, but I now see that it's simply not something that mypy identifies yet.

@Kamforka
Copy link

@chuckwondo Pylance won't identify erroneous access to optional parameters for my typed dicts, did you have to configure it to highlight such issues?

@chuckwondo
Copy link

@Kamforka, in my VS Code settings, I set "Python > Analysis: Type Checking Mode" from its default value of "off" to "basic"

After doing that, such erroneous access is highlighted.

@Kamforka
Copy link

@chuckwondo indeed that was the missing setting!
Thanks!

bsmedberg-xometry added a commit to xometry/mypy that referenced this issue Feb 16, 2023
2. When using .get() on a typeddict, the result type will now be a union of the dict[key] type and the type of the default parameter, instead of `object`

Fixes python#12094 - replaces python#12095 which is now bitrotted
@davidfstr
Copy link
Contributor

davidfstr commented Mar 18, 2023

@bsmedberg-xometry did start a PR proposing an implementation for this issue. However it requires more attention to get it to a place that doesn't mark a lot of errors in reasonable-looking existing code. Additional help from other followers of this issue is requested.

To paraphrase from the PR thread (plus integrating some comments from this issue), an implementation should satisfy:

class A(TypedDict):
    t: NotRequired["str"]

def index_a_t_1(a: A) -> str:
    # ERROR: TypedDict "A" key "t" is not required and might not be present.
    return a["t"]

def index_a_t_2(a: A) -> str:
    assert "t" in a
    # OK, because of type narrowing by: "t" in a
    return a["t"]

def index_a_t_3(a: A) -> str:
    if "t" in a:
        # OK, because of type narrowing by: "t" in a
        return a["t"]
    return "unknown"

def del_a_t_1(a: A) -> None:
    # ERROR: TypedDict "A" key "t" is not required and might not be present.
    del a["t"]

def del_a_t_2(a: A) -> None:
    assert "t" in a
    # OK. Is OK to del a NotRequired key iff type narrowed to Required
    del a["t"]

def get_a_t(a: A) -> str:
    # OK. Always OK to use get(K) or get(K, V) on a NotRequired key
    return a.get("t", "unknown")

def pop_a_t_1(a: A) -> None:
    # OK. Always OK to pop(K, V) a NotRequired key
    pop a("t", None)

def pop_a_t_2(a: A) -> None:
    assert "t" in a
    # OK. Always OK to pop(K, V) a NotRequired key, even if type narrowed to Required by: "t" in a
    pop a("t", None)

# pop(K) behaves in same way as del[K], even though pop(K, V) has different rules above

Edit: Did revise del cases above based on feedback from @JelleZijlstra below. Did also add separate pop cases.

@JelleZijlstra
Copy link
Member

def del_a_t_1(a: A) -> None:
    # OK. Always OK to del a NotRequired key
    del a["t"]

I don't think this should be OK, since del throws an error if the key is not present. Users should use a.pop("t", None) instead.

@davidfstr
Copy link
Contributor

I don't think this should be OK, since del throws an error if the key is not present.

@JelleZijlstra Agreed. I've revised the test cases in the comment above to reflect your feedback.

@untitaker
Copy link

untitaker commented May 26, 2023

We just ran across this problem as well. In our case, TypedDicts are generated code from schema definitions.

Therefore we are considering a workaround where, instead of a single typeddict, we generate two typeddicts and combine them via a Union:

class MyDict(TypedDict, total=False):
    foo_required: Required[int]
    foo_optional: NotRequired[int]


class MinimalDict(TypedDict, total=False):
    foo_required: Required[int]

MyDict2 = Union[MyDict, MinimalDict]


# mypy (incorrectly) thinks this code is correct
def x(d: MyDict):
    d['foo_optional']
    d['foo_required']

# mypy (correctly) errors because `foo_optional` may not exist
def y(d: MyDict2):
    d['foo_optional']
    d['foo_required']

this still has some edgecases where mypy will behave incorrectly, but I think it might catch the most obvious mistakes until this is fixed in mypy.

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