Skip to content

Java: add class that represents callables that are interesting for MaD models #12838

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

jcogs33
Copy link
Contributor

@jcogs33 jcogs33 commented Apr 14, 2023

Description

This PR adds a ModelApi class which represents all callables for which we might be interested in having a MaD model. The original purpose of adding this class was to disconnect the manual vs automated coverage counting from the DataFlowTargetApi class since DataFlowTargetApi is too specific to the dataflow model generator.
The ModelApi class will also be useful for replacing some code used in the heuristics queries.
The SrcCallable class may also be useful for TypeFlow.qll.

More specifically, this PR does the following:

  • Adds a SrcCallable class with an isEffectivelyPublic predicate in Member.qll to approximate an effectively public callable in Java.
  • Creates a new ModelExclusions.qll file to centralize the exclusions needed to identify APIs that are interesting for MaD models, and creates a ModelApi class which uses those exclusions.
  • Refactors parts of CaptureModelsSpecific.qll and ExternalApi.qll to use the new ModelExclusions.qll file.
  • Updates the GeneratedVsManualCoverage metrics query to use ModelApi instead of DataFlowTargetApi, and adjusts the TopJdkApisTest for this query accordingly.

Consideration

  • Any input on better class/predicate/file names and QLDocs is welcome, especially for the following:
    • Anything in Member.qll (Note: the isEffectivelyPublic name was chosen to align with C#'s isEffectivelyPublic predicate name.)
    • Is ModelExclusions.qll a good name or is there something better that can be used?
    • Is ModelApi a good name or is there something better that can be used?
  • Test libraries were previously not excluded from the model generator code, but they now will be excluded due to the following line in isUninterestingForModels: c.getDeclaringType() instanceof TestLibrary. Let me know if this exclusion is incorrect for the model generator.
  • Should I include a change note for the additions in Member.qll? Or are these trivial enough to not need a change note?

@github-actions github-actions bot added the Java label Apr 14, 2023
@jcogs33 jcogs33 force-pushed the jcogs33/add-class-for-callables-interesting-for-modeling branch from 9d301be to f139610 Compare April 19, 2023 02:05
@jcogs33 jcogs33 marked this pull request as ready for review April 19, 2023 12:18
@jcogs33 jcogs33 requested a review from a team as a code owner April 19, 2023 12:18
@michaelnebel
Copy link
Contributor

@jcogs33 Thank you for the very good PR description!!!

michaelnebel
michaelnebel previously approved these changes Apr 20, 2023
Copy link
Contributor

@michaelnebel michaelnebel left a comment

Choose a reason for hiding this comment

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

This looks good to me! Great work Jami!

@jcogs33 jcogs33 force-pushed the jcogs33/add-class-for-callables-interesting-for-modeling branch from f139610 to 8554263 Compare April 20, 2023 20:24
@jcogs33
Copy link
Contributor Author

jcogs33 commented Apr 20, 2023

Thanks for the review @michaelnebel! I rebased to get the update from #12851.

@aschackmull When you get a chance, can you review and let me know if you think there should be any changes, especially to Member.qll. 🙏

Copy link
Contributor

@aschackmull aschackmull left a comment

Choose a reason for hiding this comment

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

I've adjusted the qldoc a bit. Member.qll LGTM now.

@aschackmull
Copy link
Contributor

  • Should I include a change note for the additions in Member.qll? Or are these trivial enough to not need a change note?

No need, I think.

@jcogs33 jcogs33 added the no-change-note-required This PR does not need a change note label Apr 25, 2023
@jcogs33 jcogs33 merged commit cff7f63 into github:main Apr 25, 2023
@jcogs33 jcogs33 deleted the jcogs33/add-class-for-callables-interesting-for-modeling branch April 25, 2023 13:29
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.

5 participants