Skip to content

Java: do not generate models for toString and lambda flow methods - #15772

Merged
owen-mc merged 5 commits into
github:mainfrom
owen-mc:java/model-generator-exclude-tostring
Mar 4, 2024
Merged

Java: do not generate models for toString and lambda flow methods#15772
owen-mc merged 5 commits into
github:mainfrom
owen-mc:java/model-generator-exclude-tostring

Conversation

@owen-mc

@owen-mc owen-mc commented Feb 29, 2024

Copy link
Copy Markdown
Contributor

We generally don't want to model toString. It is better not to generate models for them, and add manual models where they are needed.

We also don't want to have any models for certain functions relating to lambda flow.

@owen-mc owen-mc added the no-change-note-required This PR does not need a change note label Feb 29, 2024
@owen-mc
owen-mc requested a review from michaelnebel February 29, 2024 21:42
@owen-mc
owen-mc requested a review from a team as a code owner February 29, 2024 21:42
@github-actions github-actions Bot added the Java label Feb 29, 2024

@atorralba atorralba 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.

Are these the correct changes? This seems identical to #15767.

@michaelnebel

Copy link
Copy Markdown
Contributor

@owen-mc : Looks like you pushed the wrong branch :-D

@owen-mc

owen-mc commented Mar 1, 2024

Copy link
Copy Markdown
Contributor Author

Oops. This is what happens when I work late at night.

@owen-mc
owen-mc force-pushed the java/model-generator-exclude-tostring branch from 51f94a6 to bbf3fa7 Compare March 1, 2024 09:59
atorralba
atorralba previously approved these changes Mar 1, 2024
michaelnebel
michaelnebel previously approved these changes Mar 1, 2024

@michaelnebel michaelnebel 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.

👍

@owen-mc
owen-mc dismissed stale reviews from michaelnebel and atorralba via bb97df1 March 1, 2024 12:12
@owen-mc owen-mc changed the title Java: do not generate models for toString Java: do not generate models for toString and lambda flow methods Mar 1, 2024
@owen-mc

owen-mc commented Mar 1, 2024

Copy link
Copy Markdown
Contributor Author

I've added an exclusion for the functions in the comment at the bottom of java/ql/src/Metrics/Summaries/TopJdkApis.qll which relate to lambda flow. I will remove them from #15766.

michaelnebel
michaelnebel previously approved these changes Mar 1, 2024

@michaelnebel michaelnebel 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.

Looks good to me!

isInternal(c.getCompilationUnit()) or
c instanceof MainMethod or
c instanceof ToStringMethod or
c instanceof LambdaFlowMethod or

@atorralba atorralba Mar 1, 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.

Would it make sense to use FunctionalInterface instead? i.e.

Suggested change
c instanceof LambdaFlowMethod or
c = any(FunctionalInterface f).getRunMethod() or

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.

Hm, nvm, some of those aren't functional interfaces.

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.

Hmm, I might be able to use that to replace some of the hard-coded method names. I'll have a play around with it.

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.

It gave almost 7000 results! I think it's best to just hard-code a few for now and if we find we want to do more in future then we can come up with a clever way to characterise them.

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.

Makes sense 👍

@michaelnebel michaelnebel 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.

LGTM! 👍

@owen-mc
owen-mc merged commit 038afc4 into github:main Mar 4, 2024
@owen-mc
owen-mc deleted the java/model-generator-exclude-tostring branch March 4, 2024 07:57
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.

3 participants