[java] Rework AccessNode#2259
Conversation
Introduce a ModifierList node, that ranges over the modifiers of a declaration (including annotations). This is a combination of a few old commits: Figure out modifiers Fix tests Remove AccessTypeNode Document Remove specific methods Fix symboltable test Fix tests Rename to JModifier Fix copypaste default/abstract Improve doc Test anon classes Remove useless impl Static modifier should not be present on toplevel classes Simplify impl Add visibility enum Port some tests Fix test ranges Fix modifier ordering Cleanup Fix unnecessary modifier rule Rename to use plural Improve visibility doc Simplify some things Checkstyle Remove some usages of typekind Fix missing import Remove irrelevant method REmove some duplication Replace AccessNode with ModifierOwner Rename to AccessNode to reduce diff Remove changes to rules Add convenience methods Make VariableDeclaratorId a ModifierOwner Fix variable name decl Make enum constant an implicit AccessNode Fix compil Checkstyle Cleanup Deprecate TypeKind Cleanup Remove TypeKind Revert "Remove TypeKind" This reverts commit 222c169c3401a01507726f339ae9f9b2b20dc69a. Fix doc Fix UnnecessaryModifierRule Use special node instead of ModifierSet Remove useless tests Fix tests WIP Work should be resumed when pmd#2211 is merged into java-grammar Fix some tests Doc
Javacc nodes now use this convention, and since this PR updates the test module to take implicit nodes into account, the scala module needs to be ported too. Refs pmd#2021
Generated by 🚫 Danger |
adangel
left a comment
There was a problem hiding this comment.
Looks good. 👍
Two points:
- Does this solve/address #1307 already?
- We probably should add an example to https://github.com/pmd/pmd/wiki/Java_clean_changes to show, how the modifiers are now represented in the AST (via a own virtual node).
| return super.isPublic(); | ||
| } | ||
|
|
||
| public boolean isSyntacticallyStatic() { |
There was a problem hiding this comment.
Is there anything we can provide on master? The methods isSyntactically* will be gone, but deprecation doesn't make sense IMHO unless we can tell, what to do instead....
Not really, though it gives a clearer path to address it. Basically, the point of #1307 is that nodes like FormalParameter should not have methods like isPublic/isSynchronized, which are meaningless. This PR gives an ASTModifierList to every "declaration" node, including FormalParameter, LocalVariableDeclaration, etc. So it looks like it has exactly the same problem... Except we don't have to keep the method So, to me, #1307 will be fixed when those methods of AccessNode are removed, and their usages replaced with calls to Note that it probably would be nice to keep shorthands like isPublic, isPrivate, for the nodes for which it makes sense. We don't need those in the XPath API though, and they could be tagged with About migration from master: We could move Ideally in the XPath API, AccessNodes have the attributes |
We also need to update https://github.com/pmd/pmd/wiki/Java_clean_changes#annotation-nesting because now annotations are a child of the ModifierList node |
isSyntactically*on ASTFieldDeclaration, ASTMethodDeclaration, which were inconsistent and don't account for all modifiers. Now you can just call egnode.hasExplicitModifier(JModifier.PUBLIC). Things likefield.isInterfaceMember()can be replaced withfield.getEnclosingType().isInterface().The implementation of AST-based symbols depends on this PR.