Shared: Model generator cleanup. #19311

michaelnebel · 2025-04-15T11:21:12Z

In this PR we

Re-factor the shared model generator implementation and put the heuristic queries in its own module.
Change the queries */utils/modelgenerator/summary-models to use the implementation from */utils/modelgenerator/mixed-summary-models and then delete the */utils/modelgenerator/mixed-summary-models queries. Also make the same change to the neutral capturing queries.
Remove the --with-mixed-* flags from the model generator python script.
Re-factor the model generator tests to use explicit summary model tags and change the neutral tests to use the combined neutral generation query.

This effectively means that when genering models with the GenerateFlowModels.py script using the flags --with-summaries and --with-neutrals now generates combined (or mixed as we used to call them) models based on both the content based and heuristic based data flow model generation.

Copilot

Pull Request Overview

This PR refactors the shared model generator by isolating heuristic queries into their own module, updating query annotations in test files, and removing legacy mixed-model queries and flags. Key changes include:

Replacing "summary" annotations with "heuristic-summary" in query comments across various test files.
Updating change note files to reflect the switch from mixed to combined (heuristic/content-based) models.
Removing deprecated flags from the model generator script.

Reviewed Changes

Copilot reviewed 49 out of 64 changed files in this pull request and generated no comments.

File	Description
java/ql/src/change-notes/2025-04-16-model-generation.md	Updated change note reflecting the query switch for Java models.
csharp/ql/test/utils/modelgenerator/dataflow/Summaries.cs	Replaced legacy "summary" tags with "heuristic-summary" where appropriate.
csharp/ql/src/change-notes/2025-04-16-model-generation.md	Updated change note reflecting the query switch for C# models.

Files not reviewed (15)

csharp/ql/src/utils/modelgenerator/CaptureMixedNeutralModels.ql: Language not supported
csharp/ql/src/utils/modelgenerator/CaptureMixedSummaryModels.ql: Language not supported
csharp/ql/src/utils/modelgenerator/CaptureNeutralModels.ql: Language not supported
csharp/ql/src/utils/modelgenerator/CaptureSinkModels.ql: Language not supported
csharp/ql/src/utils/modelgenerator/CaptureSourceModels.ql: Language not supported
csharp/ql/src/utils/modelgenerator/CaptureSummaryModels.ql: Language not supported
csharp/ql/src/utils/modelgenerator/debug/CaptureSummaryModelsPartialPath.ql: Language not supported
csharp/ql/src/utils/modelgenerator/debug/CaptureSummaryModelsPath.ql: Language not supported
csharp/ql/test/utils/modelgenerator/dataflow/CaptureHeuristicSummaryModels.ql: Language not supported
csharp/ql/test/utils/modelgenerator/dataflow/CaptureNeutralModels.ql: Language not supported
csharp/ql/test/utils/modelgenerator/dataflow/CaptureSinkModels.ql: Language not supported
csharp/ql/test/utils/modelgenerator/dataflow/CaptureSourceModels.ql: Language not supported
java/ql/src/utils/modelgenerator/CaptureMixedNeutralModels.ql: Language not supported
java/ql/src/utils/modelgenerator/CaptureMixedSummaryModels.ql: Language not supported
java/ql/src/utils/modelgenerator/CaptureNeutralModels.ql: Language not supported

Comments suppressed due to low confidence (2)

java/ql/src/change-notes/2025-04-16-model-generation.md:4

[nitpick] There appears to be a potential naming inconsistency: the change note references 'GenerateFlowModel.py', while the PR description uses 'GenerateFlowModels.py'. Consider standardizing the script name for clarity.

* Changes to the MaD model generation infrastructure: Changed the query `java/utils/modelgenerator/summary-models` to use the implementation from `java/utils/modelgenerator/mixed-summary-models`. Removed the now-redundant `java/utils/modelgenerator/mixed-summary-models` query. Similar replacement was made for `java/utils/modelgenerator/neutral-models`. That is, if `GenerateFlowModel.py` is provided with `--with-summaries` combined/mixed models are now generated instead of heuristic models (and similar for `--with-neutrals`).

csharp/ql/src/change-notes/2025-04-16-model-generation.md:4

[nitpick] There appears to be a potential naming inconsistency: the change note references 'GenerateFlowModel.py', while the PR description uses 'GenerateFlowModels.py'. Consider standardizing the script name for clarity.

* Changes to the MaD model generation infrastructure: Changed the query `cs/utils/modelgenerator/summary-models` to use the implementation from `cs/utils/modelgenerator/mixed-summary-models`. Removed the now-redundant `cs/utils/modelgenerator/mixed-summary-models` query. Similar replacement was made for `cs/utils/modelgenerator/neutral-models`. That is, if `GenerateFlowModel.py` is provided with `--with-summaries` combined/mixed models are now generated instead of heuristic models (and similar for `--with-neutrals`).

geoffw0

@paldepind FYI as you've been using this in Rust.

geoffw0 · 2025-04-17T11:06:52Z

rust/ql/src/change-notes/2025-04-16-model-generation.md

+---
+category: minorAnalysis
+---
+* Changes to the MaD model generation infrastructure: Changed the query `rust/utils/modelgenerator/summary-models` to use the implementation from `rust/utils/modelgenerator/mixed-summary-models`. Removed the now-redundant `rust/utils/modelgenerator/mixed-summary-models` query. Similar replacement was made for `rust/utils/modelgenerator/neutral-models`. That is, if `GenerateFlowModel.py` is provided with `--with-summaries` combined/mixed models are now generated instead of heuristic models (and similar for `--with-neutrals`).


We're not actually doing change notes for Rust yet, because we're still in preview - though the symmetry with other languages is nice and I can't see this one doing any harm.

paldepind · 2025-04-22T14:23:00Z

shared/mad/codeql/mad/modelgenerator/internal/ModelGeneratorImpl.qll

@@ -943,7 +1041,7 @@ module MakeModelGenerator<
   * 2. If content based flow does not yield any summary for an API, then we try and
   * generate flow summaries using the non-content based summary generator.


Might be nice to use the word "heuristic" here to refer to what is Heuristic::captureFlow?

Suggested change

* generate flow summaries using the non-content based summary generator.

* generate flow summaries using the heuristic summary generator.

Maybe it would also make sense to change the "heuristic" three lines above to a different word ("approach"?) as it's not the "heuristic" in Heuristic::captureFlow?

Good point!

paldepind · 2025-04-22T14:28:31Z

shared/mad/codeql/mad/modelgenerator/internal/ModelGeneratorImpl.qll

+    /**
+     * Gets the source model(s) of `api`, if there is flow from an existing known source to the return of `api`.
+     */
+    string captureSource(DataFlowSourceTargetApi api) {


Do captureSource and captureSink also use heuristics? I was under the impression they just used normal data flow?

The dataflow configuration for both of these implements the isAdditionalFlowStep predicate like

predicate isAdditionalFlowStep(DataFlow::Node node1, DataFlow::Node node2) { isRelevantTaintStep(node1, node2) }

The isRelevantTaintStep includes some read/store logic for fields which causes field conflation. In any case for some languages we can't avoid this, as declaring a field of an Argument or a ReturnValue as a source or sink doesn't work (this is not supported for C#).

As a consequence the generated models are somewhat imprecise (so I think it is ok to use the term Heuristic for these as well).

michaelnebel · 2025-04-23T15:00:17Z

Putting back in draft, as the C++ model generator has been merged - so this needs to be handled in this PR as well.

jketema · 2025-04-24T12:50:36Z

cpp/ql/lib/change-notes/2025-04-24-model-generation.md

+---
+category: minorAnalysis
+---
+* Changes to the MaD model generation infrastructure: Changed the query `cpp/utils/modelgenerator/summary-models` to use the implementation from `cpp/utils/modelgenerator/mixed-summary-models`. Removed the now-redundant `cpp/utils/modelgenerator/mixed-summary-models` query. Similar replacement was made for `cpp/utils/modelgenerator/neutral-models`. That is, if `GenerateFlowModel.py` is provided with `--with-summaries` combined/mixed models are now generated instead of heuristic models (and similar for `--with-neutrals`).


I think we can omit this for C++, as we didn't even add a change note when we added the infrastructure. If we do want to have a change note, it should probably say that we've added the infrastructure.

jketema

C++ 👍 I do not have an opinion on any other of the changes.

michaelnebel · 2025-04-25T11:13:14Z

The failing CPP integration test is unrelated.
The PR is ready for review. There is a subsequent PR that builds on top of this one: #19382

paldepind

Rust changes LGTM 🎉

…dels.

…n its own module.

…adjust other queries to the re-factored implementation.

… in tests.

…c version instead.

…utral generator.

…uery suite.

…ion.

smowton

Java changes look plausible

michaelnebel · 2025-04-28T08:35:52Z

Merging now as it has previously been approved for C++ and Rust as well.

github-actions bot added C# Java Rust Pull requests that update Rust code labels Apr 15, 2025

michaelnebel force-pushed the csharp/generatorcleanup branch 2 times, most recently from 3809598 to 3ef0f89 Compare April 16, 2025 09:12

github-actions bot added the documentation label Apr 16, 2025

michaelnebel marked this pull request as ready for review April 16, 2025 11:49

Copilot AI review requested due to automatic review settings April 16, 2025 11:49

michaelnebel requested review from a team as code owners April 16, 2025 11:49

Copilot AI reviewed Apr 16, 2025

View reviewed changes

geoffw0 reviewed Apr 17, 2025

View reviewed changes

michaelnebel force-pushed the csharp/generatorcleanup branch from 3e63ea9 to 846eb76 Compare April 22, 2025 09:23

michaelnebel mentioned this pull request Apr 22, 2025

C++: Instantiate model generation library #19295

Merged

paldepind reviewed Apr 22, 2025

View reviewed changes

michaelnebel force-pushed the csharp/generatorcleanup branch from 43f3d5e to af5e36a Compare April 23, 2025 14:59

michaelnebel marked this pull request as draft April 23, 2025 15:00

michaelnebel force-pushed the csharp/generatorcleanup branch from af5e36a to 27165ef Compare April 24, 2025 12:11

github-actions bot added the C++ label Apr 24, 2025

michaelnebel marked this pull request as ready for review April 24, 2025 12:45

michaelnebel requested a review from a team as a code owner April 24, 2025 12:45

jketema reviewed Apr 24, 2025

View reviewed changes

michaelnebel force-pushed the csharp/generatorcleanup branch from 27165ef to 97d78f4 Compare April 24, 2025 12:54

jketema previously approved these changes Apr 24, 2025

View reviewed changes

michaelnebel force-pushed the csharp/generatorcleanup branch from 97d78f4 to 97ac804 Compare April 25, 2025 11:34

paldepind previously approved these changes Apr 25, 2025

View reviewed changes

michaelnebel added 20 commits April 25, 2025 15:49

Shared: Remove --with-mixed-summaries logic.

2a8fe53

Shared: Remove the backwards compatbility flag.

fa5162f

Shared: Use the CaptureSummaryModels instead of CaptureMixedSummaryMo…

ae70c76

…dels.

Shared: Remove the --with-mixed-neutrals logic.

2357a69

Shared: Re-factor the model generator and put the heuristic queries i…

f78be91

…n its own module.

C#/Java/Rust: Use Mixed flow from capture summary models queries and …

2a0097e

…adjust other queries to the re-factored implementation.

C#: Re-factor tests to use the new implementations.

7801fc3

C#: Rename some of the model generator tests.

da99c75

C#: Re-factor the heuristic summary test to use heuristic-summary tag…

539a06d

… in tests.

C#: Change the capture neutral model test to use the content/heuristi…

1d6c367

…c version instead.

Java: Adjust model generator test cases to the new implementation.

08f7caa

Java: Change the heuristic summary test tag to heuristic-summary.

7e51dae

Java: Convert the model generator neutral test to use the combined ne…

71d0409

…utral generator.

Rust: Adjust tests.

2155396

C#/Java/Rust: Add change notes.

32125d2

Java: Update integration test that tracks queries not included in a q…

d187a7d

…uery suite.

Shared: Address review comments.

f6135d5

C++: Adjust the model generator queries to the new shared implementat…

d05f604

…ion.

C++: Update model generator tests.

a589014

C#/Rust: Update integration test expected output.

de12222

michaelnebel dismissed stale reviews from paldepind and jketema via de12222 April 25, 2025 13:53

michaelnebel force-pushed the csharp/generatorcleanup branch from 97ac804 to de12222 Compare April 25, 2025 13:53

smowton approved these changes Apr 28, 2025

View reviewed changes

michaelnebel merged commit 8857f0d into github:main Apr 28, 2025
63 checks passed

michaelnebel deleted the csharp/generatorcleanup branch April 28, 2025 08:36

michaelnebel mentioned this pull request Apr 28, 2025

Add query suite inclusion tests for cpp, python #19390

Merged

MathiasVP added a commit to MathiasVP/ql that referenced this pull request Apr 28, 2025

C++: Fix annotations after github#19311.

e942ec9

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Shared: Model generator cleanup. #19311

Shared: Model generator cleanup. #19311

michaelnebel commented Apr 15, 2025 •

edited

Loading

Copilot AI left a comment

geoffw0 left a comment

geoffw0 Apr 17, 2025

paldepind Apr 22, 2025

michaelnebel Apr 23, 2025

paldepind Apr 22, 2025

michaelnebel Apr 23, 2025

michaelnebel Apr 23, 2025

michaelnebel commented Apr 23, 2025

jketema Apr 24, 2025

jketema left a comment

michaelnebel commented Apr 25, 2025

paldepind left a comment

smowton left a comment

michaelnebel commented Apr 28, 2025

		@@ -943,7 +1041,7 @@ module MakeModelGenerator<
		* 2. If content based flow does not yield any summary for an API, then we try and
		* generate flow summaries using the non-content based summary generator.

	* generate flow summaries using the non-content based summary generator.
	* generate flow summaries using the heuristic summary generator.

Shared: Model generator cleanup. #19311

Shared: Model generator cleanup. #19311

Conversation

michaelnebel commented Apr 15, 2025 • edited Loading

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

geoffw0 left a comment

Choose a reason for hiding this comment

geoffw0 Apr 17, 2025

Choose a reason for hiding this comment

paldepind Apr 22, 2025

Choose a reason for hiding this comment

michaelnebel Apr 23, 2025

Choose a reason for hiding this comment

paldepind Apr 22, 2025

Choose a reason for hiding this comment

michaelnebel Apr 23, 2025

Choose a reason for hiding this comment

michaelnebel Apr 23, 2025

Choose a reason for hiding this comment

michaelnebel commented Apr 23, 2025

jketema Apr 24, 2025

Choose a reason for hiding this comment

jketema left a comment

Choose a reason for hiding this comment

michaelnebel commented Apr 25, 2025

paldepind left a comment

Choose a reason for hiding this comment

smowton left a comment

Choose a reason for hiding this comment

michaelnebel commented Apr 28, 2025

michaelnebel commented Apr 15, 2025 •

edited

Loading