[apex] Summit-AST Apex module - Part 6 Passing testsuite #4251

aaronhurst-google · 2022-12-02T15:56:46Z

Describe the PR

Fix all issues to pass full testsuite. (Note that a few tests were previously disabled with TODOs. These fixes are still outstanding.)

Quick summary:

Upgrade com.google.summit:summit-ast to release version 2,0,0
Promote guava from test to compile scope dependency. It has been a dependency for some non-test cases (e.g. running ast-dump) and these were only succeeding due to the copy of guava bundled inside the apex-jorje.jar.
Replace wrapped AST node inside ASTMethod with equivalent details, allowing PMD AST nodes to be created for synthetic methods that don't correspond to parsed code. A trigger invoke method is one such example.
Synthesize ASTMethod nodes for trigger invoke methods.
Case normalize primitive type names, to match the behavior of Jorje.
Queries for parent classes and interfaces return type-erased version of name (without type arguments), to match previous behavior.
Full support for comments: both ASTFormalComments, comments in blocks, and suppression comments. Utilized existing code to the extent possible, which now lives in ApexCommentBuilder.java.
Implement separate SOQL and SOSL nodes with queries and bindings. Implement EnumValues (which create ASTField nodes) and VariableDeclarationGroup. (This completes all syntax, AFAIK)
Update test of source code positions with some minor differences. Summit re-enables testing positions of modifiers. Also some node counts are slightly different.
Replace Xpath use of deprecated getOperator with getOp.
Delete deprecated Jorje code from pmd-visualforce.
Property getter and setter methods default to visibility modifiers on property.
Fix PMD violations.
Fix checkstyle violations.

Related issues

Issue #3766

Fix an unexpected `RuntimeException` from `AbstractApexNodeBase.getBeginColumn` in instances where a `Node` had a start column of `0`.

Set `ASTMethod.getImage` to name of type for constructors

Change-Id: I033160534044936ac2ec416428662024b63e8c5f

- Don't build the `exceptionVariable` property of `CatchBlock` nodes. Some rules (e.g. `LocalVariableNamingConventionsRule`) expect the parent of every `ASTVariableDeclaration` to be an `ASTVariableDeclarationStatements`. - Fix `LocalVariableNamingConventionsRule` crash.

Make updates related to new VariableDeclarationGroup. Translate SOQL and SOSL expressions and bindings. Change-Id: I18995800e292cabe9f61176fa7aefedfc9729def

Change-Id: I6fb29b07e3679f333cae37fe28dfd8b14c8d4a20

Change-Id: Iec7c8335b966b243a63243ad631193b82598808b

Change-Id: I662171da66517235ce30a701ed84b687edfdb3a3

* ASTFormalComments: represent and build. * Mark comment containers * Populate suppression map This reuses the existing code to the extent possible. Makes ApexDocTest pass. Change-Id: I5843ffa9174f7f501aae551e5d493ee973c3dd45

Change-Id: I06a3bc7afce01d2050c46aa2ca674a7a91fc4c5a

* Case normalize primitive types (e.g. Integer) * Use type-erased names for super classes and interfaces * Include type arguments for all other uses Add documentation. Change-Id: I9edf979c58a5fcf6f251e93013be85fea22a8be1

…ecessarily) disabled code. Change-Id: I45fab6872f5a69910452b6ac2429716b677bd93a

Change-Id: I232b8e7ad0ffc5f9ac8beab741e48d90b24063d4

Change-Id: I92b456c124c50ceb20416d69b1d88d0b5405dd89

…etOperator. Change-Id: I1fdfc314ede7ccfcfe3e169acaef309ba07c17c1

Update code to remove reference to deleted MODULO operator. Change-Id: I39d9f7adc0407aafa9af31d3b2253c6c37c735b1

…erences. Re-enable some tests that were disabled due to Jorje limitations. Change-Id: Id81582231afcc3b2e9f13ac52860ac0de538f41b

Change-Id: Id580cdfa39b4288c239a496ed4a61afb123b0b2f

…tic "invoke" method inside triggers. Change-Id: I21344a36c9795deffd9c62aa0e768eb6f6742796

ghost · 2022-12-02T16:09:10Z

	2 Messages
📖	Compared to experimental-apex-parser: This changeset changes 15 violations, introduces 34 new violations, 2 new errors and 0 new configuration errors, removes 19 violations, 0 errors and 0 configuration errors. Full report
📖	Compared to master: This changeset changes 15 violations, introduces 1202 new violations, 2 new errors and 0 new configuration errors, removes 21 violations, 0 errors and 0 configuration errors. Full report
✅	Compared to experimental-apex-parser: This changeset changes 15 violations, introduces 33 new violations, 2 new errors and 0 new configuration errors, removes 19 violations, 0 errors and 0 configuration errors. Full report
✅	Compared to master: This changeset changes 15 violations, introduces 1201 new violations, 2 new errors and 0 new configuration errors, removes 21 violations, 0 errors and 0 configuration errors. Full report
✅	Compared to experimental-apex-parser: This changeset changes 15 violations, introduces 33 new violations, 2 new errors and 0 new configuration errors, removes 19 violations, 0 errors and 0 configuration errors. Full report
✅	Compared to master: This changeset changes 15 violations, introduces 33 new violations, 2 new errors and 0 new configuration errors, removes 19 violations, 0 errors and 0 configuration errors. Full report

Generated by 🚫 Danger

…ue node. Change-Id: I1542cdd3da3675a04f36c0b0627ea837e2005ac3

Change-Id: I3371e80418e11766657e7f461d26f440fcd78e79

Change-Id: I6c371bdecaf08d306bee7308e94f2f1bc5ef0e4c

…sting behavior. Change-Id: I7b3406a60bbc18dc2b1e441b8c0122452f709404

Change-Id: Ib8d4a2d5941b47c47cbd469c1af5ab5f405b120f

…tactic type name. Change-Id: I6ea22b890e60f24c2684f85753223f240e33509a

…t were previous satisfied by copy inside Jorje JAR. Change-Id: Iebbc77d4f1ece8ec4712f749356d877858c14d21

adangel

Thanks!
I'm really happy, that the rules are basically untouched - which is a good sign for code compatibility.

I've now also made our regression tester working for this branch, so the report is now available: https://chunk.io/pmd/69b8157066734c538c40dd3e8dbe6a82/diff1/index.html

It seems, that most rules work as before. There are some, that don't find issues anymore (ApexDoc), and some that relied on jorje actually resolving the types (AvoidNonExistingAnnotations), which we don't do. Maybe in that case, it would be better to rewrite the rule to have an explicit list of allowed annotations... (and we should then deprecated ASTAnnotation::isResolved on master as well).

Delete deprecated Jorje code from pmd-visualforce.

Not sure, maybe that's too early. It depends on where this branch will land and how this branch will be distributed.
If we decide to merge this branch into master(pmd6), then we'd need to restore these deprecated methods.

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

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

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

adangel · 2022-12-20T14:44:18Z

pmd-apex/src/test/java/net/sourceforge/pmd/lang/apex/ast/ApexParserTest.java

@@ -36,7 +35,7 @@ public void understandsSimpleFile() {

        // Verify
        List<ASTMethod> methods = rootNode.findDescendantsOfType(ASTMethod.class);
-        assertEquals(4, methods.size());
+        assertEquals(1, methods.size());


I guess, this difference is because of the synthetic "<clinit>" and "<init>" methods. I checked it on the master branch: it these two methods plus "clone", which apparently jorje also generates.
Should we aim to mimic this behavior (and thus add a TODO here), or not? I think, we have several places, where we actually filter out these synthetic methods in rules... So, it would actually make more sense to not have these methods in the AST. But that would be a change to the current apex impl...

Right. A user very likely wouldn't want to see any static analysis issue in synthetic code. These are difficult to present meaningfully and generally are not actionable for them to fix. +1 to suppressing them.

Thinking more generally... Is there any mechanism to mark and skip synthetic AST blocks? I'm imagining some sort of opt-in mechanism, where rules wouldn't visit synthetic nodes unless they pass a special flag or such. Right now I see a few conditions like !node.getImage().matches("<clinit>|<init>|clone") inconsistently dispersed in the code.

Representing these methods would have value in the future to resolve calls, build a callgraph, or some other higher-level abstractions. (FYI we're discussing a separate representation for symbol objects, but this is in the very early stages. Happy to discuss if you'd like.)

But this is a bit speculative at the moment. If you don't have any objection, I think it should be fine/easier to leave them out for now and note it somewhere.

Representing these methods would have value in the future to resolve calls, build a callgraph, or some other higher-level abstractions.

That's a good point. However, I'd also remove these synthetic methods for now completely to keep it simple. If we need them, we can add them later again (and maybe mark the AST nodes somehow as "synthethic", e.g. by a simple boolean flag).

adangel · 2022-12-20T14:49:26Z

pmd-apex/src/test/java/net/sourceforge/pmd/lang/apex/rule/ApexXPathRuleTest.java

+    public void testOldOperatorProperty() {
+        Report report = apex.executeRuleOnResource(makeXPath("//BooleanExpression[@Operator='&&']"),
+                                                   "BooleanExpressions.cls");
+        assertSize(report, 0);  // retired


👍 for creating a separate test case.
But: Shouldn't this xpath query with the deprecated attribute Operator still work? It seems, it doesn't find any nodes...

The deprecated getOperator method was fully removed in this branch. The method returned a Jorje type and would block the above-mentioned goal to compile without the Jorje JAR... or faking some of its classes.

Since I updated some xpath tests, I added an old variant to investigate and capture the xpath behavior after removal.

This is another case where the merge back into pmd6 or 7 should preserve the deprecated method. (And it would also need a summit-to-jorje conversion to behave correctly.)

pmd-apex/src/test/resources/net/sourceforge/pmd/lang/apex/ast/SafeNavigationOperator.txt

pom.xml

…teralExpression.java Co-authored-by: Andreas Dangel <[email protected]>

…lass.

aaronhurst-google · 2023-01-05T17:00:01Z

I've now also made our regression tester working for this branch, so the report is now available:

Thank you much! I will start investigating some of these issues.

Delete deprecated Jorje code from pmd-visualforce.

Not sure, maybe that's too early. It depends on where this branch will land and how this branch will be distributed.
If we decide to merge this branch into master(pmd6), then we'd need to restore these deprecated methods.

Our team is eager to prototype a copy with the Jorje code actually removed (as opposed to present but deprecated), so it's useful if it could be this experimental branch. (Maybe others would find that useful?) I be happy to resolve the merge into master or pmd 7 to restore and keep this code deprecated as long as you see fit. Is that ok on your end?

The deprecation was proposed for master/pmd6.

…ately after modifiers. This matches one observed property in the way that Jorje ordered nodes. Update the dump test.

adangel · 2023-01-14T10:26:00Z

Our team is eager to prototype a copy with the Jorje code actually removed (as opposed to present but deprecated), so it's useful if it could be this experimental branch. (Maybe others would find that useful?) I be happy to resolve the merge into master or pmd 7 to restore and keep this code deprecated as long as you see fit. Is that ok on your end?

That sounds fine. I'm currently thinking about how we can release PMD 7 sooner (see #4284). So, thinking ahead: Maybe we can just deprecate the removed methods/classes for PMD 6 (I think, this is already the case) and plan to merge this experimental branch eventually into PMD 7.
That means, that there wouldn't be a PMD6-solution with jorje removed outside of this experimental branch, but PMD7 would have jorje removed... WDYT?

adangel · 2023-01-14T10:27:38Z

I've now also made our regression tester working for this branch, so the report is now available:

Thank you much! I will start investigating some of these issues.

I'm going to merge this PR now. The rule related fixes can come in further PRs.

adangel · 2023-01-14T11:20:29Z

@aaronhurst-google I've merged also master into experimental-apex-parser.

I've fixed the problem around the rule "AvoidNonExistentAnnotations" (I think): a9cbe7e

However, there is now a new change in - it's about #4146 .
This will require an update to the apex grammar -> it needs to be updated to support the new with clauses WITH USER_MODE and WITH SYSTEM_MODE: https://github.com/nawforce/apex-parser/blob/a5adae847d394fe8bb2c8ce03329a8caccc4871b/antlr/ApexParser.g4#L686-L689

Could you take care of this? Thanks!

I've left the tests activated, so the build is failing now again.

Interestingly, jorje also doesn't support all the latest language features:

pmd/pmd-apex/src/test/resources/net/sourceforge/pmd/lang/apex/rule/security/xml/ApexCRUDViolation.xml

Lines 1802 to 1824 in d73095a

    
               <test-code disabled="true"> 
        
                   <!-- disabled, because jorje can't parse "as user" --> 
        
                   <description>Consider "insert as"</description> 
        
                   <expected-problems>1</expected-problems> 
        
                   <expected-linenumbers>7</expected-linenumbers> 
        
                   <code><![CDATA[ 
        
           public class UserMode { 
        
           public void coverAllCasesWithTest() { 
        
               Contact c; 
        
               c = [SELECT Name FROM Contact WITH USER_MODE]; 
        
               // Should be flagged a critical issue 
        
               insert contact; 
        
               // Should be ignored 
        
               insert as user contact; 
        
               // Should be at best a warning because it ignores CRUD but explicitly 
        
               insert as system contact; 
        
           } 
        
           } 
        
           ]]></code> 
        
               </test-code>

eklimo and others added 20 commits December 1, 2022 18:47

Fix b/243743042

90aec7a

Fix an unexpected `RuntimeException` from `AbstractApexNodeBase.getBeginColumn` in instances where a `Node` had a start column of `0`.

Fix b/243747010

9e5d4d4

Set `ASTMethod.getImage` to name of type for constructors

Allow rules with version checks to properly run

c6ad8fb

Update TODO comments

da9e1ef

Change-Id: I033160534044936ac2ec416428662024b63e8c5f

Upgrade Summit to 1.1.0 (snapshot).

4fbe0de

Make updates related to new VariableDeclarationGroup. Translate SOQL and SOSL expressions and bindings. Change-Id: I18995800e292cabe9f61176fa7aefedfc9729def

Add back ApexTreeBuilder.java.

157558c

Change-Id: I6fb29b07e3679f333cae37fe28dfd8b14c8d4a20

Translate EnumValue node to ASTField.

fb32ba3

Change-Id: Iec7c8335b966b243a63243ad631193b82598808b

Rename former ApexTreeBuilder.java to ApexCommentBuilder.java.

611d788

Change-Id: I662171da66517235ce30a701ed84b687edfdb3a3

Comments!

581d4d6

* ASTFormalComments: represent and build. * Mark comment containers * Populate suppression map This reuses the existing code to the extent possible. Makes ApexDocTest pass. Change-Id: I5843ffa9174f7f501aae551e5d493ee973c3dd45

Comment position fixup

701fb53

Change-Id: I06a3bc7afce01d2050c46aa2ca674a7a91fc4c5a

Replicate previous behavior for type names:

ff9de70

* Case normalize primitive types (e.g. Integer) * Use type-erased names for super classes and interfaces * Include type arguments for all other uses Add documentation. Change-Id: I9edf979c58a5fcf6f251e93013be85fea22a8be1

Fix test failures related to Boolean cognitive complexity due to (unn…

914f6a8

…ecessarily) disabled code. Change-Id: I45fab6872f5a69910452b6ac2429716b677bd93a

Fix test failure due to incorrect case-sensitive identifier comparison.

ecad878

Change-Id: I232b8e7ad0ffc5f9ac8beab741e48d90b24063d4

Update TreeDumper regression test with Summit AST output.

0524d66

Change-Id: I92b456c124c50ceb20416d69b1d88d0b5405dd89

Update test to use getOp replacement for deprecated and now retired g…

072bb72

…etOperator. Change-Id: I1fdfc314ede7ccfcfe3e169acaef309ba07c17c1

Upgrade Summit AST dependency to release version 2.0.0.

6897164

Update code to remove reference to deleted MODULO operator. Change-Id: I39d9f7adc0407aafa9af31d3b2253c6c37c735b1

Update test with Jorje vs. Summit source location and node count diff…

19225dc

…erences. Re-enable some tests that were disabled due to Jorje limitations. Change-Id: Id81582231afcc3b2e9f13ac52860ac0de538f41b

Remove pmd-visualforce Jorje dependency.

d57d482

Change-Id: Id580cdfa39b4288c239a496ed4a61afb123b0b2f

Refactor ASTMethod to support synthetic definitions, and add a synthe…

62080c6

…tic "invoke" method inside triggers. Change-Id: I21344a36c9795deffd9c62aa0e768eb6f6742796

aaronhurst-google added 7 commits December 2, 2022 17:11

ASTField: store String type name instead of either typeRef or enumVal…

9fef04f

…ue node. Change-Id: I1542cdd3da3675a04f36c0b0627ea837e2005ac3

Fix PMD dogfood issues.

8734d45

Change-Id: I3371e80418e11766657e7f461d26f440fcd78e79

Fix checkstyle violations.

16a492f

Change-Id: I6c371bdecaf08d306bee7308e94f2f1bc5ef0e4c

Change case normalization of primitive from 'ID' to 'Id' to match exi…

e9eafdd

…sting behavior. Change-Id: I7b3406a60bbc18dc2b1e441b8c0122452f709404

Property getter and setter methods default to visibility of property.

6d35496

Change-Id: Ib8d4a2d5941b47c47cbd469c1af5ab5f405b120f

Update tested return type name to match Summit, which returns the syn…

1a5c571

…tactic type name. Change-Id: I6ea22b890e60f24c2684f85753223f240e33509a

Move guava to a non-test dependency, due to implicit dependencies tha…

626aec5

…t were previous satisfied by copy inside Jorje JAR. Change-Id: Iebbc77d4f1ece8ec4712f749356d877858c14d21

aaronhurst-google force-pushed the clean-testsuite branch from e5678e8 to 626aec5 Compare December 2, 2022 17:13

adangel mentioned this pull request Dec 19, 2022

[apex] Replace Jorje with fully open source front-end #3766

Closed

16 tasks

adangel changed the title ~~[apex] New Apex Parser - Passing testsuite~~ [apex] Summit-AST Apex module - Part 6 Passing testsuite Dec 19, 2022

adangel reviewed Dec 20, 2022

View reviewed changes

aaronhurst-google and others added 2 commits January 3, 2023 16:39

Update pmd-apex/src/main/java/net/sourceforge/pmd/lang/apex/ast/ASTLi…

8745d00

…teralExpression.java Co-authored-by: Andreas Dangel <[email protected]>

Reduce visibility of ApexCommentBuilder. Remove @deprecated for new c…

60da7bd

…lass.

aaronhurst-google mentioned this pull request Jan 4, 2023

[apex] Deprecate ApexRootNode.getApexVersion #4280

Merged

aaronhurst-google added 5 commits January 5, 2023 22:54

Remove development Maven repository.

656dff9

Remove ApexRootNode.getApexVersion.

b4b9dd2

The deprecation was proposed for master/pmd6.

Updated test after removing getApexVersion.

58ca81d

Sort children of ASTUserClass to order ASTField nodes earlier, immedi…

ebb861a

…ately after modifiers. This matches one observed property in the way that Jorje ordered nodes. Update the dump test.

Fix checkstyle issue.

b8ccafa

adangel self-requested a review January 10, 2023 11:47

adangel approved these changes Jan 14, 2023

View reviewed changes

adangel merged commit 5c4e5ec into pmd:experimental-apex-parser Jan 14, 2023

adangel added this to the 7.0.0 milestone Feb 13, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[apex] Summit-AST Apex module - Part 6 Passing testsuite #4251

[apex] Summit-AST Apex module - Part 6 Passing testsuite #4251

aaronhurst-google commented Dec 2, 2022 •

edited

Loading

ghost commented Dec 2, 2022 •

edited by ghost

Loading

adangel left a comment

adangel Dec 20, 2022

aaronhurst-google Jan 5, 2023

adangel Jan 14, 2023

adangel Dec 20, 2022

aaronhurst-google Jan 6, 2023

aaronhurst-google commented Jan 5, 2023

adangel commented Jan 14, 2023

adangel commented Jan 14, 2023

adangel commented Jan 14, 2023

[apex] Summit-AST Apex module - Part 6 Passing testsuite #4251

[apex] Summit-AST Apex module - Part 6 Passing testsuite #4251

Conversation

aaronhurst-google commented Dec 2, 2022 • edited Loading

Describe the PR

Related issues

ghost commented Dec 2, 2022 • edited by ghost Loading

adangel left a comment

Choose a reason for hiding this comment

adangel Dec 20, 2022

Choose a reason for hiding this comment

aaronhurst-google Jan 5, 2023

Choose a reason for hiding this comment

adangel Jan 14, 2023

Choose a reason for hiding this comment

adangel Dec 20, 2022

Choose a reason for hiding this comment

aaronhurst-google Jan 6, 2023

Choose a reason for hiding this comment

aaronhurst-google commented Jan 5, 2023

adangel commented Jan 14, 2023

adangel commented Jan 14, 2023

adangel commented Jan 14, 2023

aaronhurst-google commented Dec 2, 2022 •

edited

Loading

ghost commented Dec 2, 2022 •

edited by ghost

Loading