[pigeon] updates toString and isNullish methods#11625
[pigeon] updates toString and isNullish methods#11625tarrinneal wants to merge 2 commits intoflutter:mainfrom
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates Pigeon to version 26.4.0, introducing automatic generation of toString (or equivalent) methods for data classes across all supported languages. It also refines the Swift isNullish utility to handle nested optional values. Review feedback highlights a critical logic error in the C++ generator that results in unbalanced braces and corrupted code, along with a compilation issue when handling C++ enums. Additionally, for Swift, the feedback identifies a redundant protocol conformance error in subclasses and suggests using String(describing:) to prevent compiler warnings during string interpolation of optional fields.
| indent.writeln('ss << *$name;'); | ||
| } | ||
| }); | ||
| indent.nest(1, () { | ||
| indent.writeln('ss << "null";'); | ||
| }); | ||
| } else { | ||
| if (field.type.isClass) { | ||
| indent.writeln('ss << $name.ToString();'); | ||
| } else { | ||
| indent.writeln('ss << $name;'); |
There was a problem hiding this comment.
C++ enum class types do not support the << operator for std::ostream by default. If a data class contains an enum field, the generated ToString method will fail to compile. You should check if the field is an enum and cast it to its underlying integer type before appending it to the stream.
There was a problem hiding this comment.
Gemini is correct here. You'll also need custom handling of maps, and maybe lists.
| final Iterable<String> fieldStrings = classDefinition.fields.map(( | ||
| NamedType field, | ||
| ) { | ||
| return '${field.name}: \\(${field.name})'; |
There was a problem hiding this comment.
In Swift, string interpolation of optional values triggers a compiler warning (e.g., "Expression implicitly coerced from 'String?' to 'Any'"). It is recommended to wrap optional fields in String(describing:) to silence this warning and provide a consistent string representation.
| return '${field.name}: \\(${field.name})'; | |
| return '${field.name}: \(String(describing: ${field.name}))'; |
stuartmorgan-g
left a comment
stuartmorgan-g
left a comment
There was a problem hiding this comment.
The native unit tests should have a test that calls the to-string method for a test object and ensure that the expected string results, so that we are validating that the methods do what we expect (and also have a clear snapshot of what the expected output actually is).
| indent.writeln('ss << *$name;'); | ||
| } | ||
| }); | ||
| indent.nest(1, () { | ||
| indent.writeln('ss << "null";'); | ||
| }); | ||
| } else { | ||
| if (field.type.isClass) { | ||
| indent.writeln('ss << $name.ToString();'); | ||
| } else { | ||
| indent.writeln('ss << $name;'); |
There was a problem hiding this comment.
Gemini is correct here. You'll also need custom handling of maps, and maybe lists.
|
|
||
| _writeFunctionDefinition( | ||
| indent, | ||
| 'ToString', |
There was a problem hiding this comment.
The idiomatic C++ way to do this is to override operator<<; see this discussion for instance.
| final Iterable<String> fieldStrings = fields.map((NamedType field) { | ||
| return '${field.name}: \$${field.name}'; | ||
| }); | ||
| final String fieldsConcat = fieldStrings.join(', '); |
There was a problem hiding this comment.
Nit: Consider just inlining this as ${fieldStrings.join(', ')} since it's barely longer than the variable name.
| }); | ||
| } else if (_isNumericListType(field.type)) { | ||
| indent.writeln( | ||
| 'g_string_append_printf(str, "[...], length: %zu", self->${fieldName}_length);', |
There was a problem hiding this comment.
Why is the representation of a number list [...] rather than the list of the numbers?
| NamedType field, | ||
| ) { | ||
| if (_usesPrimitive(field.type)) { | ||
| return '@(self.${field.name})'; |
There was a problem hiding this comment.
FWIW it would be conceptually cleaner to use the right format string for each primitive type, rather than boxing them just to print them as objects. But since this is just a nice-to-have utility method, and Obj-C isn't a development focus, it's fine to leave as-is.
| g_string_append(str, ", list_map: "); | ||
| g_string_append(str, "..."); | ||
| g_string_append(str, ", map_map: "); | ||
| g_string_append(str, "..."); |
There was a problem hiding this comment.
These should also not all be ...s.
2 participants
prequel pr to #11352 to land non NI changes to simplify pr.