danielgtaylor / python-betterproto

Clean, modern, Python 3.6+ code generator & library for Protobuf 3 and async gRPC
MIT License
1.56k stars 218 forks source link

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

Open 124C41p opened 3 months ago

124C41p commented 3 months ago

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

Gobot1234 commented 3 months ago

This was just changed, this seems not ideal.

Gobot1234 commented 3 months ago

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.

124C41p commented 3 months ago

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 commented 3 months ago

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.

Gobot1234 commented 3 months ago

I meant the google implementation

124C41p commented 3 months ago

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
Gobot1234 commented 3 months ago

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