Skip to content

Conversation

michaelnebel
Copy link
Contributor

@michaelnebel michaelnebel commented Jul 20, 2022

In this PR we introduce the concept of negative flow summaries, which is a summary of a callable stating that there is no flow via this callable.
The implementation introduces a new class NegativeSummaryModelCsv class, which re-uses parts of the syntax for summaries. The CSV row syntax is namespace;type;name;signature;provenance.

  • The implementation is made for C# and Java (and dummy/empty implementation has been made for Swift and Ruby)
  • The Framework code can parse the negative summaries and new predicates have been introduced to extract the negative summaries from NegativeSummaryModelCsv subclasses.
  • Validation of the negative summaries is included in the CsvValidation.
  • The model generator(s) have been improved to also output a negative summary in case no flow through summary exists.
    • Furthermore it turned out that we need to discard Finalizers (destructors) in the model generation.
  • Telemetry queries now respect the negative summaries. That is, if a callable has a negative flow summary it will not be reported as missing by the telemetry queries.
  • Negative summaries are not included and not counted towards flow summaries in general (in the model coverage queries).

Open questions

  • Should we extends the model coverage functionality to include these summaries or alternatively make another category of coverage where this is reported?
  • Should we make some DCA support for this as well?

@hvitved Any insights about the open questions and/or the implementation itself will be valuable before continuing the work.

@github-actions github-actions bot added the C# label Jul 20, 2022
@github-actions
Copy link
Contributor

⚠️ 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``",3,12038,28,5
+    System,"``System.*``, ``System``",3,51183,28,5
-    Others,"``Dapper``, ``JsonToItemsTaskFactory``, ``Microsoft.ApplicationBlocks.Data``, ``Microsoft.CSharp``, ``Microsoft.EntityFrameworkCore``, ``Microsoft.Extensions.Caching.Distributed``, ``Microsoft.Extensions.Caching.Memory``, ``Microsoft.Extensions.Configuration``, ``Microsoft.Extensions.DependencyInjection``, ``Microsoft.Extensions.DependencyModel``, ``Microsoft.Extensions.FileProviders``, ``Microsoft.Extensions.FileSystemGlobbing``, ``Microsoft.Extensions.Hosting``, ``Microsoft.Extensions.Http``, ``Microsoft.Extensions.Logging``, ``Microsoft.Extensions.Options``, ``Microsoft.Extensions.Primitives``, ``Microsoft.Interop``, ``Microsoft.NET.Build.Tasks``, ``Microsoft.NETCore.Platforms.BuildTasks``, ``Microsoft.VisualBasic``, ``Microsoft.Win32``, ``MySql.Data.MySqlClient``, ``Newtonsoft.Json``",,554,137,
+    Others,"``AssemblyStripper``, ``Dapper``, ``Generators``, ``Internal``, ``JsonToItemsTaskFactory``, ``Microsoft.ApplicationBlocks.Data``, ``Microsoft.CSharp``, ``Microsoft.DotNet.Build.Tasks``, ``Microsoft.DotNet.PlatformAbstractions``, ``Microsoft.EntityFrameworkCore``, ``Microsoft.Extensions.Caching.Distributed``, ``Microsoft.Extensions.Caching.Memory``, ``Microsoft.Extensions.Configuration``, ``Microsoft.Extensions.DependencyInjection``, ``Microsoft.Extensions.DependencyModel``, ``Microsoft.Extensions.FileProviders``, ``Microsoft.Extensions.FileSystemGlobbing``, ``Microsoft.Extensions.Hosting``, ``Microsoft.Extensions.Http``, ``Microsoft.Extensions.Internal``, ``Microsoft.Extensions.Logging``, ``Microsoft.Extensions.Options``, ``Microsoft.Extensions.Primitives``, ``Microsoft.Interop``, ``Microsoft.NET.Build.Tasks``, ``Microsoft.NETCore.Platforms.BuildTasks``, ``Microsoft.VisualBasic``, ``Microsoft.WebAssembly.Build.Tasks``, ``Microsoft.Win32``, ``Microsoft.Workload.Build.Tasks``, ``MySql.Data.MySqlClient``, ``Newtonsoft.Json``",,3330,137,
-    Totals,,3,12599,359,5
+    Totals,,3,54520,359,5
  • Changes to framework-coverage-csharp.csv:
- package,sink,source,summary,sink:code,sink:html,sink:remote,sink:sql,sink:xss,source:local,summary:taint,summary:value
+ package,sink,source,summary,sink:code,sink:html,sink:remote,sink:sql,sink:xss,source:local,summary:none,summary:taint,summary:value
+ AssemblyStripper,,,1,,,,,,,1,,
- Dapper,55,,,,,,55,,,,
+ Dapper,55,,,,,,55,,,,,
+ Generators,,,2,,,,,,,2,,
+ Internal,,,32,,,,,,,32,,
- JsonToItemsTaskFactory,,,7,,,,,,,7,
+ JsonToItemsTaskFactory,,,33,,,,,,,26,7,
- Microsoft.ApplicationBlocks.Data,28,,,,,,28,,,,
+ Microsoft.ApplicationBlocks.Data,28,,,,,,28,,,,,
- Microsoft.CSharp,,,24,,,,,,,24,
+ Microsoft.CSharp,,,36,,,,,,,12,24,
+ Microsoft.DotNet.Build.Tasks,,,47,,,,,,,47,,
+ Microsoft.DotNet.PlatformAbstractions,,,6,,,,,,,6,,
- Microsoft.EntityFrameworkCore,6,,,,,,6,,,,
+ Microsoft.EntityFrameworkCore,6,,,,,,6,,,,,
- Microsoft.Extensions.Caching.Distributed,,,15,,,,,,,15,
+ Microsoft.Extensions.Caching.Distributed,,,41,,,,,,,26,15,
- Microsoft.Extensions.Caching.Memory,,,46,,,,,,,45,1
+ Microsoft.Extensions.Caching.Memory,,,92,,,,,,,46,45,1
- Microsoft.Extensions.Configuration,,,83,,,,,,,80,3
+ Microsoft.Extensions.Configuration,,,236,,,,,,,153,80,3
- Microsoft.Extensions.DependencyInjection,,,62,,,,,,,62,
+ Microsoft.Extensions.DependencyInjection,,,283,,,,,,,221,62,
- Microsoft.Extensions.DependencyModel,,,12,,,,,,,12,
+ Microsoft.Extensions.DependencyModel,,,115,,,,,,,103,12,
- Microsoft.Extensions.FileProviders,,,15,,,,,,,15,
+ Microsoft.Extensions.FileProviders,,,85,,,,,,,70,15,
- Microsoft.Extensions.FileSystemGlobbing,,,15,,,,,,,13,2
+ Microsoft.Extensions.FileSystemGlobbing,,,122,,,,,,,107,13,2
- Microsoft.Extensions.Hosting,,,17,,,,,,,16,1
+ Microsoft.Extensions.Hosting,,,123,,,,,,,106,16,1
- Microsoft.Extensions.Http,,,10,,,,,,,10,
+ Microsoft.Extensions.Http,,,25,,,,,,,15,10,
+ Microsoft.Extensions.Internal,,,2,,,,,,,2,,
- Microsoft.Extensions.Logging,,,37,,,,,,,37,
+ Microsoft.Extensions.Logging,,,245,,,,,,,208,37,
- Microsoft.Extensions.Options,,,8,,,,,,,8,
+ Microsoft.Extensions.Options,,,181,,,,,,,173,8,
- Microsoft.Extensions.Primitives,,,63,,,,,,,63,
+ Microsoft.Extensions.Primitives,,,142,,,,,,,79,63,
- Microsoft.Interop,,,27,,,,,,,27,
+ Microsoft.Interop,,,417,,,,,,,390,27,
- Microsoft.NET.Build.Tasks,,,1,,,,,,,1,
+ Microsoft.NET.Build.Tasks,,,82,,,,,,,81,1,
- Microsoft.NETCore.Platforms.BuildTasks,,,4,,,,,,,4,
+ Microsoft.NETCore.Platforms.BuildTasks,,,84,,,,,,,80,4,
- Microsoft.VisualBasic,,,9,,,,,,,5,4
+ Microsoft.VisualBasic,,,646,,,,,,,637,5,4
+ Microsoft.WebAssembly.Build.Tasks,,,26,,,,,,,26,,
- Microsoft.Win32,,,8,,,,,,,8,
+ Microsoft.Win32,,,122,,,,,,,114,8,
+ Microsoft.Workload.Build.Tasks,,,13,,,,,,,13,,
- MySql.Data.MySqlClient,48,,,,,,48,,,,
+ MySql.Data.MySqlClient,48,,,,,,48,,,,,
- Newtonsoft.Json,,,91,,,,,,,73,18
+ Newtonsoft.Json,,,91,,,,,,,,73,18
- ServiceStack,194,,7,27,,75,92,,,7,
+ ServiceStack,194,,7,27,,75,92,,,,7,
- System,28,3,12038,,4,,23,1,3,10096,1942
+ System,28,3,51183,,4,,23,1,3,39387,9854,1942

@github-actions
Copy link
Contributor

⚠️ 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``",3,12038,28,5
+    System,"``System.*``, ``System``",3,11796,28,5
-    Totals,,3,12599,359,5
+    Totals,,3,12357,359,5
  • Changes to framework-coverage-csharp.csv:
- System,28,3,12038,,4,,23,1,3,10096,1942
+ System,28,3,11796,,4,,23,1,3,9854,1942

@github-actions github-actions bot added the Java label Jul 22, 2022
@michaelnebel michaelnebel changed the title C#: Try out negative summary generation. C#: Negative summaries (ie. no flow through) Jul 22, 2022
@github-actions github-actions bot added the Ruby label Jul 22, 2022
@github-actions github-actions bot added the Swift label Jul 22, 2022
@github-actions
Copy link
Contributor

⚠️ 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``",3,12038,28,5
+    System,"``System.*``, ``System``",3,11796,28,5
-    Totals,,3,12599,359,5
+    Totals,,3,12357,359,5
  • Changes to framework-coverage-csharp.csv:
- System,28,3,12038,,4,,23,1,3,10096,1942
+ System,28,3,11796,,4,,23,1,3,9854,1942

@github-actions
Copy link
Contributor

⚠️ 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``",3,12038,28,5
+    System,"``System.*``, ``System``",3,11796,28,5
-    Totals,,3,12599,359,5
+    Totals,,3,12357,359,5
  • Changes to framework-coverage-csharp.csv:
- System,28,3,12038,,4,,23,1,3,10096,1942
+ System,28,3,11796,,4,,23,1,3,9854,1942

@github-actions
Copy link
Contributor

⚠️ 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``",3,12038,28,5
+    System,"``System.*``, ``System``",3,11796,28,5
-    Totals,,3,12599,359,5
+    Totals,,3,12357,359,5
  • Changes to framework-coverage-csharp.csv:
- System,28,3,12038,,4,,23,1,3,10096,1942
+ System,28,3,11796,,4,,23,1,3,9854,1942

@github-actions
Copy link
Contributor

⚠️ 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``",3,12038,28,5
+    System,"``System.*``, ``System``",3,11796,28,5
-    Totals,,3,12599,359,5
+    Totals,,3,12357,359,5
  • Changes to framework-coverage-csharp.csv:
- System,28,3,12038,,4,,23,1,3,10096,1942
+ System,28,3,11796,,4,,23,1,3,9854,1942

@michaelnebel michaelnebel force-pushed the csharp/nosummary branch 5 times, most recently from 516462b to 0d5f1c7 Compare July 28, 2022 09:31
@github-actions
Copy link
Contributor

⚠️ 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``",3,12038,28,5
+    System,"``System.*``, ``System``",3,11796,28,5
-    Totals,,3,12599,359,5
+    Totals,,3,12357,359,5
  • Changes to framework-coverage-csharp.csv:
- System,28,3,12038,,4,,23,1,3,10096,1942
+ System,28,3,11796,,4,,23,1,3,9854,1942

@michaelnebel
Copy link
Contributor Author

DCA tests have executed.
(1) Performance of nightly queries show no performance regressions or change in alerts.
(2) Performance of capture model show no performance regressions, but a significant change in the number of output rows. For .NET Runtime there is an increase in around 40k rows (which is probably acceptable as we have 10k positive rows). For .NET EFCore we don't know how many positive rows we have but there is a total of around 70k rows.
Will try and run the Capture model queries again on all DCA projects such that we get an understanding of the magnitude of rows.

@michaelnebel michaelnebel force-pushed the csharp/nosummary branch 4 times, most recently from 84c78f8 to 9162528 Compare August 3, 2022 12:12
…base (stubs for .NET has been updated in the meantime).
…en updated due to rebase and changes in the model generator).
…sound and contradicting our assumptions on interface 'contracts') - this now yields a negative summary.
@michaelnebel
Copy link
Contributor Author

(1) DCA for nightly looks fine. No problem with query execution. (2) DCA run for the Models as Data queries themselves hit an OOM exception. This should not block merging this PR, but I will investigate. @aschackmull : Since you last review'ed there has been a rebase, but no other changes.

@aschackmull : Had to rebase yet again. No other changes. Please "review" again. I will keep an eye on the CI and merge ASAP if it becomes possible.

@michaelnebel michaelnebel merged commit c514c88 into github:main Aug 24, 2022
@michaelnebel michaelnebel deleted the csharp/nosummary branch August 24, 2022 10:06
owen-mc added a commit to owen-mc/codeql that referenced this pull request Nov 29, 2022
owen-mc added a commit to owen-mc/codeql that referenced this pull request Nov 29, 2022
owen-mc added a commit to owen-mc/codeql that referenced this pull request Nov 29, 2022
owen-mc added a commit to owen-mc/codeql that referenced this pull request Nov 29, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants