Skip to content

fix: LexicalPreservingPrinter can't print PrimitiveType #4817

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 1 commit into from
Closed

fix: LexicalPreservingPrinter can't print PrimitiveType #4817

wants to merge 1 commit into from

Conversation

seokjun7410
Copy link
Contributor

@seokjun7410 seokjun7410 commented Aug 18, 2025 •
edited
Loading

fixes. #4781

Issue Analysis

When adding primitive type fields via LexicalPreservingPrinter, keywords like int, boolean get omitted:

// Expected: class Foo { int foo; }
// Actual:   class Foo { foo; }    // 'int' missing

Root Cause

Dynamically created PrimitiveType nodes lack proper NodeText initialization. The existing prettyPrintingTextNode() relied on node.toString() which returns empty strings for uninitialized nodes.

My Solution: CSM-Based Architecture Approach

Removed Special Case Handling: Eliminated the hardcoded PrimitiveType switch statement in prettyPrintingTextNode() (38 lines removed).

Enhanced getOrCreateNodeText(): Added proper handling for dynamically created PrimitiveType nodes:

if (!node.hasRange() && node instanceof PrimitiveType) {
    interpret(node, ConcreteSyntaxModel.forClass(node.getClass()), nodeText);
} else {
    prettyPrintingTextNode(node, nodeText);
}

Made Primitive enum implement Stringable: For consistency with JavaParser's design patterns.

nstead of special-casing PrimitiveType, I leveraged JavaParser's existing CSM (ConcreteSyntaxModel) infrastructure which already knows how to render primitive types correctly.

Question

Does this CSM-based approach align well with JavaParser's lexical preservation architecture? I chose this over the simpler hardcoded string approach because it maintains consistency with how other AST nodes are handled.

@jlerbsc
Copy link
Collaborator

jlerbsc commented Aug 19, 2025

Your PR contains two separate changes. One is an interesting improvement to the existing code, and the other tries to fix a bug.

It would be more practical to separate the two types of changes, as I don't think the bug fix is very relevant.

My observation is that when propagating the change in the list of members (here the fields) of the class declaration, we try to modify the parent node of the list of fields (more precisely in the Node.setParentNode(...) method). According to my debugger, this is when the data field of the node to be added, which is supposed to contain the text form of the field declaration, is modified. Before the assignment ‘parentNode = newParentNode;’, my debugger indicates that the data field (of the node to be added) is null, and after the assignment, the data field is initialised, which subsequently disrupts lexical preservation. I don't know if this is a bug in the debugger or if it's a side effect related to changing the parent node of the node to be added. Perhaps you could confirm this analysis in your work environment.

@seokjun7410
Copy link
Contributor Author

Thank you for the insightful review. I'll debug the setParentNode() issue first and separate the changes into two PRs as you suggested.

@seokjun7410 seokjun7410 mentioned this pull request Aug 23, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@seokjun7410 @jlerbsc