Skip to content
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

Merged
merged 8 commits into from
Jul 30, 2024

Conversation

theron-wang
Copy link
Collaborator

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 an ImmutableSet<Feature>, but in its synthetic definition, copyOf returned com.google.common.collect.ImmutableSet<Feature> instead of com.google.common.collect.ImmutableSet<com.github.benmanes.caffeine.cache.Feature>.

@theron-wang theron-wang requested a review from kelloggm July 29, 2024 19:04
Copy link
Collaborator

@kelloggm kelloggm left a 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.

@theron-wang theron-wang requested a review from kelloggm July 30, 2024 16:13
@theron-wang
Copy link
Collaborator Author

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.

Copy link
Collaborator

@kelloggm kelloggm left a 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.
Copy link
Collaborator

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
Copy link
Collaborator

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.

@theron-wang
Copy link
Collaborator Author

Thanks :) Just changed the comments.

@kelloggm kelloggm merged commit 229a5b5 into njit-jerse:main Jul 30, 2024
2 checks passed
@theron-wang theron-wang deleted the method-generic-type branch August 1, 2024 17:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants