[android] Check AppCompatActivity classes with CallSuperFirst and CallSuperLast rules

[maksim-m]

Check AppCompatActivity classes (activities that use the support library) with CallSuperFirst and CallSuperLast rules

[jsotuyod]

Thanks for your contribution. This seems great at first sight. I'll add to my review backlog.

[jsotuyod]

Just one comment though, the rule would work just fine as is with AppCompatActivity and FragmentActivity provided you run pmd with a complete auxclasspath. Type resolution can detect types even if extension is indirect. We can add a bunch of these cases to help people not configuring auxclasspath properly, but it won't handle indirect extension unless you use auxclasspath (A extends B, which extends AppCompatActivity).

To help in setting up the auxclasspath properly, I suggest you use / take a look at this gradle plugin

[maksim-m]

@jsotuyod thanks for the reply. I'll take a look at the plugin.

[jsotuyod]

I'm on two minds regarding this. We could merge this and even add the case for FragmentActivity and people would see less False Negatives regardless of configuration, but I feel that would be hiding the real issue, which is the lack of / misconfigured auxclasspath. This is something I've questioned in another ocassion

I am certain we need to be much more verbose regarding this, maybe even go as far as including missing classes on reports to help people figure out what is missing exactly, just as FindBugs does, although that may produce some misleading messages with generics (we would need to asses / improve that), but even if so, it would still be better than the current situation where people can't even tell there is something amiss in their setup.

Both the maven plugin and Gradle configure auxclasspath properly out of the box. Android Gradle projects (en even Java ones running old Gradle versions) can use my gradle plugin to have it working and properly configured out of the box. Maybe it's time to just push for it. What do you think @adangel ?

[adangel]

@jsotuyod Regarding auxclasspath - I think, this is the direction, we should be heading towards: Making a properly configured auxclasspath a requirement for using rules which need typeresolution. At the moment these rules are still executed, but most types are null then - which leads to false positives/negatives.
In 5.6.0 we can change the behavior slightly - if no auxclasspath is configured, we disable all rules, that need typeresolution and in that case, we can print out a warning (or even fail hard...).
You're right, maven and gradle already configure this correctly and execute PMD in a phase, where we have the compiled classes available. For ant users, we need to provide better documentation on typeresolution, auxclasspath and how to setup PMD for a project.
I recently read an older comment about typeresolution: the typresolution ruleset was intended to be temporarily and eventually the rules should replace the existing rules (e.g. CloneMethodMustImplementCloneable).
I assume, it is a bit more tricky to detect an incomplete/wrong auxclasspath setting - but it should still be possible: E.g. we could assume, that we need to be able to resolve all references. If we find any type to be null after the type resolution was done for a compilation unit, we could stop PMD and output the unknown class name.

[jsotuyod]

E.g. we could assume, that we need to be able to resolve all references. If we find any type to be null after the type resolution was done for a compilation unit, we could stop PMD and output the unknown class name.

@adangel I'd not implement it this way. Such a fail fast approach would produce a harder to fix scenario such as "hey, you missed junit", "now you are missing apache commons", "hey, do you know where is guava?". I'd personally favor a single failure with all missing classes.

I'm in for disabling type resolution rules when auxclasspath is not configured, though we may consider a hard fail on such scenarios (misconfigured). I believe right now most people are falling into this scenario rather than missing classes.

Finally, we certainly need to improve the documentation on the type resolution rules. We don't even mention auxclasspath right now https://pmd.github.io/pmd-5.5.2/pmd-java/rules/index.html#Type_Resolution

Finally killing the type resolution ruleset would be awesome

As is, I'd probably reject this PR, and create some tasks to address these 3 points on their owm (documentation, missing auxclasspath with typeresolution, missing classes).

I'd suggest the changes being impacted on 5.6.0 only aince they would be breaking changes.

[maksim-m]

Thanks for the discussion. I close the PR.

[jsotuyod]

@maksim-m thank you for bringing this up! It was certainly a useful discussion, and I'll be working to address these items.

I hope the Gradle plugin I pointed to you has helped.