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.

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

Add a ScopedVariable trait #604

Closed
wants to merge 5 commits into from
Closed

Add a ScopedVariable trait #604

wants to merge 5 commits into from

Conversation

Laurens-W
Copy link
Contributor

What's changed?

Added a trait to pull together the scope and identifier of NamedVariables and added testcases to showcase its usage

Anyone you would like to review specifically?

@sambsnyd @timtebeek

Checklist

  • I've added unit tests to cover both positive and negative cases
  • I've read and applied the recipe conventions and best practices
  • I've used the IntelliJ IDEA auto-formatter on affected files

@Laurens-W Laurens-W added the enhancement New feature or request label Nov 15, 2024
@Laurens-W Laurens-W self-assigned this Nov 15, 2024
Laurens-W and others added 3 commits November 15, 2024 14:07
…e.java

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
import org.openrewrite.trait.Trait;

@Value
public class ScopedVariable implements Trait<J.VariableDeclarations.NamedVariable> {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't we instead create this trait in the rewrite repo?

protected @Nullable ScopedVariable test(Cursor cursor) {
if (cursor.getValue() instanceof J.VariableDeclarations.NamedVariable) {
J.VariableDeclarations.NamedVariable variable = cursor.getValue();
return new ScopedVariable(cursor, variable.getDeclaringScope(cursor), variable.getName());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This getDeclaringScope() utility method is probably a good starting point. But to me it looks like it misses cases like try-with-resources and exception handlers, for example. Also note that the flow scoping of instanceof patterns is quite different and probably a bad fit. Maybe worth thinking about at least.


@Override
protected @Nullable ScopedVariable test(Cursor cursor) {
if (cursor.getValue() instanceof J.VariableDeclarations.NamedVariable) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can likely just use variable.getDeclaringScope(cursor) to obtain the variable's scope; using a trait might be overkill.

A scenario where a trait could be useful, suppose I'm at an identifier and need to determine the declaring scope of the variable that this identifier references.

void foo() {
   int x = 5;
   if (true) {
      print(x) // find the declaring scope of x at this point,
   }
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ideally we'd pull the logic from variable.getDeclaringScope(cursor) into this trait.
Any recipe wanting to do something with variable scope or checking whether an identifier is of a variable that is in scope can create an inline visitor based on this trait, visit the top level tree element they're interested in and have a list of ScopedVariables

@Laurens-W
Copy link
Contributor Author

Continued here

@Laurens-W Laurens-W closed this Nov 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants