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

Only clean when the annotated type mirror is type variable #787

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

Ao-senXiong
Copy link
Member

Fixes #786

Copy link
Member

@wmdietl wmdietl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add the test case from the issue?
Can you make that test case self-contained, so that it doesn't depend on the CF classes?

@@ -51,6 +51,7 @@ void use(
NoExplicitAnnotationsSub2 sub2,
NoExplicitAnnotationsSub3 sub3) {
nble = sub1.method1();
// :: error: (assignment.type.incompatible)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you provide a reason why this new error is desirable? Why is this change in behavior an improvement?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also note that a wilcard jtreg test fails.

Copy link
Member Author

@Ao-senXiong Ao-senXiong Jun 21, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you provide a reason why this new error is desirable? Why is this change in behavior an improvement?

Because nn is @Nonnull and method1() is @Nullable. So there should be an assignment incompatiable error.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But method1 is in the stub file without annotations.
Shouldn't the logic be that, if something appears in the stub-file, those annotations are used. If there are no annotations in the stub-file, the usual defaults are used.
That's at least what the current behavior seems to be. This makes the stub-files readable and understandable without looking at what the corresponding source says.

@Ao-senXiong
Copy link
Member Author

Ao-senXiong commented Jun 21, 2024

Can you add the test case from the issue? Can you make that test case self-contained, so that it doesn't depend on the CF classes?

Simplified version. Also updated in the issue.

import javax.lang.model.element.TypeElement;

public class Test {
    void test(TypeElement element) {
        element.asType();
    }
}

@Ao-senXiong
Copy link
Member Author

Interesting. This crash does not happen under java 17 and happens under Java 8 and Java 11.

@wmdietl
Copy link
Member

wmdietl commented Jun 21, 2024

Simplified version. Also updated in the issue.

It would be nicer if you just had a class in the test case instead of depending on TypeElement#asType. Now to understand the test, one has to look at the annotated JDK and try to see what annotations are on that method.

@wmdietl
Copy link
Member

wmdietl commented Jun 21, 2024

Simplified version. Also updated in the issue.

It would be nicer if you just had a class in the test case instead of depending on TypeElement#asType. Now to understand the test, one has to look at the annotated JDK and try to see what annotations are on that method.

You only updated the test in the issue. You should add test cases to this PR to ensure the issue is actually fixed.

@Ao-senXiong
Copy link
Member Author

Simplified version. Also updated in the issue.

It would be nicer if you just had a class in the test case instead of depending on TypeElement#asType. Now to understand the test, one has to look at the annotated JDK and try to see what annotations are on that method.

You only updated the test in the issue. You should add test cases to this PR to ensure the issue is actually fixed.

Okay, I will add it as I find the minial test case without TypeElement#asType dependency.

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.

ApplyFakeOverrides erase the annotation of return type
2 participants