Skip to content

Java: Support Argument[this] and parameters of bodiless interface methods in framework mode metadata extraction #13823

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

Conversation

kaeluka
Copy link
Contributor

@kaeluka kaeluka commented Jul 26, 2023

  1. There was an unintended limitation to only consider ExplicitParameterNodes in framework mode, but skipped the this parameter. This PR fixes the problem. I'll add a test case to the currently open test PR once this is merged.
  2. In framework mode we also skipped the parameters of interface methods without bodies — this PR fixes the problem as well. However, I'm not 100% sure this is the best way to fix this (it might have unintended consequences) and would appreciate thoughts from the Java team. We could, for instance, try to keep our changes to the DataFlow::ParameterNode types local to the automodel code (if needed).

@github-actions github-actions bot added the Java label Jul 26, 2023
@kaeluka kaeluka changed the title Java: Support Argument[this] in framework mode metadata extraction Java: Support Argument[this] and parameters of bodiless interface methods in framework mode metadata extraction Jul 27, 2023
@kaeluka kaeluka force-pushed the kaeluka/support-argument-this-in-frameworkmode-metadata-extraction branch from f56aff4 to b5862ae Compare July 27, 2023 15:23
@kaeluka kaeluka marked this pull request as ready for review July 28, 2023 07:36
@kaeluka kaeluka requested a review from a team as a code owner July 28, 2023 07:36
@kaeluka kaeluka marked this pull request as draft July 28, 2023 07:36
@kaeluka kaeluka force-pushed the kaeluka/support-argument-this-in-frameworkmode-metadata-extraction branch from b5862ae to 8ed773b Compare July 28, 2023 11:31
@kaeluka kaeluka marked this pull request as ready for review July 28, 2023 14:04
@kaeluka
Copy link
Contributor Author

kaeluka commented Jul 28, 2023

@atorralba — the one integration test that passed was a fluke, I think (I restarted it). With that, the PR is ready for review. Are you able to do that? You've seen most of the implementation already.

Thank you for your help, and also thanks to @aschackmull for the valuable input!

@kaeluka kaeluka added the no-change-note-required This PR does not need a change note label Jul 28, 2023
Copy link
Contributor

@atorralba atorralba left a comment

Choose a reason for hiding this comment

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

As discussed, LGTM :)

@kaeluka kaeluka merged commit 40eab18 into main Jul 28, 2023
@kaeluka kaeluka deleted the kaeluka/support-argument-this-in-frameworkmode-metadata-extraction branch July 28, 2023 15:38
kaeluka added a commit that referenced this pull request Jul 28, 2023
In PR #13823, we had rewritten the endpoints that are being considered for framework mode. We used to use `DataFlow::ParameterNode` as endpoints.
However, `ParameterNode`s do not exist for the implicit `this` parameter; they also do not exist for bodiless interface-methods.

In PR #13823, we forgot to model that `this` only exists for non-static methods and to only consider parameters that we have source code for.
kaeluka added a commit that referenced this pull request Aug 1, 2023
kaeluka added a commit that referenced this pull request Aug 1, 2023
In PR #13823, we had rewritten the endpoints that are being considered for framework mode. We used to use `DataFlow::ParameterNode` as endpoints.
However, `ParameterNode`s do not exist for the implicit `this` parameter; they also do not exist for bodiless interface-methods.

In PR #13823, we forgot to model that `this` only exists for non-static methods and to only consider parameters that we have source code for.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Java no-change-note-required This PR does not need a change note
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants