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.

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

We probably don't handle recent Java features #303

Closed
cpovirk opened this issue Feb 25, 2022 · 1 comment
Closed

We probably don't handle recent Java features #303

cpovirk opened this issue Feb 25, 2022 · 1 comment
Assignees
@eamonnmcmanus
Labels
P3 type=defect Bug, not working as expected

Comments

@cpovirk
Copy link
Member

cpovirk commented Feb 25, 2022

Given that cb6486c only recently added support for try-with-resources, I'd assume that we aren't yet handling instanceof String s or switch expressions + yield, much less any preview features. (I pulled this list from the Java 17 TreeVisitor API.)
@cpovirk cpovirk added type=defect Bug, not working as expected P3 labels Feb 25, 2022
@eamonnmcmanus
Copy link
Member

I think TreeDiffer.DiffVisitor is quite problematic as constructed. It extends SimpleTreeVisitor and does not override defaultAction, which means that the default behaviour for a visitor method it doesn't override is to do nothing. That includes methods that are already present in the JDK 8 API, specifically visitAnnotatedType, meaning that differences between annotated types are not currently noticed, even if both the annotations and the type are different.

Since the treatment of most node types is quite formulaic, I think we might be able to replace this with a Proxy implementing TreeVisitor in a way that doesn't need the code to know about specific node types. That way we should be able to make it work with both old and new JDKs.

@eamonnmcmanus eamonnmcmanus self-assigned this Dec 3, 2022
copybara-service bot pushed a commit that referenced this issue Dec 4, 2022
@eamonnmcmanus
Use reflection to implement TreeDiffer.DiffVisitor.
e362d10 
This is much more robust than overriding individual visitor methods.
It means we won't forget to check some property of an AST node,
and it also means we automatically handle new kinds of nodes.

Using reflection does make this potentially more expensive than having individual visitor methods. The overhead is likely to be small compared to the cost of compiling source code to produce the ASTs in the first place.

Fixes #303.

RELNOTES=n/a
PiperOrigin-RevId: 492735035
@copybara-service copybara-service bot mentioned this issue Dec 4, 2022
Closed
copybara-service bot pushed a commit that referenced this issue Dec 4, 2022
@eamonnmcmanus
Use reflection to implement TreeDiffer.DiffVisitor.
0ea3504 
This is much more robust than overriding individual visitor methods.
It means we won't forget to check some property of an AST node,
and it also means we automatically handle new kinds of nodes.

Using reflection does make this potentially more expensive than having individual visitor methods. The overhead is likely to be small compared to the cost of compiling source code to produce the ASTs in the first place.

Fixes #303.

RELNOTES=n/a
PiperOrigin-RevId: 492735035
copybara-service bot pushed a commit that referenced this issue Dec 4, 2022
@eamonnmcmanus
Use reflection to implement TreeDiffer.DiffVisitor.
53d6025 
This is much more robust than overriding individual visitor methods.
It means we won't forget to check some property of an AST node,
and it also means we automatically handle new kinds of nodes.

Using reflection does make this potentially more expensive than having individual visitor methods. The overhead is likely to be small compared to the cost of compiling source code to produce the ASTs in the first place.

Fixes #303.

RELNOTES=n/a
PiperOrigin-RevId: 492735035
copybara-service bot pushed a commit that referenced this issue Dec 4, 2022
@eamonnmcmanus
Use reflection to implement TreeDiffer.DiffVisitor.
b2b351c 
This is much more robust than overriding individual visitor methods.
It means we won't forget to check some property of an AST node,
and it also means we automatically handle new kinds of nodes.

Using reflection does make this potentially more expensive than having individual visitor methods. The overhead is likely to be small compared to the cost of compiling source code to produce the ASTs in the first place.

Fixes #303.

RELNOTES=n/a
PiperOrigin-RevId: 492735035
copybara-service bot pushed a commit that referenced this issue Dec 4, 2022
@eamonnmcmanus
Use reflection to implement TreeDiffer.DiffVisitor.
d9f0c98 
This is much more robust than overriding individual visitor methods.
It means we won't forget to check some property of an AST node,
and it also means we automatically handle new kinds of nodes.

Using reflection does make this potentially more expensive than having individual visitor methods. The overhead is likely to be small compared to the cost of compiling source code to produce the ASTs in the first place.

Fixes #303.

RELNOTES=n/a
PiperOrigin-RevId: 492735035
copybara-service bot pushed a commit that referenced this issue Dec 4, 2022
@eamonnmcmanus
Use reflection to implement TreeDiffer.DiffVisitor.
a4a82e1 
This is much more robust than overriding individual visitor methods.
It means we won't forget to check some property of an AST node,
and it also means we automatically handle new kinds of nodes.

Using reflection does make this potentially more expensive than having individual visitor methods. The overhead is likely to be small compared to the cost of compiling source code to produce the ASTs in the first place.

Fixes #303.

RELNOTES=n/a
PiperOrigin-RevId: 492735035
copybara-service bot pushed a commit that referenced this issue Dec 12, 2022
@eamonnmcmanus
Use reflection to implement TreeDiffer.DiffVisitor.
6d314fb 
This is much more robust than overriding individual visitor methods.
It means we won't forget to check some property of an AST node,
and it also means we automatically handle new kinds of nodes.

Using reflection does make this potentially more expensive than having individual visitor methods. The overhead is likely to be small compared to the cost of compiling source code to produce the ASTs in the first place.

Fixes #303.

PiperOrigin-RevId: 492735035
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@eamonnmcmanus eamonnmcmanus

Labels
P3 type=defect Bug, not working as expected
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Use reflection to implement TreeDiffer.DiffVisitor. google/compile-testing
2 participants
@cpovirk @eamonnmcmanus