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

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

Open
124C41p opened this issue Aug 22, 2024 · 7 comments
Open

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

124C41p opened this issue Aug 22, 2024 · 7 comments
Labels
bug Something isn't working

Comments

@124C41p
Copy link
Contributor

124C41p commented Aug 22, 2024

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
@Gobot1234 Gobot1234 added bug Something isn't working on hold labels Aug 22, 2024
@Gobot1234
Copy link
Collaborator

This was just changed, this seems not ideal.

@Gobot1234
Copy link
Collaborator

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
Copy link
Contributor Author

124C41p commented Aug 23, 2024

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
Copy link
Contributor Author

124C41p commented Aug 23, 2024

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
Copy link
Collaborator

I meant the google implementation

@124C41p
Copy link
Contributor Author

124C41p commented Aug 24, 2024

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 Gobot1234 removed the on hold label Aug 24, 2024
@Gobot1234
Copy link
Collaborator

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants