Skip to content

Conversation

MaxAtoms
Copy link
Contributor

This adds support for variable references to local variables declared in type patterns (spoon.reflect.reference.CtLocalVariableReference#getDeclaration).

Closes #6318

This approach only works for variables declared in a parent node.
Note that this does not reflect the actual flow scope rules the Java compiler uses.
@MaxAtoms
Copy link
Contributor Author

@monperrus What do you think of this PR thus far?
It all feels rather improvised to me.
Note that there are some failing tests showing the peculiarities of flow scope.

@monperrus
Copy link
Collaborator

I like the concept. You may document better the added code, esp in a big switch-style if/then/else.

If the existing tests encode bad behavior, we should change them, with good comments explaining the new behavior.

@SirYwell
Copy link
Collaborator

The approach seems good enough, although we might want to rewrite the whole logic at some point, also to not longer depend on the CtQuery stuff there.

How are type patterns in record patterns handled?

@MaxAtoms
Copy link
Contributor Author

@SirYwell I'd be interested to hear your thoughts on how to better structure this logic.
It feels overly complicated for finding block scoped variables.
And for flow scope, the backwards search approach is kind of awkward.

@SirYwell
Copy link
Collaborator

@SirYwell I'd be interested to hear your thoughts on how to better structure this logic. It feels overly complicated for finding block scoped variables. And for flow scope, the backwards search approach is kind of awkward.

I assume that a "naive" implementation that doesn't try to re-use existing functionality (i.e., the CtQuery stuff here) would a) be easier to maintain/understand (if written in a clean way), and b) perform better, as we noticed CtQuery to have some overhead in other cases before.

It might make sense to only do a rewrite when we target Java 21, as pattern matching for switch sounds like a really good fit here. Making it more explicit which action is taken for which code element (instead of the abstract "search siblings before" for example) would make the semantics clearer, and for subsequent changes (e.g., when more patterns are added to the language) it would also be clearer which changes are needed there.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug]: LocalVariableReference does not link to declaration within Type Pattern
3 participants