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

[java] Name disambiguation in annotations #2665

Merged
merged 6 commits into from
Jul 30, 2020

Conversation

oowekyala
Copy link
Member

@oowekyala oowekyala commented Jul 26, 2020

Describe the PR

Implement name disambiguation for annotations. This is implemented by adding a ClassOrInterfaceType inside the annotation, so that the logic is reused. This means, ClassOrInterfaceType is used everywhere a type (or type name, not the same thing) is explicitly written (apart from import statements).

This also removes/deprecates some API we don't need anymore:

  • Annotation#getAnnotationName: annotations now have a symbol, which is much more useful and powerful than just this name
  • ClassOrInterfaceType#isAnonymousClass: this is just meaningless now
  • ClassOrInterfaceType#getImage: now always returns null. There is still getSimpleName, or better, the symbol.
  • ASTType#getTypeImage (deprecated for now): with type resolution there will be more useful ways to pretty print the type. The type image is used to implement rules right now, it would be much more robust to use symbols or subtyping checks instead, when type resolution is done.
  • ASTThrowStatement#getFirstClassOrInterfaceTypeImage: this is too specific and quite frankly, useless. The only usage was in ExceptionForFlowControl. This rule compares the name of a thrown exception with the name of a caught exception. It doesn't work unless the name is explicitly written (throw new E()). When we have full type resolution we can just use the static type of the expression. The rule is rewritten to compare symbols for now.
  • ClassOrInterfaceType#isReferenceToClassSameCompilationUnit (deprecated for now): this is used to implement a fallback when type resolution fails (because of an incomplete classpath). With the new type resolution we won't need it, as if the referenced symbol is declared in the same file, we use the AST in place of a Class transparently.

The validation of disambiguated types is stricter now (eg we report an error if you write List<A, B>)

I made sure that ExceptionForFlowControl passes tests, but this will only be true when #2664 is merged

Related issues

Ready?

  • Added unit tests for fixed bug/feature
  • Passing all unit tests
  • Complete build ./mvnw clean verify passes (checked automatically by travis)
  • Added (in-code) documentation (if needed)

@oowekyala oowekyala added this to the 7.0.0 milestone Jul 26, 2020
@pmd-test
Copy link

1 Message
📖 No java rules are changed!

Generated by 🚫 Danger

@adangel adangel self-assigned this Jul 30, 2020
Copy link
Member

@adangel adangel left a comment

Choose a reason for hiding this comment

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

I'll make the changes myself when merging.

}

fun assertNoError(t: ASTClassOrInterfaceType) {
val err = logger.errors[SemanticChecksLogger.MALFORMED_GENERIC_TYPE]?.firstOrNull { it.first == t }
Copy link
Member Author

Choose a reason for hiding this comment

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

Suggested change
val err = logger.errors[SemanticChecksLogger.MALFORMED_GENERIC_TYPE]?.firstOrNull { it.first == t }
val err = logger.errors[SemanticChecksLogger.EXPECTED_ANNOTATION_TYPE]?.firstOrNull { it.first == t }

adangel added a commit that referenced this pull request Jul 30, 2020
[java] Name disambiguation in annotations #2665
@adangel adangel merged commit d4febe7 into pmd:java-grammar Jul 30, 2020
@oowekyala oowekyala deleted the grammar-annot-disambig branch July 30, 2020 19:20
@adangel adangel mentioned this pull request Jan 23, 2023
55 tasks
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.

3 participants