Skip to content

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

Merged
michaelnebel merged 7 commits into
github:mainfrom
michaelnebel:modelgenrespectmanual
Jan 12, 2024
Merged

C#/Java: Increase precision of model generation.#15179
michaelnebel merged 7 commits into
github:mainfrom
michaelnebel:modelgenrespectmanual

Conversation

@michaelnebel

@michaelnebel michaelnebel commented Dec 20, 2023 •
edited
Loading

Copy link
Copy Markdown
Contributor

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

@owen-mc owen-mc 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.

Java LGTM

owen-mc
owen-mc reviewed Jan 10, 2024
View reviewed changes

@owen-mc owen-mc 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.

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

michaelnebel commented Jan 11, 2024 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

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
Comment thread 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.

@owen-mc owen-mc Jan 12, 2024

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.

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

owen-mc
owen-mc reviewed Jan 12, 2024
View reviewed changes
Comment thread 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()

@owen-mc owen-mc Jan 12, 2024

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.

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.

@michaelnebel michaelnebel Jan 12, 2024

Copy link
Copy Markdown
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
Comment thread java/ql/test/utils/modelgenerator/dataflow/p/MultipleImpl2.java
@@ -0,0 +1,20 @@
package p;

class MultipleImpl2 {

@owen-mc owen-mc Jan 12, 2024

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.

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

owen-mc commented Jan 12, 2024

Copy link
Copy Markdown
Contributor

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

michaelnebel commented Jan 12, 2024

Copy link
Copy Markdown
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

michaelnebel commented Jan 12, 2024 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

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

@michaelnebel
michaelnebel merged commit 9becd08 into github:main Jan 12, 2024
@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