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.

Sign up for GitHub

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

Jump to bottom

C#/Java: Increase precision of model generation. #15179

Merged
merged 7 commits into from
Jan 12, 2024
Merged

C#/Java: Increase precision of model generation. #15179

merged 7 commits into from
Jan 12, 2024

Conversation

michaelnebel
Copy link
Contributor

@michaelnebel michaelnebel commented Dec 20, 2023 •
edited
Loading

This PR includes a couple of changes to the model generator.

  • For Java and C# we no longer generate any models, if there already exists a manual summary or neutral summary model (This was already the case for C# if a manual summary model existed).
  • For Java we don't generate neutral summaries and summaries for the same callable (this could happen if two APIs would disagree on whether the "super implementation" (lifted API) would exhibit flow).

@michaelnebel michaelnebel force-pushed the modelgenrespectmanual branch 3 times, most recently from ce23bd0 to 0f05504 Compare January 5, 2024 10:33
@michaelnebel michaelnebel changed the title C#/Java: Only generate models if there doesn't exist manual summary or neutral summary model. C#/Java: Increase precision of model generation. Jan 5, 2024
@michaelnebel michaelnebel force-pushed the modelgenrespectmanual branch from 0f05504 to d91e2c6 Compare January 10, 2024 08:05
@michaelnebel michaelnebel marked this pull request as ready for review January 10, 2024 10:29
@michaelnebel michaelnebel requested review from a team as code owners January 10, 2024 10:29
@michaelnebel michaelnebel added the no-change-note-required This PR does not need a change note label Jan 10, 2024
owen-mc
owen-mc previously approved these changes Jan 10, 2024
View reviewed changes
Copy link
Contributor

@owen-mc owen-mc left a comment

Choose a reason for hiding this comment

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

Java LGTM

owen-mc
owen-mc reviewed Jan 10, 2024
View reviewed changes
Copy link
Contributor

@owen-mc owen-mc left a comment

Choose a reason for hiding this comment

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

Actually, for java, is this only considering default method bodies provided by interfaces? Because I think we want methods of any class implementing an interface. I've asked @jcogs33 to clarify.

@michaelnebel michaelnebel force-pushed the modelgenrespectmanual branch from d91e2c6 to 945bf78 Compare January 11, 2024 10:29
@michaelnebel michaelnebel requested a review from owen-mc January 11, 2024 12:11
@michaelnebel
Copy link
Contributor Author

michaelnebel commented Jan 11, 2024 •
edited
Loading

DCA
Java: (1) Performance looks good (2) I spot checked the diff in the produced models and it looks as expected.
C#: (1) Performance looks good (2) Nothing in production is affected.

owen-mc
owen-mc reviewed Jan 12, 2024
View reviewed changes
java/ql/src/utils/modelgenerator/internal/CaptureSummaryFlowQuery.qll Outdated
@@ -77,6 +77,6 @@ string captureFlow(DataFlowTargetApi api) {
* A neutral model is generated, if there does not exist any summary model.
Copy link
Contributor

Choose a reason for hiding this comment

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

Please update this QLDoc to describe the new logic that you've added.

owen-mc
owen-mc reviewed Jan 12, 2024
View reviewed changes
java/ql/src/utils/modelgenerator/internal/CaptureModelsSpecific.qll
if exists(superImpl(api))
then superImpl(api).fromSource() and result = superImpl(api).getDeclaringType()
else result = api.getDeclaringType()
result = api.lift().getDeclaringType()
Copy link
Contributor

Choose a reason for hiding this comment

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

Just to check that I've understood correctly: this change does nothing, but the logic in lift has been extracted to a predicate so it can be used somewhere else too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Correct.

owen-mc
owen-mc reviewed Jan 12, 2024
View reviewed changes
java/ql/test/utils/modelgenerator/dataflow/p/MultipleImpl2.java
@@ -0,0 +1,20 @@
package p;

class MultipleImpl2 {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it would be helpful for future readers of the code to add a comment explaining what intended behaviour this test is testing.

@owen-mc
Copy link
Contributor

owen-mc commented Jan 12, 2024

It isn't immediately obvious to me what the reasoning is for not generating a neutral summary when the default implementation of an interface method is trivial.

@michaelnebel
Copy link
Contributor Author

It isn't immediately obvious to me what the reasoning is for not generating a neutral summary when the default implementation of an interface method is trivial.

Let's remove that again. The stuff we wanted was to avoid creating neutral summary models, if at least one implementation exhibited flow.
I could imagine someone using the (anti) pattern of defining empty/almost empty defaults in an interface with the expectation that "if logic in this area is needed - then implement". However, since this hasn't been observed as a pattern - then let us remove it again.

@michaelnebel michaelnebel force-pushed the modelgenrespectmanual branch from 19de2d5 to 37a21ec Compare January 12, 2024 12:36
@michaelnebel
Copy link
Contributor Author

michaelnebel commented Jan 12, 2024 •
edited
Loading

Review comments addressed, rebased the PR to remove the undesired parts, description updated.

@michaelnebel michaelnebel merged commit 9becd08 into github:main Jan 12, 2024
34 checks passed
@michaelnebel michaelnebel deleted the modelgenrespectmanual branch January 12, 2024 14:12
@owen-mc owen-mc mentioned this pull request Jan 14, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@owen-mc owen-mc owen-mc approved these changes

Assignees
No one assigned
Labels
C# 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.
2 participants
@michaelnebel @owen-mc