python-attrs / cattrs

Composable custom class converters for attrs, dataclasses and friends.
https://catt.rs
MIT License
799 stars 111 forks source link

SyntaxError generating TypedDict structure function #376

Closed simonschmidt closed 1 year ago

simonschmidt commented 1 year ago

Since 23.1 creating a converter for TypedDict can lead to SyntaxError in the generated function if the TypedDict fields are not valid identifiers.

For example here's a TypedDict with a field containing .

from typing import TypedDict
import cattrs

converter = cattrs.Converter()

TD = TypedDict(
    "TD",
    {"x.y": int},

)

print(
    converter.structure(
        {"x.y": "3"},
        TD,
    )
)
Traceback (most recent call last):
  File "<string>", line 1, in <module>
  File "/app/cattrs/src/repro.py", line 13, in <module>
    converter.structure(
  File "/app/cattrs/src/cattrs/converters.py", line 334, in structure
    return self._structure_func.dispatch(cl)(obj, cl)
  File "/app/cattrs/src/cattrs/dispatch.py", line 48, in _dispatch
    res = self._function_dispatch.dispatch(typ)
  File "/app/cattrs/src/cattrs/dispatch.py", line 133, in dispatch
    return handler(typ)
  File "/app/cattrs/src/cattrs/converters.py", line 973, in gen_structure_typeddict
    return make_typeddict_dict_struct_fn(
  File "/app/cattrs/src/cattrs/gen/typeddicts.py", line 513, in make_dict_structure_fn
    eval(compile(script, fname, "exec"), globs)
  File "<cattrs generated structure repro.TD>", line 1
    def structure_TD(o, _, *, __cl=__cl, __c_cve=__c_cve, __c_avn=__c_avn, __c_structure_x.y=__c_structure_x.y, __c_type_x.y=__c_type_x.y, __c_structure_a=__c_structure_a, __c_type_a=__c_type_a):

SyntaxError: invalid syntax

The issue seems to be in how struct_handler_name and type_name are created in https://github.com/python-attrs/cattrs/blob/main/src/cattrs/gen/typeddicts.py#L241

Though I don't know the details here well enough to know if it'd be sufficient to sanitize those variables or if there's more going on here.

Tinche commented 1 year ago

Ooph ok, thanks for reporting. We'll need to figure something out, yeah.

Tinche commented 1 year ago

Hi, I think this is fixed on main, can you verify for your use case?

simonschmidt commented 1 year ago

It's fixed, thank you!