Usage of nameEndsWith() in conjunction with isImportedJavaElement() is investigated.

protected static boolean isMatchedAnnotation(PsiClass unit, PsiAnnotation annotation, String annotationFQName) {
String elementName = annotation.getQualifiedName();
if (nameEndsWith(annotationFQName, elementName) && unit != null) {
// For performance reason, we check if the import of annotation name is
// declared
if (isImportedJavaElement(unit, annotationFQName))
return true;
// only check fully qualified annotations
if (annotationFQName.equals(elementName)) {
PsiReference ref = annotation.getReference();
PsiElement def = ref.resolve();
if (def instanceof PsiAnnotation) {
String fqName = ((PsiAnnotation)def).getQualifiedName();
return fqName.equals(annotationFQName);
}
}
}
return false;
}

The above is the code snippet supporting the same. Both the conditions must keep aside.

• nameEndsWith provides a fast suffix check to filter out annotations with non-matching names early on.
• isImportedJavaElement checks if the annotation is already imported, allowing for an early return if it is.
• If the import isn’t available, a fully qualified name check with equals confirms that the annotation matches precisely.

By keeping nameEndsWith, we optimize for cases where there’s no match early on, ensuring only likely candidates undergo further checks. This structure provides a good balance between efficiency and readability.

@dessina-devasia I've read your comment from yesterday and saw you opened a PR to refactor the code for performance. The purpose of this issue is to determine whether AbstractDiagnosticsCollector is behaving correctly. The original design for the diagnostic collectors was not fully inclusive of qualified names and now that qualified names are being provided by all of the extensions of this class it is questionable whether the design of the base class is still correct. For what seems that should be as simple as checking whether the annotation name found in the application source is the same as the expected name, this class has very complex logic. What does it mean to check if an annotation is imported if we already have the fully resolved name? I believe the import code is trying to resolve a qualified name from a non-qualified name. but since all the names are now qualified what productive work is this code doing? Is it harmless? Is it non-sensical and producing the wrong result? That is the analysis we were looking for here. I'd suggest building some test scenarios to validate that.

@dessina-devasia If you're wondering about what to test, the scenarios are described at a high-level in this issue here (eclipse/lsp4jakarta#507). As an example, someone who specified an annotation like "ce.Entity" in their application should never receive a diagnostic about "jakarta.persistence.Entity". "jakarta.persistence.Entity" ends with "ce.Entity" but they are not the same and if somehow we're determining that they're equivalent, that would be a bug in LTI.

For better understanding, I'm just framing the issue as below:

The class AbstractDiagnosticCollector is extended by several specific diagnostic collector classes, including:

AnnotationDiagnosticsCollector
BeanValidationDiagnosticsCollector
ManagedBeanDiagnosticsCollector
DependencyInjectionDiagnosticsCollector
Jax_RSClassDiagnosticsCollector
ResourceMethodDiagnosticsCollector
JsonbDiagnosticsCollector
JsonpDiagnosticCollector
PersistenceEntityDiagnosticsCollector
PersistenceMapKeyDiagnosticsCollector
FilterDiagnosticsCollector
ListenerDiagnosticsCollector
ServletDiagnosticsCollector
WebSocketDiagnosticsCollector

The methods we are focusing on are isMatchedAnnotation() and isMatchedJavaElement(), both of which utilize nameEndsWith() in combination with isImportedJavaElement(), as noted in this comment.

Starting with isMatchedAnnotation(), this method is invoked from AnnotationDiagnosticsCollector. When I debugged this, the values of relevant variables in the code were as follows:

• annotation.getQualifiedName() returns jakarta.annotation.PostConstruct (since I've debugged PostConstructAnnotationQuickFix in AnnotationDiagnosticCollector)
• annotationFQName is jakarta.annotation.PostConstruct

Here we can see that the variables annotationFQName & annotation.getQualifiedName() are compared in nameEndsWith() as below

When isImportedJavaElement() is called, it checks again as shown in the screenshot below, indicating that this check occurs multiple times even when the fully qualified name is already obtained.

Also, @mrglavas could you please clarify on what exactly mentioned in the example

As an example, someone who specified an annotation like "ce.Entity" in their application should never receive a diagnostic about "jakarta.persistence.Entity". "jakarta.persistence.Entity" ends with "ce.Entity" but they are not the same and if somehow we're determining that they're equivalent, that would be a bug in LTI.

Is it something like instead of providing the @Entity, if we provide @ce.Entity, it won't show any diagnostics right?

Currently, it will show like below. Please correct me if I'm wrong.

Also, @mrglavas could you please clarify on what exactly mentioned in the example

As an example, someone who specified an annotation like "ce.Entity" in their application should never receive a diagnostic about "jakarta.persistence.Entity". "jakarta.persistence.Entity" ends with "ce.Entity" but they are not the same and if somehow we're determining that they're equivalent, that would be a bug in LTI.

Is it something like instead of providing the @Entity, if we provide @ce.Entity, it won't show any diagnostics right?

@dessina-devasia Yes, though a developer would normally write that using an import of the other package (import ce.Entity;). We should be testing both scenarios (imported package and direct usage of the qualified name) and ideally should be writing automated tests to ensure that these scenarios work correctly going forward.

Sure @mrglavas. Thank you for the clarification.
Let me create automated tests and will update you

Sure @mrglavas. Thank you for the clarification. Let me create automated tests and will update you

@dessina-devasia I created this PR #1081 to cover the Java built-in types to help inspire development of test annotation classes (e.g. ce.Entity) which would impersonate the official Jakarta EE annotations. The idea is that someone writing a new test for qualified names could reference the relevant imposter classes in the code in order to verify that our implementation does not incorrectly mistake them for the real ones.

@mrglavas I've raised a PR #1085 on this issue having the automation test corresponding to the AnnotationDiagnosticsCollector.

@dessina-devasia You should create separate issues for improvements to the test automation. I know you added a checklist here, but want to clarify that the expectation for this issue is to resolve issues with AbstractDiagnosticsCollector and a PR associated with this GitHub issue would contain changes to align with the current design. I would not expect that it's required to have a full set of tests (for every diagnostic collector) to complete this investigation. The focus should be on completing this task first. We can discuss prioritization of development of the complete set of automated tests as a team.

@mrglavas, Thank you for the clarification. But AbstractDiagnosticsCollector is the base class of all the collectors listed in the checklist, updates will likely be required across all of them to fully implement this. Please correct me if I'm wrong. Also, I'll investigate, whether there is a common way to handle the same as a whole.