FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

Java: Support Argument[this] and parameters of bodiless interface methods in framework mode metadata extraction by kaeluka · Pull Request #13823 · github/codeql · GitHub

/ codeql Public

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

Merged
kaeluka merged 6 commits into
mainfrom
kaeluka/support-argument-this-in-frameworkmode-metadata-extraction
Jul 28, 2023
ConversationCommits (6)ChecksFindingsFiles changed
Merged

Java: Support Argument[this] and parameters of bodiless interface methods in framework mode metadata extraction#13823
kaeluka merged 6 commits into
mainfrom
kaeluka/support-argument-this-in-frameworkmode-metadata-extraction

Conversation

kaeluka commented Jul 26, 2023 •
edited
Loading

Copy link
Copy Markdown
  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 Bot added the Java label Jul 26, 2023
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 force-pushed the kaeluka/support-argument-this-in-frameworkmode-metadata-extraction branch from f56aff4 to b5862ae Compare July 27, 2023 15:23
kaeluka marked this pull request as ready for review July 28, 2023 07:36
kaeluka requested a review from a team as a code owner July 28, 2023 07:36
kaeluka marked this pull request as draft July 28, 2023 07:36
Stephan Brandauer added 2 commits July 28, 2023 12:55
kaeluka force-pushed the kaeluka/support-argument-this-in-frameworkmode-metadata-extraction branch from b5862ae to 8ed773b Compare July 28, 2023 11:31
kaeluka marked this pull request as ready for review July 28, 2023 14:04

kaeluka commented Jul 28, 2023

Copy link
Copy Markdown
Author

@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 added the no-change-note-required This PR does not need a change note label Jul 28, 2023
atorralba approved these changes Jul 28, 2023
View reviewed changes

atorralba left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

As discussed, LGTM :)

kaeluka merged commit 40eab18 into main Jul 28, 2023
kaeluka deleted the kaeluka/support-argument-this-in-frameworkmode-metadata-extraction branch July 28, 2023 15:38
kaeluka pushed a commit that referenced this pull request Jul 28, 2023
Java: update text expectations after merging #13823
279b36f
kaeluka pushed a commit that referenced this pull request Jul 28, 2023
Java: Drive-by: fix oversight in #13823
2c20ec4 
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 pushed a commit that referenced this pull request Aug 1, 2023
Java: update text expectations after merging #13823
5ad984f
kaeluka pushed a commit that referenced this pull request Aug 1, 2023
Java: Drive-by: fix oversight in #13823
0582368 
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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer
atorralba atorralba approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Java no-change-note-required This PR does not need a change note

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

Back | FazBrowse Home | New Git URL