github / codeql

CodeQL: the libraries and queries that power security researchers around the world, as well as code scanning in GitHub Advanced Security
https://codeql.github.com
MIT License
7.73k stars 1.55k forks source link

C#: Default subtypes to true. #18060

Open michaelnebel opened 4 days ago

github-actions[bot] commented 4 days ago

:warning: The head of this PR and the base branch were compared for differences in the framework coverage reports. The generated reports are available in the artifacts of this workflow run. The differences will be picked up by the nightly job after the PR gets merged.

Click to show differences in coverage ### csharp #### Generated file changes for csharp - Changes to framework-coverage-csharp.rst: ```diff - System,"``System.*``, ``System``",47,10818,54,5 + System,"``System.*``, ``System``",47,10817,54,5 - Totals,,104,12893,396,5 + Totals,,104,12892,396,5 ``` - Changes to framework-coverage-csharp.csv: ```diff - System,54,47,10818,,6,5,5,,,4,1,,33,2,,6,15,17,4,3,,5511,5307 + System,54,47,10817,,6,5,5,,,4,1,,33,2,,6,15,17,4,3,,5511,5306 ```
michaelnebel commented 3 days ago

@owen-mc : Didn't report any changes.

owen-mc commented 3 days ago

Interesting. Do you think it's a feature that isn't used much? Or is it possible that inheritance isn't quite working as intended?

michaelnebel commented 3 days ago

Interesting. Do you think it's a feature that isn't used much? Or is it possible that inheritance isn't quite working as intended?

I think it works as intended, but maybe many of the summaries that already were relevant for subtypes = true had that set. The FlowSummaries test also reports that changing the logic only means that all the summaries applies to 44 extra callables (in all the libraries/frameworks included in the test) - so it is not that surprising that we don't see any changes in the alerts.

michaelnebel commented 3 days ago

QA run: https://github.com/github/codeql-qa-ops/issues/233

michaelnebel commented 23 hours ago

@owen-mc : The QA dashboard only shows a few alert changes, so I don't think it will have a big effect for C# if we set the subtypes = true as default.

owen-mc commented 15 hours ago

Great. Out of interest, did you check those alerts to see if you think they are valid?

michaelnebel commented 1 hour ago

Great. Out of interest, did you check those alerts to see if you think they are valid?

No, I did not :-)