Skip to content
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] Apex should only have a single RootNode #1937

Closed
Tracked by #2524
oowekyala opened this issue Jul 23, 2019 · 1 comment
Closed
Tracked by #2524

[apex] Apex should only have a single RootNode #1937

oowekyala opened this issue Jul 23, 2019 · 1 comment
Labels
in:ast About the AST structure or API, the parsing step
Milestone

Comments

@oowekyala
Copy link
Member

oowekyala commented Jul 23, 2019

Part of #2524

Several classes implement the RootNode interface in the Apex AST:

  • ASTAnonymousClass
  • ASTUserClass
  • ASTUserEnum
  • ASTUserInterface
  • ASTUserTrigger

The problem with that is that it doesn't respect the contract of the RootNode interface. Granted, the contract is not documented and the interface is rarely used in PMD, so there's not many occasions to find out implementation errors.

Anyway the unspoken contract for now is that there is only one RootNode instance per AST and that it's at the root of the tree. Eg DocumentNavigator relies on this assumption to identify the document node:

public boolean isDocument(Object arg0) {
return arg0 instanceof RootNode;
}

(This implementation is wrong for reasons outside the scope of this issue, see #1938)

To come back to Apex, the problem is that classes may be nested, so you may find a RootNode instance that is not the root of the tree. The simplest way to respect the contract is to introduce a new node, eg an ASTApexFile, that is at the top of any Apex AST, and to make all current ApexRootNodes unimplement the interface.


Rationale

I think for 7.0, it may be useful to flesh out the RootNode interface to use it to its full potential. Eg, having a method to provide the URI of the source file (or database resource, etc), the full text of the file (not hard to implement, already done for PLSQL), etc. There are also many places in PMD core where the only Nodes we expect are roots and we could maybe be stricter... #1426 uses the interface in that sense.

The point is, that those changes require a strong contract to the RootNode interface. Lastly, having a single implementation of RootNode per language model helps reduce code duplication.

oowekyala added a commit to oowekyala/pmd that referenced this issue Aug 29, 2019
oowekyala added a commit that referenced this issue Jan 10, 2020
@adangel adangel added this to the 7.0.0 milestone May 2, 2020
@adangel adangel mentioned this issue May 23, 2020
6 tasks
@adangel adangel added the has:pr label May 23, 2020
@adangel
Copy link
Member

adangel commented Jun 20, 2020

Fixed via #2491

@adangel adangel closed this as completed Jun 20, 2020
@adangel adangel added the in:ast About the AST structure or API, the parsing step label Jan 12, 2023
@adangel adangel mentioned this issue Jan 23, 2023
55 tasks
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

No branches or pull requests

2 participants