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] Use typeof in MissingSerialVersionUID #1258

Merged
merged 2 commits into from Jul 30, 2018

Conversation

Projects
None yet
3 participants
@krichter722
Contributor

krichter722 commented Jul 26, 2018

Before submitting a PR, please check that:

  • The PR is submitted against master. The PMD team will merge back to support branches as needed.
  • ./mvnw test passes.
  • ./mvnw pmd:check passes.
  • ./mvnw checkstyle:check passes. Check this for more info

PR Description:
I'm not 100% sure about the following, however #1078 is open since May and @jsotuyod suggested the solution which I hope I understood correctly.

The construct

count(ImplementsList
[ClassOrInterfaceType/@Image='Serializable'
or ClassOrInterfaceType/@Image='java.io.Serializable']) =1

doesn't cover subclasses of a class or interface implementing or
extending Serializable whereas the typeof operator does.

close #1078

Use typeof in MissingSerialVersionUID
The construct

```
count(ImplementsList
[ClassOrInterfaceType/@image='Serializable'
or ClassOrInterfaceType/@image='java.io.Serializable']) =1
```

doesn't cover subclasses of a class or interface implementing or
extending Serializable whereas the `typeof` operator does.

close #1078

@jsotuyod jsotuyod added this to the 6.6.0 milestone Jul 26, 2018

@jsotuyod

This comment has been minimized.

Member

jsotuyod commented Jul 26, 2018

@krichter722 thanks for the PR! This is effectively good.

However, typeof has been deprecated since PMD 6.4.0. A new simpler typeIs function supercedes it

A test case should be added too. Doing so would require:

  1. Add the base class implementing Serializable as a nested class in https://github.com/pmd/pmd/blob/master/pmd-java/src/test/java/net/sourceforge/pmd/lang/java/rule/errorprone/ErrorProneRulesTest.java (with a comment saying it's used to test this scenario)
  2. Add the test case importing the base class from point 1 in https://github.com/pmd/pmd/blob/master/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/errorprone/xml/MissingSerialVersionUID.xml

Given that, this is good to merge, and may make it in time for 6.6.0

count(ImplementsList
[ClassOrInterfaceType/@Image='Serializable'
or ClassOrInterfaceType/@Image='java.io.Serializable']) =1
//ClassOrInterfaceType[typeof(@Image, 'java.io.Serializable','Serializable')]

This comment has been minimized.

@jsotuyod

jsotuyod Jul 26, 2018

Member

careful here, since you are looking everywhere, not just in implements / extends, this can produce FPs with nested classes

@adangel adangel modified the milestones: 6.6.0, 6.7.0 Jul 28, 2018

@krichter722

This comment has been minimized.

Contributor

krichter722 commented Jul 29, 2018

I started this PR because I'm missing the feature and thought it was as easy replacing the count with typeof (based on the comments in the referenced issue). I do not understand the underlying principle and don't have the resources to learn them. Please edit this PR with the solution or post it as comment as it's not reasonable to figure it out for myself.

@adangel

This comment has been minimized.

Member

adangel commented Jul 29, 2018

Hi @krichter722, thanks for letting us know.
I've pushed a new commit, which

  • adds the test case
  • uses typeIs
  • changed the xpath expression slightly, to only look below implements/extends
  • enable typeResolution for the rule
@krichter722

This comment has been minimized.

Contributor

krichter722 commented Jul 29, 2018

@adangel Thank you so much! I can delete my commit or squash the two if you want or you make the commit on master and we close this PR (after all it's your work).

@jsotuyod jsotuyod merged commit 582218b into pmd:master Jul 30, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

jsotuyod added a commit that referenced this pull request Jul 30, 2018

@krichter722 krichter722 deleted the krichter722:fix-1078 branch Jul 30, 2018

adangel added a commit that referenced this pull request Aug 7, 2018

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