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

Support Locating AST elements by Line/Column #41

Closed
wants to merge 1 commit into from

Conversation

JLLeitschuh
Copy link
Collaborator

@JLLeitschuh JLLeitschuh commented Dec 15, 2023

This allows for finding AST nodes that are flagged by other scanners to be identified within the OpenRewrite Java AST.

Unfortunately, since each printer is unique to each language, this logic as it is currently is not multi-language

Signed-off-by: Jonathan Leitschuh [email protected]

@JLLeitschuh JLLeitschuh force-pushed the feat/JLL/line_column_locator branch 2 times, most recently from 68d55cc to ceadb4f Compare December 15, 2023 02:55
@JLLeitschuh
Copy link
Collaborator Author

@timtebeek is this something that would be desired in rewrite-java or is this the appropriate home for it?

@knutwannheden
Copy link
Contributor

I think this could go into rewrite-java. What is your use case for this?

@knutwannheden
Copy link
Contributor

Also, I think if this is something we may also want for other source types, that would be another candidate for something like a SourceFileService as in the following PR: openrewrite/rewrite#3817

Up until now I have been reluctant to add this, because we are still lacking use cases for it.

@JLLeitschuh
Copy link
Collaborator Author

What is your use case for this?

Matching up OpenRewrite AST nodes with those from security scanners, for examples SARIF files generated by CodeQL

@JLLeitschuh
Copy link
Collaborator Author

Do you have a better name for this utility than CoordinateLocator? LineColumnLocator was another name I considered. Thoughts?

@knutwannheden
Copy link
Contributor

Coordinate is insofar a bit unfortunate, as this term is already used in a different situation. Maybe something with "source file offset" or "element" (referring to the LST element)? But naming things isn't my strength either 😄

@jkschneider
Copy link
Member

Indeed, we already have an OpenRewrite concept of Coordinates, so the naming overlap is not good. In the IDE it is called "Go to Line" but also involves a column. So GoToLine seems reasonable to me here as well.

image

Please don't have the public API signature return FJ types. We are trying to get away from FJ.

@JLLeitschuh
Copy link
Collaborator Author

JLLeitschuh commented Dec 15, 2023

Please don't have the public API signature return FJ types.

Good point. I think the method type singnature was generated by GitHub copilot and I just went with it, I should have spotted that change. Will change types

@timtebeek timtebeek added the enhancement New feature or request label Dec 15, 2023
@JLLeitschuh
Copy link
Collaborator Author

Should finding AST elements by line/column be in core or in rewrite-java? This seems like it's likely something that we'd want to be available to all languages, eventually. But maybe starting with a smaller scope initially is better? @jkschneider what are your thoughts?

@JLLeitschuh
Copy link
Collaborator Author

Superseded by: openrewrite/rewrite#3829

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.

None yet

4 participants