[pigeon] updates toString and isNullish methods#11625
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.
| 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.
There was a problem hiding this comment.
lmk if my change is what you had in mind.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request implements toString (or equivalent) method generation for data classes across all supported Pigeon generators, including Dart, Java, Kotlin, Swift, Objective-C, C++, and GObject. It also updates the Swift isNullish helper to correctly handle double-nested Any? values and adds new list-based echo methods to the core integration tests. Feedback identifies a missing nullability check for boolean fields in the GObject generator and potential integer truncation issues in the Objective-C description implementation on 32-bit platforms. Additionally, a suggestion was made to maintain field name consistency in GObject string representations.
stuartmorgan-g
left a comment
stuartmorgan-g
left a comment
There was a problem hiding this comment.
Re-review in progress; here are the partial notes. Also, there are some real CI failures.
stuartmorgan-g
left a comment
stuartmorgan-g
left a comment
There was a problem hiding this comment.
Remaining comments below. Generally looks good!
| return 'self.${field.name}'; | ||
| }); | ||
|
|
||
| final args = fieldValues.isEmpty ? '' : ', ${fieldValues.join(', ')}'; |
There was a problem hiding this comment.
This conditional shouldn't be needed.
There was a problem hiding this comment.
the comma isn't needed? (or isn't needed to be not there)
|
@stuartmorgan-g I think I've covered all your comments (waiting for ci to tell me if I missed anything build related), but I don't really remember since I did most of this work 2 weeks ago. |
|
This pull request is not mergeable in its current state, likely because of a merge conflict. Pre-submit CI jobs were not triggered. Pushing a new commit to this branch that resolves the issue will result in pre-submit jobs being scheduled. |
2 participants
prequel pr to #11352 to land non NI changes to simplify pr.