Skip to content

Java/C#: exclude parameterless constructors from DataFlowTargetApi #11624

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
Merged
merged 6 commits into from
Dec 13, 2022
Merged

Java/C#: exclude parameterless constructors from DataFlowTargetApi #11624

merged 6 commits into from
Dec 13, 2022

Conversation

jcogs33
Copy link
Contributor

@jcogs33 jcogs33 commented Dec 8, 2022 •
edited
Loading

Description

This PR updates DataFlowTargetApi to exclude parameterless constructors.

Consideration

Same as for #11623:

  • Please let me know if the exclusion for C# is correct as I don't have much experience with C#.
  • Are there existing library tests for DataFlowTargetApi where I could add a test case for this scenario? I couldn't find any existing tests in the library-tests directory, but I may have missed them.
  • Does a change like this need a DCA run?
  • Is this change minor enough that I can add the no-change-note-required label?

(cc @michaelnebel)

@jcogs33 jcogs33 changed the title Java/C#: exclude parameterless constructors from DataFlowTargetApi Java/C#: exclude parameterless constructors from DataFlowTargetApi Dec 8, 2022
@jcogs33 jcogs33 marked this pull request as ready for review December 8, 2022 16:17
@jcogs33 jcogs33 requested review from a team as code owners December 8, 2022 16:17
@jcogs33 jcogs33 mentioned this pull request Dec 8, 2022
Merged
@michaelnebel
Copy link
Contributor

michaelnebel commented Dec 9, 2022 •
edited by jcogs33
Loading

  • For both Java and C# consider introducing a member predicate in the Constructor class named isParamterless to avoid the duplicate implementation.
  • The implementation for C# looks correct. Are you up for adding a unit test for C# as well that would cover not creating neutral models for parameterless constructors?
  • Update java unit test output.

michaelnebel
michaelnebel requested changes Dec 9, 2022
View reviewed changes
Copy link
Contributor

@michaelnebel michaelnebel left a comment

Choose a reason for hiding this comment

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

The changes looks good, but I have added some comments for refactoring and testing.

@jcogs33 jcogs33 added the no-change-note-required This PR does not need a change note label Dec 9, 2022
@jcogs33
Copy link
Contributor Author

jcogs33 commented Dec 9, 2022

Thanks again for the review! 🙂 I've addressed your comments, let me know if anything else needs to be adjusted.
I've also started DCA runs for this one as well.

michaelnebel
michaelnebel previously approved these changes Dec 12, 2022
View reviewed changes
Copy link
Contributor

@michaelnebel michaelnebel left a comment

Choose a reason for hiding this comment

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

Yes, this is excellent!

@michaelnebel
Copy link
Contributor

@jcogs33 : I have just merged some renaming including the renaming of some of the testcases, which has lead to some merge conflicts in this PR.
Also, we need to run DCA again with the uninterpreted-queries (I had forgotten about that initially). There is a short guide in the documentation section of: https://github.com/github/codeql-csharp-team/issues/223

@jcogs33 jcogs33 force-pushed the jcogs33/exclude-paramless-constructors-from-dataflowtargetapi branch from 4f943c5 to 22f8d97 Compare December 12, 2022 18:30
michaelnebel
michaelnebel approved these changes Dec 13, 2022
View reviewed changes
Copy link
Contributor

@michaelnebel michaelnebel left a comment

Choose a reason for hiding this comment

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

Excellent work!

@michaelnebel
Copy link
Contributor

DCA looks good. Both Java and C# shows a good reduction in the number of generated neutral models!

@jcogs33 jcogs33 merged commit 9b0163c into github:main Dec 13, 2022
@jcogs33 jcogs33 deleted the jcogs33/exclude-paramless-constructors-from-dataflowtargetapi branch December 13, 2022 15:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

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