Fix an issue with parcelable code gen with sets and no lists #434
Fix an issue with parcelable code gen with sets and no lists #434
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this solution is incomplete. See my inline comment.
@@ -249,6 +249,35 @@ class JavaGenerator(spec: Spec) extends Generator(spec) { | |||
val javaName = if (r.ext.java) (ident.name + "_base") else ident.name | |||
val javaFinal = if (!r.ext.java && spec.javaUseFinalForRecord) "final " else "" | |||
|
|||
def checkIfRecordContainsSets(r: Record): Boolean = { | |||
for (f <- r.fields) { | |||
f.ty.resolved.base match { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This fix addresses the specific test case, but doesn't go deep enough. E.g. if the record contains an optional<set> this function won't find it. I tried such a record and was able to see the Java compile fail. I think you need to recurse into the args as well as looking at the base type.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"I think you need to recurse into the args as well as looking at the base type."
Can you elaborate a bit more on this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
r.args
contains the type args. So for instance with optional<set<string>>
r.base
would be MOptional
, while r.args
would contain one item whose base
would be MSet
. I think you need to search for MSet
in the args recursively to find cases like that, or cases like list<set<string>>
, etc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I still don't think this is sufficient. I was using optional<> as an example, not the only relevant type. You'd have the same problem with map<string, set>, for instance.
I don't think you need to hard-code any specific set of types. Just always recurse by calling this same function on each element of f.ty.resolved.args
, and combine the results with logical or.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@artwyman The parcelable generated code doesn't need to deal with nested collections.
Optional<>
was a corner case that I didn't remembered.
But a set<strings>
is the same as a set<list<string>>
or a set<map<string, string>>
, and the same applies to the other collections, the optional<>
was the only exception.
Check the unit tests to see what I'm referring to.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, I see, what's needed here isn't a full recursive processing of the type but only one level. I assumed recursive, and took the optional<set<string>>
case as an indication that theory held true. I just thought about optional<optional<set<string>>>
now, but we explicitly disallow direct nesting of optionals.
It's surprising to that writeToParcel
of map<string, set<string>>
doesn't need to use writeList
in the same way that it does for set<string>
. But Thanks for adding the additional test cases to demonstrate that all of those cases work.
@artwyman all the issues are now fixed |
@@ -249,6 +249,35 @@ class JavaGenerator(spec: Spec) extends Generator(spec) { | |||
val javaName = if (r.ext.java) (ident.name + "_base") else ident.name | |||
val javaFinal = if (!r.ext.java && spec.javaUseFinalForRecord) "final " else "" | |||
|
|||
def checkIfRecordContainsSets(r: Record): Boolean = { | |||
for (f <- r.fields) { | |||
f.ty.resolved.base match { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I still don't think this is sufficient. I was using optional<> as an example, not the only relevant type. You'd have the same problem with map<string, set>, for instance.
I don't think you need to hard-code any specific set of types. Just always recurse by calling this same function on each element of f.ty.resolved.args
, and combine the results with logical or.
@artwyman I also added more unit tests to the parcelable feature to ensure more code coverage to the feature. |
@@ -249,6 +249,35 @@ class JavaGenerator(spec: Spec) extends Generator(spec) { | |||
val javaName = if (r.ext.java) (ident.name + "_base") else ident.name | |||
val javaFinal = if (!r.ext.java && spec.javaUseFinalForRecord) "final " else "" | |||
|
|||
def checkIfRecordContainsSets(r: Record): Boolean = { | |||
for (f <- r.fields) { | |||
f.ty.resolved.base match { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, I see, what's needed here isn't a full recursive processing of the type but only one level. I assumed recursive, and took the optional<set<string>>
case as an indication that theory held true. I just thought about optional<optional<set<string>>>
now, but we explicitly disallow direct nesting of optionals.
It's surprising to that writeToParcel
of map<string, set<string>>
doesn't need to use writeList
in the same way that it does for set<string>
. But Thanks for adding the additional test cases to demonstrate that all of those cases work.
This PR fixes #408.
I added some conditions to only add the import of
ArrayList
when necessary, avoiding the import duplication.