Skip to content

C#/Java: Re-factor the model generator to be a parameterized module. #17509

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

Merged
merged 8 commits into from
Sep 26, 2024

Conversation

michaelnebel
Copy link
Contributor

@michaelnebel michaelnebel commented Sep 18, 2024

In this PR we replace the "pyrameterised" module of the model generator with a parameterised module.
The comments used in the signature module are mostly taken from the C#/Java specific implementation.

@michaelnebel michaelnebel force-pushed the modelgen/parammodule branch 2 times, most recently from 117287d to d09948f Compare September 19, 2024 07:24
@michaelnebel michaelnebel changed the title C#/Java: Make model generation a parameterised module. C#/Java: Re-factor the model generator to be a parameterized module. Sep 19, 2024
@michaelnebel michaelnebel added the no-change-note-required This PR does not need a change note label Sep 19, 2024
@michaelnebel michaelnebel marked this pull request as ready for review September 19, 2024 12:36
@michaelnebel michaelnebel requested review from a team as code owners September 19, 2024 12:36
@michaelnebel
Copy link
Contributor Author

DCA looks good!

Copy link
Contributor

@hvitved hvitved left a comment

Choose a reason for hiding this comment

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

Very nice refactor, some minor comments.

/**
* A node.
*/
class NodeExtended extends Lang::Node {
Copy link
Contributor

Choose a reason for hiding this comment

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

I would just call it Node.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That will cause a compilation error since both Lang and ModelGeneratorInput are imported (even though Lang is imported inside the DataFlow module).

@@ -89,5 +89,5 @@ string captureFlow(DataFlowSummaryTargetApi api) {
string captureNoFlow(DataFlowSummaryTargetApi api) {
not exists(DataFlowSummaryTargetApi api0 | exists(captureFlow(api0)) and api0.lift() = api.lift()) and
api.isRelevant() and
result = Printing::asNeutralSummaryModel(api)
result = ModelPrinting::asNeutralSummaryModel(api)
Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like the contents of this query can also be fully shared?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The historical reason for not doing so, is because of the language specific examples in the comments. Maybe we should just consider scrapping those as this is now captured in a more readable way in the tests.

@@ -0,0 +1,909 @@
/**
* INTERNAL: Do not use.
Copy link
Contributor

Choose a reason for hiding this comment

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

If that's the case, then better to put this file inside an internal folder.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good idea :-D

result = ModelPrinting::asLiftedValueModel(api, qualifierString(), "ReturnValue")
}

private int accessPathLimit0() { result = 2 }
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 nice to put all logic related to content-sensitive model generation inside a nested module, say ContentSensitive.

*/

private import codeql.dataflow.DataFlow
private import codeql.dataflow.TaintTracking as Tt

Check warning

Code scanning / CodeQL

Names only differing by case Warning

Tt is only different by casing from TT that is used elsewhere for modules.
@michaelnebel michaelnebel merged commit dd993c3 into github:main Sep 26, 2024
54 checks passed
@michaelnebel michaelnebel deleted the modelgen/parammodule branch September 26, 2024 08:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C# 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.

2 participants