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

[apex] ClassCastException caused by Javadoc #1396

Closed
danbrycefairsailcom opened this Issue Oct 17, 2018 · 7 comments

Comments

Projects
None yet
5 participants
@danbrycefairsailcom

danbrycefairsailcom commented Oct 17, 2018

Affects PMD Version: 6.8.0

Rules:
ExcessiveParameterList
ExcessivePublicCount
ExcessiveClassLength
NcssConstructorCount
NcssMethodCount
NcssTypeCount

Possibly: ExcessiveClassLength
Possible all the inherit from AbstractStatisticalApexRule

Description: ClassCastException occurs when running pmd.bat against Apex files containing Javadoc comments.

java.lang.ClassCastException: net.sourceforge.pmd.lang.apex.ast.ASTFormalComment cannot be cast to net.sourceforge.pmd.lang.apex.ast.ApexNode
	at net.sourceforge.pmd.lang.apex.rule.design.ExcessiveNodeCountRule.visit(ExcessiveNodeCountRule.java:42)
	at net.sourceforge.pmd.lang.apex.rule.AbstractApexRule.visit(AbstractApexRule.java:157)
	at net.sourceforge.pmd.lang.apex.ast.ASTMethod.jjtAccept(ASTMethod.java:22)
	at net.sourceforge.pmd.lang.apex.rule.design.ExcessiveNodeCountRule.visit(ExcessiveNodeCountRule.java:42)
	at net.sourceforge.pmd.lang.apex.rule.AbstractApexRule.visit(AbstractApexRule.java:162)
	at net.sourceforge.pmd.lang.apex.rule.AbstractApexRule.visitAll(AbstractApexRule.java:134)
	at net.sourceforge.pmd.lang.apex.rule.AbstractApexRule.apply(AbstractApexRule.java:128)
	at net.sourceforge.pmd.lang.apex.rule.AbstractStatisticalApexRule.apply(AbstractStatisticalApexRule.java:31)
	at net.sourceforge.pmd.lang.rule.AbstractDelegateRule.apply(AbstractDelegateRule.java:336)
	at net.sourceforge.pmd.RuleSet.apply(RuleSet.java:499)
	at net.sourceforge.pmd.RuleSets.apply(RuleSets.java:143)
	at net.sourceforge.pmd.SourceCodeProcessor.processSource(SourceCodeProcessor.java:184)
	at net.sourceforge.pmd.SourceCodeProcessor.processSourceCode(SourceCodeProcessor.java:96)
	at net.sourceforge.pmd.SourceCodeProcessor.processSourceCode(SourceCodeProcessor.java:51)
	at net.sourceforge.pmd.processor.PmdRunnable.call(PmdRunnable.java:78)
	at net.sourceforge.pmd.processor.PmdRunnable.call(PmdRunnable.java:24)
	at java.util.concurrent.FutureTask.run(Unknown Source)
	at java.util.concurrent.Executors$RunnableAdapter.call(Unknown Source)
	at java.util.concurrent.FutureTask.run(Unknown Source)
	at java.util.concurrent.ThreadPoolExecutor.runWorker(Unknown Source)
	at java.util.concurrent.ThreadPoolExecutor$Worker.run(Unknown Source)
	at java.lang.Thread.run(Unknown Source)

Code Sample demonstrating the issue:

public class SomeClass {

    /**
     * Comment
     */
    public void doSomething() {
    	System.debug("hello world");
    }

}

Running PMD through: CLI

Windows 10
java version "1.8.0_181"

@rsoesemann

This comment has been minimized.

Contributor

rsoesemann commented Oct 17, 2018

@jeffhube can this be a problem introduced by you pull request #1314?

@danbrycefairsailcom

This comment has been minimized.

danbrycefairsailcom commented Oct 17, 2018

I should mention that I wasn't using the ApexDoc rule.

@jeffhube

This comment has been minimized.

Contributor

jeffhube commented Oct 17, 2018

@rsoesemann It looks like it, though I'm not sure how I didn't run into it during testing.

I think we could just make jjtAccept() public on AbstractApexNodeBase and change a few of the casts from ApexNode<?> to AbstractApexNodeBase like so: master...jeffhube:cce

Though I'm wondering if we should also change the visit() methods in AbstractApexRule.java to delegate to visit(AbstractApexNodeBase, Object) instead of visit(ApexNode<?>, Object), and whether visit(ApexNode<?>, Object) should be removed entirely?

@JeffMTI

This comment has been minimized.

JeffMTI commented Oct 17, 2018

Adding my details from the other Issue here for tracking:
CLI on
Ubuntu 18.04.1
Java 1.8.0_171

adangel added a commit to adangel/pmd that referenced this issue Oct 19, 2018

[apex] ClassCastException caused by Javadoc
* Add test cases for the affected rules
* refs pmd#1396
@adangel

This comment has been minimized.

Member

adangel commented Oct 19, 2018

Hi @jeffhube ,
I've added some tests to reproduce the issue here: master...adangel:issue-1396
With the changes from your branch it almost works. There is one change necessary: You added the visit method for ASTFormalComment in AbstractNcssCountRule. However, we also need this visit method in ExcessiveParameterList and ExcessivePublicCount. So we could maybe move this method up to the same base class "AbstractStatisticalApexRule"?

Though I'm wondering if we should also change the visit() methods in AbstractApexRule.java to
delegate to visit(AbstractApexNodeBase, Object) instead of visit(ApexNode, Object), and whether visit(ApexNode, Object) should be removed entirely?

For the problem at hand, it doesn't make any difference. We could delegate always to visit(AbstractApexNodeBase, Object), which looks nicer and more consistent.
Removing visit(ApexNode<?>, Object) is however not an option, since this is part of the API of the visitor.

Let me know, if you want to create a PR with your patch or whether I can just take your fix and integrate it - either way is fine.

Thanks,
Andreas

@jeffhube

This comment has been minimized.

Contributor

jeffhube commented Oct 22, 2018

So the reason I brought up changing the visitor here is because it seems that the rules fail because while visit(ApexNode<?>, Object) used to be called for every node, that is not the case now, since ASTFormalComment is not an ApexNode<?>. One way to work around that is to implement visit(ASTFormalComment, Object) as I had for AbstractNcssCountRule, but I am thinking that we may be better off getting back to having a visit method that is called for every node, which would be visit(AbstractApexNodeBase, Object), and then AbstractNcssCountRule and ExcessiveNodeCountRule would override that instead of visit(ApexNode<?>, Object), and the override of visit(ASTFormalComment, Object) that I added would be removed.

The options I see for doing so would be:

  1. Change all visit() methods in AbstractApexRule to delegate to visit(AbstractApexNodeBase, Object). Replace all former usages of visit(ApexNode<?>, Object) with visit(AbstractApexNodeBase), since the former will no longer be invoked for every node. At this point we might as well remove visit(ApexNode<?>, Object) from AbstractApexRule and the visitor interface entirely.

  2. Change only visit(ApexNode<?>, Object> to delegate to visit(AbstractApexNodeBase, Object). Then ApexNode<?> would still be called for every node other than ASTFormalComment, and AbstractApexNodeBase would be called for every node. Then update AbstractNcssCountRule and ExcessiveNodeCountRule to override visit for AbstractApexNodeBase instead of ApexNode<?>.

Regardless, feel free to just take my changes and integrate them as you see fit.

Thanks,
Jeff

@adangel

This comment has been minimized.

Member

adangel commented Oct 23, 2018

Thanks for your suggestions.

Option 1 is almost possible as you described it: it would change the implementation of the visitor in AbstractApexRule. If we directly delegate to visit(AbstractApexNodeBase), we can reduce the stack size (in comparison to delegate to visit(ApexNode) which delegates then to visit(AbstractApexNodeBase)). Removing visit(ApexNode) should not be done, but we can deprecate it and mark it for removal for PMD 7.0.0. Unless there is a usecase to visit only all nodes except for ASTFormalComments....

For you 2 option: I think, changing AbstractNcssCountRule and ExcessiveNodeCountRule should be changed to override visit(AbstractApexNodeBase) in any case...

Regardless, feel free to just take my changes and integrate them as you see fit.

Will do, thanks again!

@adangel adangel self-assigned this Oct 23, 2018

adangel added a commit to adangel/pmd that referenced this issue Oct 24, 2018

@adangel adangel added the has:pr label Oct 24, 2018

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