[kotlin] Add AST improvements, KotlinAstUtil#6670
Conversation
…es and KotlinAstUtil - KotlinNode: add default getIdentifier() and getModifiers() methods - KotlinInnerNode: implement getIdentifier(), getModifiers(), getImage(); add NonNullAttributeIterator to skip null XPath attributes in PMD Designer - KotlinTerminalNode: add null-attribute filter for getXPathAttributesIterator() - KotlinAstUtil: new static utility class for navigating Kotlin AST from rule code (getIdentifierText, getPrimaryExpressionText, typeContainsName, isDirectChildOfFunctionBody, isDirectDescendantOf, getLhsVarName, collectAllParamNames, collectLocalVarNames, collectClassVarFieldNames) - Tests: 5 new inline tests for AST attributes; new KotlinAstUtilTest with 8 tests
Detects local variable declarations and lambda explicit parameters that shadow a parameter of any enclosing named function. Visits each KtFunctionDeclaration once and collects a combined set of params from that function and all its enclosing named functions. Uses KotlinAstUtil.isWithin to scope checks so each declaration is processed exactly once by its nearest enclosing function visit. Also adds collectLambdaParamNames utility method to KotlinAstUtil. Test cases cover: basic shadowing, var (mutable), vararg params, destructuring (no false positive), extension functions, inner blocks, nested named functions, lambdas with explicit params.
The {0} placeholder in the rule message was not substituted because
addViolation was called without arguments. Pass the variable name as
the format argument.
Also add expected-messages assertion to the first test case so that
unformatted message placeholders cannot regress undetected.
… overloads; remove unused typeContainsName from KotlinAstUtil
|
Compared to main: (comment created at 2026-05-26 18:31:04+00:00 for 89fe9b8) |
adangel
left a comment
adangel
left a comment
There was a problem hiding this comment.
Thanks for the PR!
I' not a fan, that we add attributes on KotlinNode, that are not useful for all nodes and we need to filter them out later. This is just a manifestation of missing solution for #4338.
I spent now some time to implement a different solution, which I will push shortly to this PR.
The Java API for this feels a bit clunky, but I couldn't see a different way if we don't want to throw away the current Kotlin AST (-> #4337). This would be a bigger effort and in all possible ways incompatible (but hey - who uses pmd-kotlin anyway??)
Maybe someone sees a simpler, more elegant solution.
Let me know, what you think of this - it adds another level of indirection for accessing information from the AST (because we can't modify the AST classes).
I've seen, you sneaked in LocalVariableShadowsParameter. This is a new rule and not part of the migration effort of migrating Java rules to Kotlin. It is new - and it deserves its own PR + issue using the new-rule-issue-template.
This PR now does two things: Improve AST and add a new rule - too much IMHO.
Sure, the rule depends on the AST improvements (especially the KotlinAstUtil), so can be done only after. But that's how you do it: First improve the foundation, then use the foundation to do new things.
Please extract the rule into it's own PR.
I guess, we should rebase this PR in the end to have it clean.
Just started the Designer with the new changes. Here are the things, we need to solve:
- The default attributes (like Image, BeginLine, ...) are appearing twice: Once from the node itself and once of the attribute view (which mostly delegates).
- We need to add a DesignerBinding for some nodes
adangel
added
the
needs:user-input
AttributeView implements Node so PMD reflection picks up all delegated get* methods, causing duplicates when concatenating base and view attributes. Null-valued attributes (e.g. @modifiers with no modifiers) are also filtered out. Update tree dump reference files accordingly.
|
Thanks for the detailed review — and especially for the Addressed points: KotlinDesignerBindings added ( Duplicate and null XPath attributes fixed in three commits:
The Designer now no longer shows duplicate attribute entries, and attributes with a null value (like Regression tests added for no-null and no-duplicate invariants, covering inner nodes with and without an LocalVariableShadowsParameter — the rule was mainly there as a means to validate and discover the changes needed in the AST. Those insights have been turned into unit tests instead. Totally fine to keep PRs distinct; the rule will follow as a separate PR. |
stokpop
changed the title
adangel
added
an:enhancement
| * Returns the text of the first {@code SimpleIdentifier} direct child, | ||
| * or {@code null} if none is present. | ||
| */ | ||
| default String getIdentifier() { |
There was a problem hiding this comment.
It might make sense to actually name this attribute "Name" and not "Identifier". But this can be done later as well in a separate PR when we have rules (the attribute is experimental).
2 participants
Describe the PR
See issue #6669 for a full description of the three improvements: AST node improvements, the new
KotlinAstUtilutility class.Related issues
Ready?
./mvnw clean verifypasses (checked automatically by github actions)