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] CommentDefaultAccessModifierRule shows incorrect message #1440

Closed
jsotuyod opened this Issue Nov 7, 2018 · 6 comments

Comments

Projects
None yet
3 participants
@jsotuyod
Copy link
Member

jsotuyod commented Nov 7, 2018

Affects PMD Version:
6.9.0

Rule:
codestyle/CommentDefaultAccessmodifier

Description:

When reporting on a constructor, the error message is:

To avoid mistakes add a comment at the beginning of the null constructor if you want a default access modifier

It shouldn't say null, but the name of the constructor:

To avoid mistakes add a comment at the beginning of the Foo constructor if you want a default access modifier

Code Sample demonstrating the issue:

public class Foo {
    Foo() { }
}

Running PMD through: Any

@keerthikorivi

This comment has been minimized.

Copy link

keerthikorivi commented Nov 8, 2018

Hi,

Can i take up this issue?

@jsotuyod

This comment has been minimized.

Copy link
Member

jsotuyod commented Nov 8, 2018

@keerthikorivi of course you can! All PRs are welcomed

@keerthikorivi

This comment has been minimized.

Copy link

keerthikorivi commented Nov 10, 2018

Hi @jsotuyod,

I just wanted to confirm my understanding of the issue, the issue is that when we create a class say, Foo, as shown below with a default constructor and use pmd 6.9.0 in the build then it fails with the error message: To avoid mistakes add a comment at the beginning of the null constructor if you want a default access modifier
Instead, it should specify the class name instead of null

public class Foo {
Foo() { }
}

Please let me know if that's correct?

@jsotuyod

This comment has been minimized.

Copy link
Member

jsotuyod commented Nov 10, 2018

@keerthikorivi exactly.

@stationeros

This comment has been minimized.

Copy link

stationeros commented Nov 11, 2018

@jsotuyod I ran the code locally and saw the value of decl.getImage() is null.

@override
public Object visit(final ASTClassOrInterfaceDeclaration decl, final Object data) {
// check for nested classes
if (decl.isNested() && shouldReport(decl)) {
addViolationWithMessage(data, decl, String.format(MESSAGE, decl.getImage(), "nested class"));
}
return super.visit(decl, data);
}

Going through the code I was not able to figure out where does this get set actually. Can you please point me to that code snippet. This should help me debug further.

@jsotuyod

This comment has been minimized.

Copy link
Member

jsotuyod commented Nov 11, 2018

@keerthikorivi the problem lies within the grammar itself, not setting the image for the node to the <IDENTIFIER>:

void ConstructorDeclaration(int modifiers) :
{jjtThis.setModifiers(modifiers);
Token t;}
{
[ TypeParameters() ]
<IDENTIFIER> FormalParameters() [ "throws" NameList() ]

You can see how this is done for other nodes such as:

t=<IDENTIFIER> {jjtThis.setImage(t.image);} [ Arguments() ] [ ClassOrInterfaceBody() ]

stationeros pushed a commit to stationeros/pmd that referenced this issue Nov 11, 2018

@jsotuyod jsotuyod added the has:pr label Nov 12, 2018

@jsotuyod jsotuyod added this to the 6.10.0 milestone Nov 12, 2018

@adangel adangel changed the title [java] CommendDefaultAccessModifierRule shows incorrect message [java] CommentDefaultAccessModifierRule shows incorrect message Nov 21, 2018

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