python-betterproto icon indicating copy to clipboard operation
python-betterproto copied to clipboard

One-of pattern matching not supported by static type analysis tools

Open 124C41p opened this issue 1 year ago • 7 comments

According to the Readme it should be possible to access fields of a oneof-group by pattern matching so that static analysis tools can provide type hints:

test = Test()
match test:
    case Test(on=value):
        print(value)  # value: bool
    case Test(count=value):
        print(value)  # value: int
    case Test(name=value):
        print(value)  # value: str
    case _:
        print("No value provided")

However, the tool pyright (used by the pylance extension for vscode) does not provide type hints for the second and third case. I first thought this was a bug in pyright, but according to a pyright maintainer this is actually intentional. Apparently, from the point of view of a type checker, the second and third case blocks are unreachable (this pattern even triggers a warning from pyright when configured accordingly).

Possible solution

Let's assume we change the compiler to generate the following class:

@dataclass(eq=False, repr=False)
class Test(betterproto.Message):
    on: Optional[bool] = betterproto.bool_field(1, group="foo")
    count: Optional[int] = betterproto.int32_field(2, group="foo")
    name: Optional[str] = betterproto.string_field(3, group="foo")

Then the following match statement is properly supported by pyright:

test = Test()
match test:
    case Test(on=bool(value)):
        print(value)  # value: bool
    case Test(count=int(value)):
        print(value)  # value: int
    case Test(name=str(value)):
        print(value)  # value: str
    case _:
        print("No value provided")

I think one could argue that it is actually correct to mark these fields optional. What do you think?

System Information

  • python 3.11.9
  • betterproto 2.0.0b7
  • pylance v2024.8.1

124C41p avatar Aug 22 '24 09:08 124C41p

This was just changed, this seems not ideal.

Gobot1234 avatar Aug 22 '24 12:08 Gobot1234

What does the default implementation do when accessing an unset field? I'm slightly more inclined to follow that behaviour but I've been thinking about this. The change would need to be made quickly to minimise disruption in this regard.

Gobot1234 avatar Aug 22 '24 21:08 Gobot1234

As of version 2.0.0b7, accessing an ordinary unset field gives the default value, whereas accessing an unset field which is part of a oneof group raises an AttributeError:

message Empty {}

message Test {
  oneof foo {
    bool on = 1;
  }
  int32 i = 2;
  Empty e = 3;
}
test = Test()
print(test.i)   # 0
print(test.e)   # Empty()
print(test.on)  # AttributeError

The intention behind raising an AttributeError was to make the match-case-pattern for accessing oneof groups work, so that static analysis tools can detect errors. Following the default behavior in the oneof-case would mean to simply rollback that change. But in both cases there would be no way to access a oneof group in a type checked manner.

This is why I like the idea to fix the pattern instead of rolling back.

124C41p avatar Aug 23 '24 07:08 124C41p

Just for clarification: The pattern shown in the opening post is correctly working as intended. It is just incompatible with typical assumptions made by type checkers (e.g. fields are always set). So all four cases are practically reachable for the Python interpreter. They are just unreachable from a type checker's point of view.

124C41p avatar Aug 23 '24 07:08 124C41p

I meant the google implementation

Gobot1234 avatar Aug 24 '24 12:08 Gobot1234

Oh, I see. The google implementation gives the default value. Apparently, you are supposed to use the HasField() method to figure out whether a field is actually set:

test = Test()
print(test.on)              # False
print(test.HasField("on"))  # False

124C41p avatar Aug 24 '24 15:08 124C41p

Yeah ok. I think doing this the way you've suggested makes the most sense then

Gobot1234 avatar Aug 24 '24 15:08 Gobot1234