[java] Similar/duplicated implementations for determining FQCN

[adangel]

As mentioned in this comment we have similar implementations for determining the fully qualified names of classes, especially for anonymous classes and local classes.

We should try to unify the implementations. On first sight, it looks like it should be part of the node (e.g. ClassOrInterfaceDeclaration). Given, that we - thanks to the metrics efforts - have a getQualifiedName() method, we probably should reuse this in the SymbolTable and TypeResolution, too.

[oowekyala]

Unifying the implementations looks like the right thing to do. Where possible, we should probably try as well to make it match the JLS about binary names you linked in the linked comment

[jsotuyod]

Actually, the method should be on ASTAnyTypeDeclaration, but the implementation would affect ASTClassOrInterfaceDeclaration, ASTAnnotationTypeDeclaration and ASTEnumDeclaration...

Avoiding duplicate code may need some refactoring of ASTAnyTypeDeclaration into an abstract class extending AbstractJavaAccessTypeNode perhaps?

[oowekyala]

@jsotuyod That's a great idea. The method isNested could be abstracted away, since the implementation is identical across subclasses, which would allow us to write getQualifiedName in that abstract class

[oowekyala]

It seems to me, like a big part of the work in ClassTypeResolver is about resolving the qualified name of some things (eg class declaration, imports) and then loading the class. I think we could use the implementation that's in JavaQualifiedName to do just that, thus relieving ClassTypeResolver from that work.

There are however some issues with how qualified names are computed for the metrics framework.

• Qualified names are computed lazily in the node, and thus the node asks for its qualified name at the time of use. This complicates the resolution of the qualified names of local and anonymous classes, since those type of classes should be indexed with a counter, in document order. However, computing qualified names lazily kills the document order: the anonymous class which will have the index 1 is the first to ask for it, not the first in the AST. The two coincide in most cases, but it's intrinsically unreliable. Anonymous or local classes could be given names that don't coincide with the .class files, which would prevent us from loading a Class with that implementation. The metrics framework doesn't care, but we couldn't be sure this implementation is correct if we used it for type resolution
• Lazy evaluation forces us to keep static maps of Integers to keep track of the counters without a document, which 1. forces us to reset the counters between tests, and 2. could waste memory.
• Lastly, I'm really not convinced lazy evaluation saves that much computing time, or space. In general, metrics rules compute the qualified names of next to every QualifiableNode they get their hands on (including CyclomaticComplexity, which may be the most popular of them)

We could, in place of on-demand evaluation, use a visitor to populate all declaration nodes with their qualified name. Once we have the qualified name, we normally can have the type quite simply (ask a ClassLoader), which means we could have the visitor populate these types too (or compute them lazily).

We could perhaps enable that visitor to populate method declarations nodes with the associated j.lang.reflect.Method, which could allow us to finally write a rule to check for missing Override annotations. We could also leave that to the remaining type resolution step.

[jsotuyod]

@oowekyala I may be wrong, but I believe the implementation in JavaQualifiedName deals exclusively with finding FQCN for classes defined in an analyzed file and explicit FQ accesses, while ClassTypeResolver attempts to get a FQCN of pretty much anything it finds when analyzing the source.

AFAIK, JavaQualifiedName can't tell Integer refers to java.lang.Integer, nor that under the presence of import java.util.* a simple List refers to java.util.List; which ClassTypeResolver can.

On the other hand, I think JavaQualifiedName is better at dealing with anon inner classes.

[oowekyala]

JavaQualifiedName deals exclusively with finding FQCN for classes defined in an analyzed file and explicit FQ accesses, while ClassTypeResolver attempts to get a FQCN of pretty much anything it finds when analyzing the source.

@jsotuyod That's true, but I don't think it's limiting. We could limit the visitor's actions to just resolve the qualified names (and types) it can resolve, ie class declarations mostly, including anonymous ones. It would be like a "preprocessing phase" for type resolution, resolving what it can, ie the low hanging fruits of type resolution.

AFAIK, JavaQualifiedName can't tell Integer refers to java.lang.Integer

That's also true, but the ability to discern between those would only be relevant to find the Method instance of the method declaration (we'd need it for formal parameters). Class declarations can be resolved anyway.

We could also make that preprocessing step resolve the type of things like imports, and explicit things like extends lists, and formal parameters, for later use of ClassTypeResolver. It would then need the ability you describe.

There are two approaches I think, which differ in the responsibilities of each visitor:

• The "just FQCNs" preprocessing:
  • Stage 1: We have a preprocessing visit which resolves the qnames (for the metrics framework) of just class and method declarations. It doesn't care about any other nodes, and doesn't carry much logic (eg doesn't resolve imports). The computed FQCNs can be used to load types by the later TR step, and are used by Metrics
  • Stage 2 (ClassTypeResolver): does everything else (including Method resolution)
• The "true" typeres preprocessing visitor:
  • Stage 1: Preprocessing visit, which resolves FQCNs, but also imports. Knowing about imports is enough to resolve everything else that's completely explicit (formal parameters, extends list, field types, Method instance, etc.)
  • Stage 2 (ClassTypeResolver): takes over where the previous visitor left off, so the imports are already parsed, and many types are already resolved. The main function of this visitor is to resolve the type of expressions, which involves much more complicated algorithms (shadowing, type inference, etc.)

[oowekyala]

This is fixed in PMD 7, we use symbols uniformly now (and metrics don't need qualified names since #2241). Refs #1207