[apex] Apexlink POC #2830

nawforce · 2020-10-13T20:06:07Z

Describe the PR

This is a POC for using apexlink to provide unused method analysis.

To control the global analysis I have added the flag -multifileanalysisdirectory which should be set to the root directory of the Salesforce metadata, where sfdx-project.json resides. The POC code will fail on non-SFDX projects currently but could be fixed to handle them.

If the flag is set then the apexlink analysis runs when the first class file is parsed. On large projects this analysis can take some time so this isn't a good place for it but was OK for a quick POC.

I have added an AvoidUnusedMethodRule as an example, this peeks into the 'Issues' reported by the apexlink analysis and adds a violation if there is a match. The handling here could be made more efficient, it iterates all Issues for the class on each method, but again this was sufficient to prove the approach was viable.

I have put a few extra comments in ApexMultifileAnalysis.java that will hopefully help with understanding what is happening.

Related issues

Fixes [apex] Integrate nawforce/ApexLink to build robust Unused rule #2667

rsoesemann · 2020-10-13T20:38:13Z

@oowekyala, @adangel I guess with Multifile rules we cannot use the XML based rule testing of PMD anymore. Or are there examples for tests that require multiple files in other languages?

oowekyala

This looks good!

A couple of changes can be made when #2518 is implemented:

The new CLI option should be converted to a language property
The ApexMultifileAnalysis class should be part of the language state

Other than that I have very little to nitpick on. Thanks for the good work!

pmd-apex/src/main/resources/category/apex/design.xml

pmd-apex/src/main/java/net/sourceforge/pmd/lang/apex/rule/design/AvoidUnusedMethodRule.java

pmd-apex/src/main/java/net/sourceforge/pmd/lang/apex/multifile/ApexMultifileAnalysis.java

pmd-apex/src/main/java/net/sourceforge/pmd/lang/apex/rule/design/AvoidUnusedMethodRule.java

pmd-apex/src/main/java/net/sourceforge/pmd/lang/apex/multifile/ApexMultifileAnalysis.java

adangel

Thanks for the PoC, looks promising!

pmd-apex/src/main/java/net/sourceforge/pmd/lang/apex/ast/ApexRootNode.java

pmd-apex/src/main/java/net/sourceforge/pmd/lang/apex/rule/design/AvoidUnusedMethodRule.java

adangel · 2020-10-17T08:21:46Z

pmd-core/src/main/java/net/sourceforge/pmd/cli/PMDParameters.java

@@ -75,6 +75,10 @@
            description = "Specifies the string that marks a line which PMD should ignore; default is NOPMD.")
    private String suppressmarker = "NOPMD";

+    @Parameter(names = "-multifileanalysisdirectory",
+            description = "Specifies a directory that contains sources the may be used in a multi-file analysis; default is empty.")


This option is language dependent, I agree with @oowekyala that this would be a fit for language properties.

So that I understand, how this works: The directory, you specify here, contains the file "sfdx-project.json". Where does this file come from? Is it generated by an execution of ApexLink before executing PMD? Is this directory a different directory than you would specify using "-dir" (the "Root directory for sources.")? I'm unfamiliar with the structure of Apex projects.

For me, this looks like a property to enable/make use of ApexLink, rather than enabling multifileanalysis... and ApexLink happens to understand already the relationships between the files that are being analyzed (that's what we want with multifile analysis). PMD already has the knowledge where all files a located (-dir), but doesn't take advantage of this, because we analyze file by file and don't build up a global metadata structure like "sfdx-project.json". Hence my question: Who builds this file and when?

Ok, looking at e.g. https://github.com/apex-enterprise-patterns/fflib-apex-common/blob/master/sfdx-project.json this file actually just contains the path to the sources (possibly multiple paths). So, ApexLink then builds the metadata structure, when creating the Org, right?

I assume, if the paths specified by sfdx-project.json and the paths specified by "-dir" don't match, that would be a configuration error and I'm wondering, what we could do to make it easier for users to call PMD. For apex projects, that have a sfdx-project structure (https://developer.salesforce.com/docs/atlas.en-us.sfdx_dev.meta/sfdx_dev/sfdx_dev_source_file_format.htm) we could say, that you need to specify the directory where sfdx-project.json is located via "-dir" and PMD would then add the referenced directories, as if specified manually via "-dir". In that case, we might have instead a parameter to opt-in/opt-out for that feature (apex language property: consider directories specified in sfdx-project.json).

Ok, looking at e.g. https://github.com/apex-enterprise-patterns/fflib-apex-common/blob/master/sfdx-project.json this file actually just contains the path to the sources (possibly multiple paths). So, ApexLink then builds the metadata structure, when creating the Org, right?

Yes, it reads the directories from sfdx-project.json to work out what to include in the analysis. The handling of this used to be built into ApexLink but I recently split into a separate library. There is a bit more to the format than being multiple directories, it's really more like multiple modules with either implicit or explicit dependencies between them. Treating them as modules matters in a multi-file analysis because there are rules around visibility and duplicates etc that you can't handle if you think of them as just the sum of a set of directories. There is also .forceignore which is similar in nature to .gitignore, if present its in the same directory as sfdx-project.json.

The sfdx-project.json also contains a namespace for the project. It's critical this is available for multi-file work as it enables the analysis to understand package boundaries. For single file analysis I would imagine a few rules might find it also handy if it could be made available.

I assume, if the paths specified by sfdx-project.json and the paths specified by "-dir" don't match, that would be a configuration error and I'm wondering, what we could do to make it easier for users to call PMD. For apex projects, that have a sfdx-project structure (https://developer.salesforce.com/docs/atlas.en-us.sfdx_dev.meta/sfdx_dev/sfdx_dev_source_file_format.htm) we could say, that you need to specify the directory where sfdx-project.json is located via "-dir" and PMD would then add the referenced directories, as if specified manually via "-dir". In that case, we might have instead a parameter to opt-in/opt-out for that feature (apex language property: consider directories specified in sfdx-project.json).

Having "-dir" understand sfdx-project.json makes sense to me as a general approach but likely you want to delegate resolving "-dir" to a set of class files for analysis to a Salesforce specific handler that works alongside the existing handling.

The library I have for this might be a bit overweight for what PMD needs as ApexLink is interested in other project files and they can have some quite unusual handling behaviours. The part I have that is probably most valuable to PMD is the forceignore handling which should be straight forward to port to Java from its current Scala implementation if that made sense.

nawforce · 2020-10-18T09:42:09Z

Yes, that will need to be considered. As PMD doesn't support multifiles in any language, there are no examples.

I didn't look at testing as part of this work but thought I should mention that I approached this using virtual file system support, the FileSystemHelper.run sets this up. You do get some issues around conversions between Paths & Strings but mostly it works well for me.

  test("Unused method") {
    FileSystemHelper.run(
      Map(
        "Dummy.cls" -> "public class Dummy {public void foo() {}}"
      )
    ) { root: PathLike =>
      val org = Org.newOrg().asInstanceOf[OrgImpl]
      val pkg = org.newMDAPIPackageInternal(None, Seq(root), Seq())
      assert(!org.issues.hasMessages)
      assert(
        pkg.reportUnused().getMessages(root.join("Dummy.cls").toString) == 
          "Unused: line 1 at 32-35: Unused Method 'void foo()'\n")
    }
  }

# Conflicts: # pmd-apex/pom.xml # pmd-apex/src/main/java/net/sourceforge/pmd/lang/apex/ApexParser.java # pmd-apex/src/main/java/net/sourceforge/pmd/lang/apex/ast/ApexParser.java # pmd-apex/src/main/java/net/sourceforge/pmd/lang/apex/ast/ApexRootNode.java # pmd-core/src/main/java/net/sourceforge/pmd/ant/internal/PMDTaskImpl.java # pmd-core/src/main/java/net/sourceforge/pmd/lang/ParserOptions.java # pom.xml

ghost · 2020-10-22T22:47:42Z

	2 Messages
📖	Compared to pmd/7.0.x: This changeset changes 0 violations, introduces 0 new violations, 0 new errors and 0 new configuration errors, removes 0 violations, 0 errors and 0 configuration errors. Full report
📖	Compared to master: This changeset changes 9192 violations, introduces 8327 new violations, 1 new errors and 0 new configuration errors, removes 15657 violations, 10 errors and 2 configuration errors. Full report
✅	Compared to pmd/7.0.x: This changeset changes 0 violations, introduces 0 new violations, 0 new errors and 0 new configuration errors, removes 0 violations, 0 errors and 0 configuration errors. Full report
✅	Compared to master: This changeset changes 2916 violations, introduces 8103 new violations, 1 new errors and 0 new configuration errors, removes 15648 violations, 10 errors and 2 configuration errors. Full report
✅	Compared to pmd/7.0.x: This changeset changes 0 violations, introduces 0 new violations, 0 new errors and 0 new configuration errors, removes 0 violations, 0 errors and 0 configuration errors. Full report
✅	Compared to master: This changeset changes 2916 violations, introduces 8103 new violations, 1 new errors and 0 new configuration errors, removes 15648 violations, 10 errors and 2 configuration errors. Full report
✅	Compared to pmd/7.0.x: This changeset changes 0 violations, introduces 0 new violations, 0 new errors and 0 new configuration errors, removes 0 violations, 0 errors and 0 configuration errors. Full report
✅	Compared to master: This changeset changes 2916 violations, introduces 8103 new violations, 1 new errors and 0 new configuration errors, removes 15648 violations, 10 errors and 2 configuration errors. Full report
✅	Compared to pmd/7.0.x: This changeset changes 0 violations, introduces 0 new violations, 0 new errors and 0 new configuration errors, removes 0 violations, 0 errors and 0 configuration errors. Full report
✅	Compared to master: This changeset changes 2916 violations, introduces 8103 new violations, 1 new errors and 0 new configuration errors, removes 15648 violations, 10 errors and 2 configuration errors. Full report
✅	Compared to pmd/7.0.x: This changeset changes 0 violations, introduces 0 new violations, 0 new errors and 0 new configuration errors, removes 0 violations, 0 errors and 0 configuration errors. Full report
✅	Compared to master: This changeset changes 2919 violations, introduces 8103 new violations, 1 new errors and 0 new configuration errors, removes 15645 violations, 10 errors and 2 configuration errors. Full report
✅	Compared to pmd/7.0.x: This changeset changes 0 violations, introduces 0 new violations, 0 new errors and 0 new configuration errors, removes 0 violations, 0 errors and 0 configuration errors. Full report
✅	Compared to master: This changeset changes 2919 violations, introduces 8103 new violations, 1 new errors and 0 new configuration errors, removes 15645 violations, 10 errors and 2 configuration errors. Full report
✅	Compared to pmd/7.0.x: This changeset changes 0 violations, introduces 0 new violations, 0 new errors and 0 new configuration errors, removes 0 violations, 0 errors and 0 configuration errors. Full report
✅	Compared to master: This changeset changes 2068 violations, introduces 6043 new violations, 2 new errors and 0 new configuration errors, removes 15167 violations, 10 errors and 2 configuration errors. Full report
✅	~~No java rules are changed!~~
✅	Compared to pmd/7.0.x: This changeset introduces 0 new violations, 0 new errors and 0 new configuration errors, removes 0 violations, 0 errors and 0 configuration errors. Full report
✅	Compared to master: This changeset introduces 791 new violations, 6 new errors and 0 new configuration errors, removes 11 violations, 16 errors and 2 configuration errors. Full report
✅	Compared to pmd/7.0.x: This changeset introduces 0 new violations, 0 new errors and 0 new configuration errors, removes 0 violations, 0 errors and 0 configuration errors. Full report
✅	Compared to master: This changeset introduces 791 new violations, 6 new errors and 0 new configuration errors, removes 11 violations, 16 errors and 2 configuration errors. Full report

Generated by 🚫 Danger

# Conflicts: # pom.xml

adangel

This looks pretty ready for now. I think, the only part we should add, is a rule test for the new rule AvoidUnusedMethod - otherwise we'll never notice, when it doesn't work anymore.
There are of course some parts to be improved, e.g. the language properties. But that could evolve later.
Wdyt @oowekyala ?

pmd-apex/pom.xml

pmd-apex/src/main/resources/category/apex/design.xml

rsoesemann · 2021-04-07T09:29:49Z

@rmohan20 and @jbartolotta-sfdc I want to point your attention to this. The addition of ApexLinnk will bring PMD closer to Multifile Analysis and give us a chance to remove a huge source of false positives in other rules. All the existing rules currently report security issues because a Check is delegated to another method or class.

Does it make sense that your team looks closer into this and maybe join forces?

pmd-apex/src/main/java/net/sourceforge/pmd/lang/apex/multifile/ApexMultifileAnalysis.java

jbartolotta-sfdc · 2021-04-08T18:32:06Z

@rmohan20 and @jbartolotta-sfdc I want to point your attention to this. The addition of ApexLinnk will bring PMD closer to Multifile Analysis and give us a chance to remove a huge source of false positives in other rules. All the existing rules currently report security issues because a Check is delegated to another method or class.

Does it make sense that your team looks closer into this and maybe join forces?

@rsoesemann Thanks for pointing this out, this looks like a good addition for multi file analysis.

We are currently investigating Dataflow analysis for Apex. We will circle back when we have more information on our planned roadmap and see how this might all fit together.

FYI @rmohan20

nawforce · 2021-04-08T19:41:50Z

I have moved the apexlink version to latest, there was a bugfix in the caching that could impact the output.

pmd-apex/src/main/resources/category/apex/design.xml

Refs #2830

nawforce · 2021-04-16T21:54:48Z

Thanks for all the help getting my rough POC merged. Looking forward to see where we can take this next.

nawforce added 4 commits September 25, 2020 22:57

Add apexlink dependency

7bf7b1a

Add a configuration for requesting multi-file analysis

c635d53

Hack in unused method rule

d48c8a7

Code tidy

a5fe957

nawforce changed the title ~~Apexlink~~ Apexlink POC Oct 13, 2020

rsoesemann requested a review from oowekyala October 13, 2020 20:38

rsoesemann added the a:new-rule Proposal to add a new built-in rule label Oct 13, 2020

rsoesemann requested a review from adangel October 13, 2020 20:39

oowekyala changed the title ~~Apexlink POC~~ [apex] Apexlink POC Oct 13, 2020

oowekyala added this to the 7.0.0 milestone Oct 13, 2020

oowekyala reviewed Oct 15, 2020

View reviewed changes

adangel reviewed Oct 17, 2020

View reviewed changes

This comment was marked as off-topic.

Sign in to view

oowekyala mentioned this pull request Oct 18, 2020

[apex] Remove Apex ProjectMirror #2836

Merged

4 tasks

nawforce added 3 commits October 24, 2020 21:08

Resolve PR comments

279c673

Merge remote-tracking branch 'upstream/pmd/7.0.x' into apexlink

d98ec55

# Conflicts: # pom.xml

Merge remote-tracking branch 'upstream/pmd/7.0.x' into apexlink

862ee7b

This was referenced Oct 28, 2020

[core] Language properties #2518

Closed

[vf] Provide expression type information to Visualforce rules to avoid false positives #2864

Merged

oowekyala mentioned this pull request Nov 27, 2020

[core] Language properties CLI surface syntax #2947

Open

6 tasks

oowekyala mentioned this pull request Feb 13, 2021

[core] Improve Parser interface #3085

Merged

4 tasks

oowekyala added 4 commits February 13, 2021 21:02

Merge branch '7.0.x' into pr/2830

7c42741

Remove some things from pmd core

fb359c2

Remove sentinel value

8fb4a08

Simplify tests

268f66f

adangel reviewed Feb 19, 2021

View reviewed changes

pmd-apex/pom.xml Outdated Show resolved Hide resolved

pmd-apex/src/main/resources/category/apex/design.xml Outdated Show resolved Hide resolved

pmd-apex/src/main/resources/category/apex/design.xml Outdated Show resolved Hide resolved

adangel mentioned this pull request Feb 22, 2021

[core] Support multiple files in rule tests #3131

Open

adangel added 3 commits February 23, 2021 19:24

Update ApexLink to 1.2.2

e8ac569

Update ruledoc for AvoidUnusedMethod (apex)

b2ab30a

[doc] Update release notes

ee6e819

oowekyala mentioned this pull request Mar 28, 2021

[doc] Document language module features #3175

Closed

oowekyala force-pushed the pmd/7.0.x branch from e7de833 to f9c4512 Compare April 2, 2021 22:03

oowekyala reviewed Apr 3, 2021

View reviewed changes

pmd-apex/src/main/resources/category/apex/design.xml Show resolved Hide resolved

oowekyala added 4 commits April 7, 2021 01:01

Merge branch '7.0.x' into apexlink

ae195a8

Fix APEX_ROOT_DIRECTORY not being set

c314ba8

Merge branch '7.0.x' into apexlink

77fbc0b

Add language attribute

862a320

jbartolotta-sfdc reviewed Apr 8, 2021

View reviewed changes

pmd-apex/src/main/java/net/sourceforge/pmd/lang/apex/multifile/ApexMultifileAnalysis.java Outdated Show resolved Hide resolved

Upgrade apex-link version

b2a421f

nawforce and others added 3 commits April 8, 2021 21:38

Extend description on AvoidUnusedMethod

97c8c3b

Only attempt loading once

cf3ed1c

Merge branch '7.0.x' into apexlink

1572484

oowekyala reviewed Apr 9, 2021

View reviewed changes

pmd-apex/src/main/resources/category/apex/design.xml Outdated Show resolved Hide resolved

oowekyala and others added 2 commits April 9, 2021 16:19

Add tests for logging behavior

1187b9e

Change rule name to UnusedMethod, correct logging and add FIXME

8bc57f3

adangel reviewed Apr 15, 2021

View reviewed changes

pmd-apex/src/main/resources/category/apex/design.xml Show resolved Hide resolved

adangel assigned oowekyala Apr 16, 2021

oowekyala added a commit that referenced this pull request Apr 16, 2021

Merge branch 'apexlink' into 7.0.x

a328794

Refs #2830

oowekyala merged commit 73f86ce into pmd:pmd/7.0.x Apr 16, 2021

adangel linked an issue Jan 13, 2023 that may be closed by this pull request

[apex] Integrate nawforce/ApexLink to build robust Unused rule #2667

Closed

adangel mentioned this pull request Jan 13, 2023

[apex] Integrate nawforce/ApexLink to build robust Unused rule #2667

Closed

adangel mentioned this pull request Jan 23, 2023

PMD 7 Tracking Issue #3898

Closed

55 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[apex] Apexlink POC #2830

[apex] Apexlink POC #2830

nawforce commented Oct 13, 2020 •

edited by adangel

Loading

rsoesemann commented Oct 13, 2020

oowekyala left a comment

adangel left a comment

adangel Oct 17, 2020

nawforce Oct 18, 2020

This comment was marked as off-topic.

nawforce commented Oct 18, 2020

ghost commented Oct 22, 2020 •

edited by ghost

Loading

adangel left a comment

rsoesemann commented Apr 7, 2021

jbartolotta-sfdc commented Apr 8, 2021

nawforce commented Apr 8, 2021

nawforce commented Apr 16, 2021

[apex] Apexlink POC #2830

[apex] Apexlink POC #2830

Conversation

nawforce commented Oct 13, 2020 • edited by adangel Loading

Describe the PR

Related issues

rsoesemann commented Oct 13, 2020

oowekyala left a comment

Choose a reason for hiding this comment

adangel left a comment

Choose a reason for hiding this comment

adangel Oct 17, 2020

Choose a reason for hiding this comment

nawforce Oct 18, 2020

Choose a reason for hiding this comment

This comment was marked as off-topic.

nawforce commented Oct 18, 2020

ghost commented Oct 22, 2020 • edited by ghost Loading

adangel left a comment

Choose a reason for hiding this comment

rsoesemann commented Apr 7, 2021

jbartolotta-sfdc commented Apr 8, 2021

nawforce commented Apr 8, 2021

nawforce commented Apr 16, 2021

nawforce commented Oct 13, 2020 •

edited by adangel

Loading

ghost commented Oct 22, 2020 •

edited by ghost

Loading