-
Notifications
You must be signed in to change notification settings - Fork 7
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
Fully qualify types in generic synthetic method return types #341
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.
Overall looks great, but I have one concern that needs to be addressed.
I added another test case, as you suggested. For the expected output I put the incorrect one, but I changed the check compilation scripts to ignore the compilation error for this case. |
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 don't need to review again after you make the requested changes to the wording of a few comments. Nice work :)
* This test checks that Specimin includes the fully qualified class name in generics when | ||
* generating a synthetic method. For example, if a return type is Bar<com.foo.Foo>, the actual | ||
* synthetic return type should be com.qualified.Bar<com.foo.Foo>. At this moment, we should expect | ||
* this test case to fail. |
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.
Rather than "we should expect this test to fail", I would phrase this as "this test encodes Specimin's current behavior, which doesn't produce compilable output. If you break this test, it might not be a bad thing."
// TODO: handle already fully qualified generic type arguments. Right now JavaTypeCorrect | ||
// only outputs the simple class name, so there is no way to get its fully qualified name | ||
// here without ambiguity (i.e. org.example.Foo and com.example.Foo are different signatures) | ||
// with the same simple class name |
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.
Add a reference to the test that demonstrates the "bad" behavior to this comment.
Thanks :) Just changed the comments. |
Previously, when a synthetic method with a generic return type had a type parameter in the same package as the target method, it would not keep its fully qualified name in its synthetic definition, leading to compilation errors seen in na-791b.
For example,
ImmutableSet.copyOf
returned anImmutableSet<Feature>
, but in its synthetic definition,copyOf
returnedcom.google.common.collect.ImmutableSet<Feature>
instead ofcom.google.common.collect.ImmutableSet<com.github.benmanes.caffeine.cache.Feature>
.