C#: Default subtypes to true.

[michaelnebel]

No description provided.

[github-actions]

⚠️ 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:

-    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:

- 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]

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

[owen-mc]

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]

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]

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

[michaelnebel]

@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]

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

[michaelnebel]

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

No, I did not :-)

[michaelnebel]

@owen-mc : Do we want to merge this PR or do you prefer to do this for all languages in the same PR?

[owen-mc]

@michaelnebel I'm happy for you to merge it. I'm not sure when I'll get around to working on this.

[michaelnebel]

@michaelnebel I'm happy for you to merge it. I'm not sure when I'll get around to working on this.

Ok, I think it makes sense to merge, if we do the same thing in conjunction with Java.
Is there a similar Java PR?
If we do so, we also need to update the MaD documentation.

[owen-mc]

Oops, I forgot that this PR wasn't just changing the "false" to "true" in all the models. Yes, in that case we should do something more coordinated between at least the static languages. And see if the dynamic languages have an equivalent.

[michaelnebel]

Ah, so you merged a PR that changes the override/subtyping from false -> true for all Java models?

[owen-mc]

No, I hadn't got round to thinking about it yet. And I had forgotten that you had already done all the hard work here, and we just have to copy it for other languages. For Go I manually changed all the models to use "True" where it has any meaning, and "False" when it doesn't have a meaning (I wanted to use "True" for all but others disagreed). I haven't done anything for Java.

[michaelnebel]

Alright! Then let us leave it for now. It doesn't seem "urgent", so we can postpone - but hopefully not indefinitely! :-)