Skip to content

C#/Java: Re-factor the isSupported predicate. #12261

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 3 commits into from
Feb 23, 2023

Conversation

michaelnebel
Copy link
Contributor

@michaelnebel michaelnebel commented Feb 20, 2023

It turns out that all uses of isSupported also includes logic to cater for neutrals. In this PR we put this into the isSupported predicate itself.
Thank you @starcke!

@michaelnebel michaelnebel force-pushed the csharpjava/refactorissupported branch from b709709 to 86888b8 Compare February 20, 2023 12:53
@michaelnebel
Copy link
Contributor Author

DCA looks good, except there seems to be an issue with a C# project that doesn't build anymore (this is also an issue in another DCA run).

@michaelnebel michaelnebel marked this pull request as ready for review February 21, 2023 09:45
@michaelnebel michaelnebel requested review from a team as code owners February 21, 2023 09:45
@michaelnebel michaelnebel added the no-change-note-required This PR does not need a change note label Feb 21, 2023
aschackmull
aschackmull previously approved these changes Feb 21, 2023
mbg
mbg previously approved these changes Feb 21, 2023
Copy link
Member

@mbg mbg left a comment

Choose a reason for hiding this comment

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

LGTM aside from one minor comment.

/** Holds if this API is a known neutral. */
pragma[nomagic]
predicate isNeutral() { this instanceof FlowSummaryImpl::Public::NeutralCallable }

/** Holds if this API is supported by existing CodeQL libraries, that is, it is either a recognized source or sink or has a flow summary. */
Copy link
Member

Choose a reason for hiding this comment

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

Should this comment be updated to reflect the additional condition?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, definitely! Thank you!

@michaelnebel michaelnebel dismissed stale reviews from mbg and aschackmull via 47c69d9 February 22, 2023 08:05
@michaelnebel michaelnebel merged commit cf659f9 into github:main Feb 23, 2023
@michaelnebel michaelnebel deleted the csharpjava/refactorissupported branch February 23, 2023 09:06
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.

3 participants