Java/C#: exclude FunctionalExprs from DataFlowTargetApi

[jcogs33]

Description

This PR updates DataFlowTargetApi to exclude functional expressions.

Consideration

• 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?

[aschackmull]

Java part LGTM. When I did a quick test on a C# db, then the C# change didn't appear to have any effect, so maybe these are already excluded somehow? I.e. the following predicate appears empty:

predicate test(TargetApiSpecific api) {
  isRelevantForModels(api) and
  api instanceof CS::AnonymousFunctionExpr
}

[aschackmull]

• Does a change like this need a DCA run?

No, that shouldn't be necessary.

• Is this change minor enough that I can add the no-change-note-required label?

Yes, I think so. It doesn't affect any normal queries.

[michaelnebel]

Java part LGTM. When I did a quick test on a C# db, then the C# change didn't appear to have any effect, so maybe these are already excluded somehow? I.e. the following predicate appears empty:

predicate test(TargetApiSpecific api) {
  isRelevantForModels(api) and
  api instanceof CS::AnonymousFunctionExpr
}

I think that:

api.getDeclaringType().getNamespace().getFullName() != ""

implies (by accident)

not api instanceof CS::AnonymousFunctionExpr

https://github.com/github/codeql/blob/main/csharp/ql/src/utils/modelgenerator/internal/CaptureModelsSpecific.qll#L38

In any case, I think the case for anonymous functions should be explicit in C# as well.

[michaelnebel]

Looks good to me, but try and run DCA before mad-queries.qls before merging to see, if we overlooked something.

[michaelnebel]

• Does a change like this need a DCA run?

I think we should try and run DCA to see the impact on the generated models.
There exist a a query suite in DCA called mad-queries.qls for both Java and C# and it will report the difference in the generated models on the PR against those generated on main on the projects that are being analyzed.
Maybe run the query for java on commons-io and for C# on the dotnet project.

[jcogs33]

Maybe run the query for java on commons-io and for C# on the dotnet project.

@michaelnebel I haven't run DCA on specific projects before, so can I confirm that I would need to use the "Manually select sources" option for this? (That's what I did for the two "CustomSources" DCA runs above, just want to make sure that was correct 🙂)

[michaelnebel]

Yes, you are right - you need to select custom sources and for C#, it is enough to select the dotnet/dotnet project.
Also, to get the numbers reported by DCA you also need to add the mad-queries.qls as *uninterpretedq-queries. This is done via adding an option.
There is a short description on https://github.com/github/codeql-csharp-team/issues/223 in the documentation section on how to achieve this.
You will need wait executing the DCA run until https://github.com/github/codeql-dca/pull/1310 is merged as I have just renamed negative summary models to neutral models and merged that into main (and now we need to the corresponding rename in DCA before we can get the summaries reported here).

[jcogs33]

Thanks! I see you've rerun DCA for this.

[michaelnebel]

(1) DCA reports no changes in the generated models for C# (as we expected).
(2) DCA for Java failed for some reason. Will trigger DCA run again.

[michaelnebel]

@jcogs33 : There are no changes in the number of generated models for commons-io (there appears to be some flakyness with the neutral numbering reported by DCA). Is there a project in DCA, where we expected a difference in the produced models?

[michaelnebel]

@jcogs33 : There are no changes in the number of generated models for commons-io (there appears to be some flakyness with the neutral numbering reported by DCA). Is there a project in DCA, where we expected a difference in the produced models?

The flakyness is probably because the branch needs to be rebased on main (the PR renaming the negative to neutral models has been merged in the meantime).
Could you rebase the branch and re-execute DCA?

[jcogs33]

@michaelnebel The new DCA run looks good to me. Let me know if you disagree.

[michaelnebel]

@michaelnebel The new DCA run looks good to me. Let me know if you disagree.

The numbers reported by DCA looks a bit wrong. The summary section shows that all the neutral models are unique in both variants of the analysis.

This is not a problem with the content of this PR, but rather a problem with the DCA implementation.
I will try and fix the DCA implementation and then we can re-execute DCA again.

[michaelnebel]

DCA looks good now :-)
PR is good to be merged.

[jcogs33]

PR is good to be merged.

@michaelnebel Can you re-approve? Rebasing yesterday dismissed your approval.

[jcogs33]

Thanks @tamasvajk! 😄