Fixed AbstractTypeAwareCheck for generics in interfaces (issue #473) #177

Merged
merged 1 commit into from Jul 7, 2014

3 participants

@tobiasbaum
Contributor

AbstractTypeAwareCheck did not take generic parameters in interface definitions into account, which seems to be the cause for (sourceforge) issue #473.

It seems to me that the additional "TokenType" must be added in every subclass of AbstractTypeAwareCheck. I fixed it for the standard checks but naturally couldn't fix it for custom checks "out in the wild". Perhaps Check should somehow be refactored to make it easier for the "default tokens" to be the union of the needed tokens of all classes in the check's supertype hierarchy.

@coveralls

Coverage Status

Coverage increased (+0.0%) when pulling 7dc94ba on tobiasbaum:master into 55c2560 on checkstyle:master.

@romani romani merged commit 7dc94ba into checkstyle:master Jul 7, 2014

1 check passed

continuous-integration/travis-ci The Travis CI build passed
Details
@romani
Member
romani commented Jul 7, 2014

merged, but ...

I have a bad news for you, that Check and all other Checks that are TypeAwareCheck will be deprecated in next releases.
Reason: implementation of that feature is now full and too error prone. I would suggest to use similar validations in FindBug or PMD project if any or request to implement it.

@tobiasbaum
Contributor

Adding checks to FindBugs is much harder, and some checks (like Javadoc) are not possible because FindBugs relies on the bytecode only. I have no experience with PMD because so far the combination of FindBugs and Checkstyle satisfied my needs.

I always thought the "syntax tree + bytecode for type infos" idea of Checkstyle was a quite elegant solution for writing powerful checks without the need to re-implement too much of the compiler's workings. Why has it become too complicated? Are some new features in Java 8 causing problems?

@romani
Member
romani commented Jul 7, 2014

The main idea of Checkstyle is validation base on one file, Checkstyle does not aware of types and hierarchy of classes. All Checkstyle have is a parsing tree of tokens (String).

That idea make it very easy to create new Checks, but make it impossible to validate more complicated cases (too much false positives appear).

Right now we rely on class-path to get information about other class. But class-path is runtime known structure, it could be different for UTs and Runtime. Right now we have two checks that try to use class-path - both of them problematic. Moreover, there is complain from Sonar project that that kind of implementation base on class-path cause problems with memory lick.
AbstractTypeAwareCheck - was attempt to cover cross-file validation, as right now we do not have enough developers to make it stable, I would rather move it to staging area.

So, for now I would rather remove that Checks from Checkstyle, resolve more acute problems in Checkstyle and then come back to stable class-path implementation as we have time as human resources.

in 5.8 this Check will stay in Checkstyle - I can not remove/ignore your fix :) , most likely it will be removed in next release, so you will have option to stay on 5.8 till you think it is reasonable.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment