[apex] Summit-AST Apex module - Part 1 #4093

eklimo · 2022-08-15T10:21:29Z

Describe the PR

First part of Summit-AST-based Apex module.

Changes

Replace Jorje dependency with Summit-AST dependency
Refactor base classes
Replace ApexTreeBuilder
Build a few kinds of nodes

Upcoming changes

Completed tree builder

Related issues

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

Operators - Create operator enums to wrap internal Jorje operators. - Create getters returning new operator enums in operator-expression nodes. Deprecate old getters. - Refactor metrics classes to use new operator enums. Tests - Remove references to internal Jorje nodes.

pmd-apex/pom.xml

eklimo · 2022-08-18T05:02:54Z

Hi @oowekyala,
Sorry if this wasn't clear, but these changes are intended for PMD 6, so experimental-apex-parser should currently be synced with 2977bd8 (current master) without any commits from the 7.0.x branch.

- Remove Jorje dependencies - Add Summit-AST dependency - Comment out code until compilation succeeds

- Replace `ApexNode` type arguments with `Void` - Remove `ApexNode.getNode` - Remove `ASTCommentContainer` type parameter - Deprecate `ASTUserClassOrInterface` type parameter

Remove `node` field from `AbstractApexNode` to remove restriction of wrapping a single node. Create `AbstractApexNode.Single` subclass that wraps a single node. Other subclasses can be created as needed for common cases, or `AbstractApexNode` can be extended directly in irregular cases.

- Add call to `SummitAST` in `ApexParser` - Fix NPE involving `suppressMap` to allow `ApexTreeBuilder` to run - Remove unused `TopLevelVisitor`

Replace `CompilationUnit` with `TypeDeclaration` as the top-level node to match the AST produced by Jorje.

- Replace `ApexTreeBuilder.java` with `ApexTreeBuilder.kt` - Set up tree builder foundation - Build `TypeDeclaration` nodes

- Add `AbstractApexNode.Many`

- Add `buildChildren` function to tree builder

pom.xml

oowekyala · 2022-08-18T16:44:31Z

Hi @oowekyala, Sorry if this wasn't clear, but these changes are intended for PMD 6, so experimental-apex-parser should currently be synced with 2977bd8 (current master) without any commits from the 7.0.x branch.

My bad, thanks for clarifying. I just fixed it.

adangel

Thanks for working on this!

I noticed, that the CI build is not enabled for this branch - I'll see, whether I can let gh action execute a build for the branch experimental-apex-parser as well - without publishing the snapshots...

pmd-apex/src/main/java/net/sourceforge/pmd/lang/apex/ApexJorjeLogging.java

pom.xml

adangel · 2022-09-01T07:48:51Z

FYI @eklimo @aaronhurst-google I've updated the branch experimental-apex-parser to the latest master. Now we've got a couple of conflicts, probably due to #4081 and maybe #4092 and #4097.

… part-1 Change-Id: I254208dd5d1b9dbf5335a59a51372ec1d55f8535

aaronhurst-google · 2022-09-14T16:29:27Z

Merge conflicts resolved. @adangel

With the piecemeal PR strategy, the tests won't pass until completion. How do you want to handle this in the experimental branch, temporarily?

Ignore failures
Disable tests
Reconsider piecemeal approach and submit a single monolithic PR once all test pass

adangel · 2022-09-15T18:50:34Z

Merge conflicts resolved. @adangel

Thanks!

With the piecemeal PR strategy, the tests won't pass until completion. How do you want to handle this in the experimental branch, temporarily?
* Ignore failures

* Disable tests

* Reconsider piecemeal approach and submit a single monolithic PR once all test pass

Ignoring or disabling tests is always dangerous. If you are confident, that the failing test situation is only temporarily and will be resolved at some point, I think, we can risk it to ignore the failures for now (only in this experimental branch). The PR/branch builds will of course fail.

Before the experimental branch can be merged back into master, the tests need to pass, obviously. When the time has come to merge, we know hopefully more about possible compatibility issues (e.g. to answer the question, whether it's really feasible to replace jorje in a minor version). I also think, merging the experimental branch will require another overall big review in the end to check, that there are no leftovers (I see a couple of commented out sections with some TODOs). The overall review will hopefully be quicker because we have seen the code already in the smaller PRs.

When should we merge this PR into the experimental branch? Is this Part 1 ready from your side?

aaronhurst-google · 2022-09-23T17:04:00Z

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

-     *      this information.
-     */
-    @Deprecated
-    T getNode();


There is no longer a 1:1 mapping between PMD and the AST. Either this deprecated API needed to be reword or eliminated... the latter seemed like the clear choice.

Having access to the underlaying Jorje node made it possible to have the Jorje classes leak into PMDs AST - that's why we deprecated this method. It makes sense to have this removed.

aaronhurst-google · 2022-09-23T17:07:09Z

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

+     */
+    @Deprecated
+    @InternalApi
+    public static abstract class Single<T extends Node> extends AbstractApexNode {


This is a point worth calling out. Since even the (internal) mapping between PMD and the Summit AST isn't 1:1, we use subclass for the different relations between the two. For example, most nodes continue to wrap a Single AST node, but there are a few that wrap Many.

aaronhurst-google · 2022-09-23T17:29:19Z

@adangel Definitely: a fully-passing testsuite is the only acceptable merge state. A final review pass (from all of us) also sounds essential.

The pieces can go in to the experimental branch whenever you're finished giving review feedback. (The next several ones are based on top of this but are otherwise ready to submit.) As @eklimo mentioned, we've already had a few rounds of review before creating these, but want to get your early validation of the overall organization, class structure, etc.

I called at out least two points to look at. Also, some complexity arises in ApexTreeBuilder. Because I'd say that Summit is marginally "closer" to the parse tree representation than Jorje was, some minor restructuring of the representation by Jorje now effectively happens here.

oowekyala · 2022-09-23T18:31:15Z

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

+@Deprecated("internal")
+@InternalApi
+@Suppress("DEPRECATION")
+class ApexTreeBuilder(val sourceCode: String, val parserOptions: ApexParserOptions) {


I'm concerned that your converting existing classes to kotlin will make the merge conflicts with pmd 7 much harder to handle. The java version of this class has been changed in pmd 7 and will conflict. I think it's fine to use kotlin for new classes but I see no solid reason to convert existing classes to kotlin when they already work - at least while we have three parallel development branches.

This class serves the same functional purpose but is essentially a wholesale rewrite. (The input is obviously different, but the general structure changed to handle the many:many relation between the two ASTs.)

Any concerns about specific common pieces that we might want to factor out and share? I can rework to preserve those in ApexTreeBuilder and move the Summit-specific bits to a new class.

aaronhurst-google · 2022-09-23T18:44:18Z

@zorzella

adangel · 2022-10-06T06:56:44Z

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

@@ -4,6 +4,7 @@

 package net.sourceforge.pmd.lang.apex.ast;

+import com.google.summit.ast.CompilationUnit;


Nit: That's probably an unused import (like in the other test classes).

adangel · 2022-10-06T07:57:32Z

I'm going to merge this now into experimental-apex-parser.

As this is for sure not the last change - we still have the ability to tackle the mentioned issues (e.g. ApexTreeParser and compatibility with PMD 7). I'd defer these issues until later.

aaronhurst-google · 2022-10-11T00:27:40Z

As this is for sure not the last change - we still have the ability to tackle the mentioned issues (e.g. ApexTreeParser and compatibility with PMD 7). I'd defer these issues until later.

Thanks. I've created a doc "PMD Apex Front-end Tracker" to capture and track some of the tasks that span beyond a specific CL but need to be addressed before completion. I've shared with @adangel and @oowekyala and will keep it up-to-date (but feel free to comment / edit).

eklimo commented Aug 15, 2022

View reviewed changes

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

oowekyala changed the title ~~Summit-AST Apex module - Part 1~~ {apex] Summit-AST Apex module - Part 1 Aug 17, 2022

oowekyala changed the title ~~{apex] Summit-AST Apex module - Part 1~~ [apex] Summit-AST Apex module - Part 1 Aug 17, 2022

oowekyala changed the base branch from experimental-apex-parser to pmd/7.0.x August 17, 2022 12:34

oowekyala changed the base branch from pmd/7.0.x to experimental-apex-parser August 17, 2022 12:34

eklimo added 10 commits August 18, 2022 06:23

Replace Jorje with Summit-AST

42f4a09

- Remove Jorje dependencies - Add Summit-AST dependency - Comment out code until compilation succeeds

Deprecate ApexNode type parameter

1c01ee0

- Replace `ApexNode` type arguments with `Void` - Remove `ApexNode.getNode` - Remove `ASTCommentContainer` type parameter - Deprecate `ASTUserClassOrInterface` type parameter

Invoke SummitAST from ApexParser

4181043

- Add call to `SummitAST` in `ApexParser` - Fix NPE involving `suppressMap` to allow `ApexTreeBuilder` to run - Remove unused `TopLevelVisitor`

Add Kotlin compilation to Apex module

2c32d5d

Set top-level Apex node to TypeDeclaration

a32f8c2

Replace `CompilationUnit` with `TypeDeclaration` as the top-level node to match the AST produced by Jorje.

Add new ApexTreeBuilder

0557c22

- Replace `ApexTreeBuilder.java` with `ApexTreeBuilder.kt` - Set up tree builder foundation - Build `TypeDeclaration` nodes

Build declaration modifiers

88e1875

- Add `AbstractApexNode.Many`

Build class/interface declarations

bb70984

- Add `buildChildren` function to tree builder

Call additional methods in separate pass

f1a09b9

eklimo force-pushed the part-1 branch from 098302c to f1a09b9 Compare August 18, 2022 06:52

eklimo commented Aug 18, 2022

View reviewed changes

pom.xml Show resolved Hide resolved

oowekyala force-pushed the experimental-apex-parser branch from 8a4547d to 2977bd8 Compare August 18, 2022 16:43

adangel reviewed Aug 24, 2022

View reviewed changes

pmd-apex/src/main/java/net/sourceforge/pmd/lang/apex/ApexJorjeLogging.java Show resolved Hide resolved

pom.xml Show resolved Hide resolved

Merge remote-tracking branch 'upstream/experimental-apex-parser' into…

ba9d48f

… part-1 Change-Id: I254208dd5d1b9dbf5335a59a51372ec1d55f8535

aaronhurst-google reviewed Sep 23, 2022

View reviewed changes

oowekyala reviewed Sep 23, 2022

View reviewed changes

adangel reviewed Oct 6, 2022

View reviewed changes

adangel mentioned this pull request Oct 6, 2022

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

Closed

16 tasks

adangel merged commit a06340d into pmd:experimental-apex-parser Oct 6, 2022

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 1 #4093

[apex] Summit-AST Apex module - Part 1 #4093

eklimo commented Aug 15, 2022 •

edited

Loading

eklimo commented Aug 18, 2022

oowekyala commented Aug 18, 2022

adangel left a comment

adangel commented Sep 1, 2022

aaronhurst-google commented Sep 14, 2022

adangel commented Sep 15, 2022

aaronhurst-google Sep 23, 2022

adangel Oct 6, 2022

aaronhurst-google Oct 11, 2022

aaronhurst-google Sep 23, 2022

aaronhurst-google commented Sep 23, 2022

oowekyala Sep 23, 2022

aaronhurst-google Sep 23, 2022

aaronhurst-google commented Sep 23, 2022

adangel Oct 6, 2022

adangel commented Oct 6, 2022

aaronhurst-google commented Oct 11, 2022

		@@ -4,6 +4,7 @@

		package net.sourceforge.pmd.lang.apex.ast;

		import com.google.summit.ast.CompilationUnit;

[apex] Summit-AST Apex module - Part 1 #4093

[apex] Summit-AST Apex module - Part 1 #4093

Conversation

eklimo commented Aug 15, 2022 • edited Loading

Describe the PR

Changes

Upcoming changes

Related issues

eklimo commented Aug 18, 2022

oowekyala commented Aug 18, 2022

adangel left a comment

Choose a reason for hiding this comment

adangel commented Sep 1, 2022

aaronhurst-google commented Sep 14, 2022

adangel commented Sep 15, 2022

aaronhurst-google Sep 23, 2022

Choose a reason for hiding this comment

adangel Oct 6, 2022

Choose a reason for hiding this comment

aaronhurst-google Oct 11, 2022

Choose a reason for hiding this comment

aaronhurst-google Sep 23, 2022

Choose a reason for hiding this comment

aaronhurst-google commented Sep 23, 2022

oowekyala Sep 23, 2022

Choose a reason for hiding this comment

aaronhurst-google Sep 23, 2022

Choose a reason for hiding this comment

aaronhurst-google commented Sep 23, 2022

adangel Oct 6, 2022

Choose a reason for hiding this comment

adangel commented Oct 6, 2022

aaronhurst-google commented Oct 11, 2022

eklimo commented Aug 15, 2022 •

edited

Loading