Skip to content
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

IllegalType should ignore parameters of methods with modifiers not in 'memberModifiers' #6123

Closed
ebruneton opened this issue Sep 16, 2018 · 5 comments
Milestone

Comments

@ebruneton
Copy link
Contributor

/var/tmp $ javac TestClass.java
/var/tmp $ cat config.xml
<?xml version="1.0"?>
<!DOCTYPE module PUBLIC
          "-//Puppy Crawl//DTD Check Configuration 1.3//EN"
          "http://www.puppycrawl.com/dtds/configuration_1_3.dtd">
<module name = "Checker">
  <module name="TreeWalker">
    <module name="IllegalType">
      <property name="memberModifiers" value="LITERAL_PUBLIC"/>
    </module>
  </module>
</module>

/var/tmp $ cat TestClass.java
import java.util.HashSet;
class TestClass {
  private void m(HashSet<String> param) {}
}

/var/tmp $ RUN_LOCALE="-Duser.language=en -Duser.country=US"
/var/tmp $ java $RUN_LOCALE -jar checkstyle-8.12-all.jar -c config.xml TestClass.java
Starting audit...
[ERROR] TestClass.java:3:18: Declaring variables, return values or parameters of type 'HashSet' is not allowed. [IllegalType]
Audit done.
Checkstyle ends with 1 errors.

I would expect no error, since method 'm' is not public, and the check is configured for LITERAL_PUBLIC. The code seems to implement this logic (

), but this is somehow not working?

@rnveach
Copy link
Member

rnveach commented Sep 16, 2018

isCheckedMethod

As the code shows that method only checks ignored method names. The method you are looking for is isVerifiable and isContainVerifiableType.

I am not seeing what the issue is but it looks valid to me.

@ebruneton
Copy link
Contributor Author

Indeed, I missed that isCheckedMethod() does not check for 'memberModifiers'. So I think the line I pointed out should be && isCheckedMethod(grandParentAST) && isVerifiable(grandParentAST). It seems to me that when you want to filter out some methods with memberModifiers, this should filter out their parameters too (as does ignoredMethodNames).

@rnveach
Copy link
Member

rnveach commented Sep 16, 2018

this should filter out their parameters too

Now I see it, isVerifiable is never called for parameters.

private void visitParameterDef(DetailAST parameterDef) {

This is why violations for them aren't hidden.

@romani Please confirm this issue and if memberModifiers should apply to parameters.

@romani
Copy link
Member

romani commented Oct 12, 2018

Sorry for delay, issue is valid.

@ebruneton , please be welcome with PR to fix this issue.

rnveach pushed a commit to rnveach/checkstyle that referenced this issue Nov 3, 2018
@romani romani added the bug label Nov 9, 2018
@romani romani added this to the 8.15 milestone Nov 9, 2018
@romani
Copy link
Member

romani commented Nov 9, 2018

fix is merged.
special thank to @rnveach to help in PR to make it sooner.

@romani romani closed this as completed Nov 9, 2018
tsunghanjacktsai pushed a commit to tsunghanjacktsai/checkstyle that referenced this issue Dec 3, 2018
tsunghanjacktsai pushed a commit to tsunghanjacktsai/checkstyle that referenced this issue Dec 3, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants