Skip to content

[java] Improve try-with-resources grammar#1897

Merged
adangel merged 9 commits into
pmd:java-grammarfrom
oowekyala:grammar-improve-resource
Aug 6, 2019
Merged

[java] Improve try-with-resources grammar#1897
adangel merged 9 commits into
pmd:java-grammarfrom
oowekyala:grammar-improve-resource

Conversation

@oowekyala

Copy link
Copy Markdown
Member
  • ASTResources is made void. There were two layers of nodes that represented the "resource list" abstraction, and that wasn't necessary
  • Rename ResourceSpecification to ResourceList (this is more consistent with some other *List nodes we have: ExtendsList, ArgumentList, ImplementsList, soon-to-be ThrowsList)
  • Add a LocalVariableDeclaration node inside Resource node, when they're not a concise resource
    • This is because Resource in that case has an identical API/ represents the same concept
    • This simplifies type resolution, since ClassTypeResolver can work on the VariableDeclarator's initializer just like in other LocalVariableDeclaration
  • Remove the workaround in OccurenceFinder. This worked around the fact that concise resources were just a Name, however, it's now an expression, so is handled by the rest of the class
  • Resource doesn't extend FormalParameter anymore, refs [java] AST inconsistencies around FormalParameter #998 point 3
  • Add an attribute to ResourceList to expose whether there was a trailing semicolon or not -> we can make a rule later to check that

@oowekyala oowekyala added the in:ast About the AST structure or API, the parsing step label Jul 1, 2019
@oowekyala oowekyala added this to the 7.0.0 milestone Jul 1, 2019

@adangel adangel left a comment

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.

So we renamed here ResourceSpecification into ResourceList. I don't remember, what we should do here in terms of documentation/API compatibility... we can't deprecate ResourceSpecification on PMD6 since the successor is only in PMD7 available. Maybe just mention it in the javadoc, like for the nodes that will be gone?

Comment thread pmd-java/etc/grammar/Java.jjt Outdated
Comment thread pmd-java/etc/grammar/Java.jjt Outdated
Rename ResourceSpecification production to ResourceList,
remove Resources production
@oowekyala

Copy link
Copy Markdown
Member Author

@adangel I'm not sure what mentioning this in the javadocs right now would achieve... Maybe we should just think about this when we're done updating the grammar. We're keeping a tidy list so that seems easy

@adangel

adangel commented Aug 6, 2019

Copy link
Copy Markdown
Member

I'm not sure what mentioning this in the javadocs right now would achieve... Maybe we should just think about this when we're done updating the grammar. We're keeping a tidy list so that seems easy

Agreed. Let's just collect the removed nodes in the wiki. It's maybe better suited for a later "Upgrade guide".

@adangel adangel self-assigned this Aug 6, 2019
@adangel adangel merged commit ec57361 into pmd:java-grammar Aug 6, 2019
@adangel

adangel commented Aug 6, 2019

Copy link
Copy Markdown
Member

Merged and added a section here: https://github.com/pmd/pmd/wiki/Java_clean_changes#improve-try-with-resources-grammar

@oowekyala oowekyala deleted the grammar-improve-resource branch August 6, 2019 15:12
@adangel adangel mentioned this pull request 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

Successfully merging this pull request may close these issues.

2 participants