Fix handling of Enums in Literal types

[brakhane]

Literal allows Enum values to be specified, however, the Literal check did not account for that:

import cattrs, enum
from typing import Literal

class A(enum.Enum):
    BLA = 1
    BLAA = 2

@attrs.define
class Works:
    a: A

@attrs.define
class Broken:
    a: Literal[A.BLA]

print(cattrs.structure({"a": 1}, Works))   # Works(a=<A.BLA: 1>)
print(cattrs.structure({"a": 1}, Broken))  # Exception: 1 not in literal typing.Literal[<A.BLA: 1>]

(This would work if A would also inherit from int.)

It makes sense that if cattrs already accepts 1 as a valid enum value, the Literal check should go by value as well.

This PR fixes this bug, and also slightly modifies an existing test to make sure that Literal[1, Literal[2]] is handled like Literal[1, 2]. (Edit: the last part is not correctly supported by cattrs yet, so the test was reversed in a later version of this PR)

[Tinche]

Cool, can you compare the performance of structuring Literal[1] using pyperf timeit?

[codecov-commenter]

Codecov Report

Merging #231 (9fb9c44) into main (3a5c1f6) will decrease coverage by 10.47%.
The diff coverage is 37.50%.

@@             Coverage Diff             @@
##             main     #231       +/-   ##
===========================================
- Coverage   98.18%   87.71%   -10.48%     
===========================================
  Files          16       16               
  Lines         770      773        +3     
===========================================
- Hits          756      678       -78     
- Misses         14       95       +81     

Impacted Files	Coverage Δ
src/cattr/converters.py	91.91% <37.50%> (-7.00%)	⬇️
src/cattr/_compat.py	60.25% <0.00%> (-33.98%)	⬇️
src/cattr/_generics.py	80.00% <0.00%> (-20.00%)	⬇️
src/cattr/dispatch.py	100.00% <0.00%> (ø)

📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more

[brakhane]

OK, I've run tests with pyperf timeit -s 'import cattrs' -s 'from typing import Literal' 'cattrs.structure(1, Literal[1])' and noticed I could optimize the code a bit by only creating the list of Enums if the naïve check fails, I've pushed a new version.

original: Mean +- std dev: 1.34 us +- 0.01 us
old vers: Mean +- std dev: 2.03 us +- 0.02 us
new vers: Mean +- std dev: 1.36 us +- 0.02 us

original is the "broken" version without proper Enum support. "old" and "new" refer to the old vs the new patch.

I've also noticed that cattrs currently does not handle nested Literals correctly, which is why the test on 3.8 failed. Therefore, I've removed the test that I added which checks that, and might make a separate fix for that later.

[brakhane]

I'm sorry, I had overoptimized and broken the resolution for Enums that also inherit from int or str.

The new version is a bit slower, but I doubt it matters in reality, and it also does the correct thing now, which is a plus ;)

Not supporting Enums: Mean +- std dev: 1.46 us +- 0.06 us
Supporting Enums:     Mean +- std dev: 1.92 us +- 0.03 us

[Tinche]

Here are my (adjusted) benchmarks:

$ pyperf timeit -s 'from cattrs import structure; from typing import Literal; l = Literal[1]' 'structure(1, l)'
main: 1.33 us +- 0.08 us
pr: 2.03 us +- 0.13 us

So, significant.

You might look into how to turn this from a hook into a hook factory, and produce special hooks for literals with primitives and literals with enums. I can provide support on this.

[brakhane]

Here are my (adjusted) benchmarks:

$ pyperf timeit -s 'from cattrs import structure; from typing import Literal; l = Literal[1]' 'structure(1, l)'
main: 1.33 us +- 0.08 us
pr: 2.03 us +- 0.13 us

So, significant.

One year ago I would have agreed, but since watching https://www.youtube.com/watch?v=r-TLSBdHe1A I now understand that these kinds of measurements should be taken with a grain of salt. (I don't mean this in a "I know better" kind of way, I just want to highly recommend the talk since it was an eye opener for me)

You might look into how to turn this from a hook into a hook factory, and produce special hooks for literals with primitives and literals with enums. I can provide support on this.

I think cattrs should do the correct thing by default. Currently, it literally (pun intended) violates the Literal constraint:

class Foo(int, enum.Enum):
    FOO = 1

@attrs.define
class Bar:
    foo: Literal[Foo.FOO]

bar = cattrs.structure({"foo": 1}, Bar)
print(type(bar.foo)) # int

[Tinche]

I'm saying we can do the right thing quickly by using hook factories. ;)

[brakhane]

I'm saying we can do the right thing quickly by using hook factories. ;)

Ah, I misunderstood then. :) I will take a look at it later.

[brakhane]

OK, that was easier than expected. New version is back at Mean +- std dev: 1.45 us +- 0.03 u

[Tinche]

Ah I had a different solution in mind but yours is actually better. Just add a changelog entry please.

[brakhane]

done.

[brakhane]

Ah I had a different solution in mind but yours is actually better. Just add a changelog entry please.

BTW, may I ask what you had in mind? I'm still new to cattrs, and probably haven't grokked all details yet.

[Tinche]

Hello! I'm merging in stuff in preparation for a release, and it's this PR's turn ;) So please rebase and let's get it in.

To answer your question, I was thinking of creating a hook factory per literal seen, with a special code path for the simple case to make it fast. But your approach accomplishes the same thing but simpler, so good job.

[Tinche]

Looks like there's a formatting error; I can fix this myself. Thanks!