Simplify check for Google::Protobuf::Map type - redux
#1541
Conversation
paracycle
force-pushed
the
uk-protobuf-fix-again
branch
2 times, most recently
from
e1dbe63
to
6e60346
Compare
| @@ -78,7 +78,7 @@ class Field < T::Struct | |||
|
|
|||
| extend T::Sig | |||
|
|
|||
| ConstantType = type_member { { fixed: Module } } | |||
| ConstantType = type_member { { fixed: T::Class[T.anything] } } | |||
There was a problem hiding this comment.
Wait, could this cause problems on applications using a Sorbet version older than sorbet/sorbet#6781?
There was a problem hiding this comment.
|
Another thing to add would be that the Ruby protobuf implementation has an open PR to expose the |
| @@ -206,7 +206,7 @@ def type_of(descriptor) | |||
| # > field, even if it was not defined in the enum. | |||
| "T.any(Symbol, Integer)" | |||
| when :message | |||
| descriptor.subtype.msgclass.name | |||
| descriptor.subtype.msgclass.name || "T.untyped" | |||
There was a problem hiding this comment.
There is a name in the submsg_name. I think that we should add that before the T.untyped.
There was a problem hiding this comment.
No, that returns the name of the protobuf message, not the class name; I was wrong about that in the previous change.
For example, in the binary serialized test, if we hadn't assigned the MyCart.Progress message to a constant, its subtype.msgclass.name would be nil, but it would return MyCart.Progress from submsg_name, which is not a valid constant name.
There was a problem hiding this comment.
Right! Would MyCart::Progress be a valid constant in that case though?
There was a problem hiding this comment.
No, it wouldn't since there is no named constant defined in Ruby that corresponds to that msgclass. That constant is anonymous, so we can't refer to it.
If the `msgclass` of a subtype is not assigned to a constant, then its `#name` method would return `nil` and the compiler would return it as the type name of the descriptor. Not only does this break the contract of the `type_of` method defined by its signature which promises to always return a `String` instance, but it also leads to us generating broken RBI files, since these `nil` values serialize to empty strings. This commit fixes this by mapping the message class to `T.untyped` if it does not have a name.
The realization is that you can't assign an array type to `Map` fields and that you can't assign a hash type to `Repeated` fields. So we can use that to detect the type of the field. We try to instantiate a new instance of the constant type, setting the value of the current descriptor field to an empty hash. If the type is `Map`, then it will succeed. If it's `Repeated`, it will fail.
paracycle
force-pushed
the
uk-protobuf-fix-again
branch
from
6e60346
to
011b826
Compare
Motivation
Fix #1533
Implementation
The realization is that you can't assign an array type to
Mapfields and that you can't assign a hash type toRepeatedfields. So we can use that to detect the type of the field.We try to instantiate a new instance of the constant type, setting the value of the current descriptor field to an empty hash. If the type is
Map, then it will succeed. If it'sRepeated, it will fail.Another small change in this PR makes it so that when we fail to read the type of the descriptor for repeated/map fields, we won't end up generating invalid RBI files, due to the message class not having a name.
Tests
Added failing tests that pass after the changes.