Skip to content

[apex] Make apex have a single RootNode #2491

Merged
oowekyala merged 8 commits into
pmd:pmd/7.0.xfrom
oowekyala:apex-single-rootnode-2
Jun 14, 2020
Merged

[apex] Make apex have a single RootNode #2491
oowekyala merged 8 commits into
pmd:pmd/7.0.xfrom
oowekyala:apex-single-rootnode-2

Conversation

@oowekyala

Copy link
Copy Markdown
Member

Describe the PR

  • Introduce ASTApexFile as the RootNode for apex trees. The problem is that the contract of RootNode mandates a single RootNode instance per tree, but with nested classes/interfaces, this is not respected by Apex if eg ASTUserClass/Interface implement RootNode
  • I also simplified the apex CompilerService, most of which is useless copy-paste

Another unrelated change, is that CanSuppressWarning is now removed in Apex. This aligns apex annotation suppression with the java module (on java-grammar), see #1927

Related issues

Ready?

  • Added unit tests for fixed bug/feature
  • Passing all unit tests
  • Complete build ./mvnw clean verify passes (checked automatically by travis)
  • Added (in-code) documentation (if needed)

@oowekyala oowekyala added this to the 7.0.0 milestone May 18, 2020
@oowekyala oowekyala mentioned this pull request May 21, 2020
6 tasks
@ghost

ghost commented May 21, 2020

Copy link
Copy Markdown
1 Warning
⚠️ This PR cannot be merged yet.
✅ Nice work.
No java rules are changed!

Generated by 🚫 Danger

@oowekyala

Copy link
Copy Markdown
Member Author

@adangel FYI I'm going to merge this soon as this is blocking #2490

@oowekyala oowekyala self-assigned this Jun 14, 2020
oowekyala added a commit that referenced this pull request Jun 14, 2020
@oowekyala oowekyala merged commit 0861378 into pmd:pmd/7.0.x Jun 14, 2020
@oowekyala oowekyala deleted the apex-single-rootnode-2 branch June 19, 2020 14:44
@adangel adangel added the in:ast About the AST structure or API, the parsing step label Jun 20, 2020
/**
* Adapter for the Apex jorje parser
*/
public class ApexParser extends AbstractParser {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change effectively moves ApexParser from package lang.apex into lang.apex.ast.
I guess, we should internalize this one on master, as we did with the JavaLanguageParser.

Done in 23c08b5

Comment on lines -134 to -140
for (Object element : nodes) {
if (element instanceof ASTUserClass) {
visit((ASTUserClass) element, ctx);
} else if (element instanceof ASTUserInterface) {
visit((ASTUserInterface) element, ctx);
} else if (element instanceof ASTUserTrigger) {
visit((ASTUserTrigger) element, ctx);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On master, in seems we have a bug: we don't visit top-level ASTUserEnum and ASTAnonymousClass...

Enums in Apex can't have additional user defined methods, so they only define the constants. This means, only rules like Naming conventions would suffer from this bug.
Just tested it: this rule uses rulechain, so we directly visit UserEnum - that works. The tests look like, they were written before introducing rule chain, since they don't have a top-level enum...

ASTAnonymousClass - hm... a anonymous class can't be top-level. So, this should be no issue at all... In fact, I have no clue, how to write an anonymous class in Apex....

This is addressed in #2610

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

Labels

in:ast About the AST structure or API, the parsing step

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants