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

Don't honor dataclass field aliases if they aren't valid identifiers #9394

Closed
wants to merge 3 commits into from

Conversation

debonte
Copy link
Collaborator

@debonte debonte commented Nov 5, 2024

What do you think of this approach? In #9386 (comment) you seemed to be suggesting fixing this in the completion provider, but it's also an issue for signature help and hover. So it seemed cleaner to handle this in the type synthesis code for dataclasses and TypedDicts. Unless this is going to cause problems elsewhere?

Copy link
Contributor

github-actions bot commented Nov 5, 2024

According to mypy_primer, this change doesn't affect type check results on a corpus of open source code. ✅

@erictraut
Copy link
Collaborator

I don't think this is the correct approach. As it was pointed out in the other thread, it's still fine to call the constructor using an unpacked dictionary, and the keys of this dictionary can be arbitrary strings. They are not limited to valid identifiers.

from pydantic import BaseModel, Field

class Resource(BaseModel):
    odataType: str = Field(alias="@odata.type")

Resource(**{"@odata.type": "int"})

Your proposed change would introduce a false positive in the above code.

@debonte
Copy link
Collaborator Author

debonte commented Nov 5, 2024

Your proposed change would introduce a false positive in the above code.

I'm not seeing a diagnostic with that code, even in strict.

@erictraut
Copy link
Collaborator

Oops, the example I provided doesn't generate an error because the literal types are not retained when evaluating the type of the dict expression. Here's a different example:

TD1 = TypedDict("TD1", {"@odata.type": str})

def func(td1: TD1):
    Resource(**td1)

My point is that your change does not correctly model the type and its runtime behaviors. The current code does, and I don't think we should change it to fix shortcomings in the LS provider code. The right place to fix this is in the completion provider and the signature help provider.

I don't think there's a need to change this in the hover provider. It's arguably showing the correct type for the __init__ method even though the signature looks a bit odd and cannot be created through a def statement. There are other cases like this, such as when a ParamSpec specialization creates a signature that has multiple parameters with the same name.

@debonte debonte closed this Nov 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants