Skip to content
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

C#: Exclude properties with both a getter and setter as candidates for modelling. #16121

Merged
merged 16 commits into from
Apr 16, 2024

Conversation

michaelnebel
Copy link
Contributor

@michaelnebel michaelnebel commented Apr 4, 2024

This is related to #16088.
Now that all non-source code properties are perceived as "fields" (in terms of read/store steps) we should no longer model properties that have both a getter and setter (as this will be handled by the data flow library).

@github-actions github-actions bot added the C# label Apr 4, 2024
Copy link
Contributor

github-actions bot commented Apr 4, 2024

⚠️ 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``",44,11872,67,9
+    System,"``System.*``, ``System``",44,10429,59,9
-    Others,"``Amazon.Lambda.APIGatewayEvents``, ``Amazon.Lambda.Core``, ``Dapper``, ``ILCompiler``, ``ILLink.RoslynAnalyzer``, ``ILLink.Shared``, ``ILLink.Tasks``, ``Internal.IL``, ``Internal.Pgo``, ``Internal.TypeSystem``, ``JsonToItemsTaskFactory``, ``Microsoft.Android.Build``, ``Microsoft.Apple.Build``, ``Microsoft.ApplicationBlocks.Data``, ``Microsoft.CSharp``, ``Microsoft.Diagnostics.Tools.Pgo``, ``Microsoft.EntityFrameworkCore``, ``Microsoft.Extensions.Caching.Distributed``, ``Microsoft.Extensions.Caching.Memory``, ``Microsoft.Extensions.Configuration``, ``Microsoft.Extensions.DependencyInjection``, ``Microsoft.Extensions.DependencyModel``, ``Microsoft.Extensions.Diagnostics.Metrics``, ``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.NET.WebAssembly.Webcil``, ``Microsoft.VisualBasic``, ``Microsoft.WebAssembly.Build.Tasks``, ``Microsoft.Win32``, ``Mono.Linker``, ``MySql.Data.MySqlClient``, ``Newtonsoft.Json``, ``SourceGenerators``, ``Windows.Security.Cryptography.Core``",54,1548,148,
+    Others,"``Amazon.Lambda.APIGatewayEvents``, ``Amazon.Lambda.Core``, ``Dapper``, ``ILCompiler``, ``ILLink.RoslynAnalyzer``, ``ILLink.Shared``, ``ILLink.Tasks``, ``Internal.IL``, ``Internal.Pgo``, ``Internal.TypeSystem``, ``JsonToItemsTaskFactory``, ``Microsoft.Android.Build``, ``Microsoft.Apple.Build``, ``Microsoft.ApplicationBlocks.Data``, ``Microsoft.CSharp``, ``Microsoft.Diagnostics.Tools.Pgo``, ``Microsoft.EntityFrameworkCore``, ``Microsoft.Extensions.Caching.Distributed``, ``Microsoft.Extensions.Caching.Memory``, ``Microsoft.Extensions.Configuration``, ``Microsoft.Extensions.DependencyInjection``, ``Microsoft.Extensions.DependencyModel``, ``Microsoft.Extensions.Diagnostics.Metrics``, ``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.NET.WebAssembly.Webcil``, ``Microsoft.VisualBasic``, ``Microsoft.WebAssembly.Build.Tasks``, ``Microsoft.Win32``, ``Mono.Linker``, ``MySql.Data.MySqlClient``, ``Newtonsoft.Json``, ``SourceGenerators``, ``Windows.Security.Cryptography.Core``",54,1518,148,
-    Totals,,98,13427,409,9
+    Totals,,98,11954,401,9
  • Changes to framework-coverage-csharp.csv:
- ILLink.Tasks,,,5,,,,,,,,,,,,,,,,,,,5,
+ ILLink.Tasks,,,3,,,,,,,,,,,,,,,,,,,3,
- JsonToItemsTaskFactory,,,7,,,,,,,,,,,,,,,,,,,7,
+ JsonToItemsTaskFactory,,,5,,,,,,,,,,,,,,,,,,,5,
- Microsoft.Apple.Build,,,7,,,,,,,,,,,,,,,,,,,7,
+ Microsoft.Apple.Build,,,5,,,,,,,,,,,,,,,,,,,5,
- Microsoft.Extensions.Caching.Distributed,,,15,,,,,,,,,,,,,,,,,,,15,
+ Microsoft.Extensions.Caching.Distributed,,,9,,,,,,,,,,,,,,,,,,,9,
- Microsoft.Extensions.Caching.Memory,,,38,,,,,,,,,,,,,,,,,,,37,1
+ Microsoft.Extensions.Caching.Memory,,,30,,,,,,,,,,,,,,,,,,,29,1
- Microsoft.Extensions.Configuration,,2,89,,,,,,,,,,,,,2,,,,,,86,3
+ Microsoft.Extensions.Configuration,,2,83,,,,,,,,,,,,,2,,,,,,81,2
- Microsoft.Extensions.Http,,,10,,,,,,,,,,,,,,,,,,,10,
+ Microsoft.Extensions.Http,,,8,,,,,,,,,,,,,,,,,,,8,
- Mono.Linker,,,163,,,,,,,,,,,,,,,,,,,163,
+ Mono.Linker,,,161,,,,,,,,,,,,,,,,,,,161,
- System,67,44,11872,,8,8,9,,,4,5,,33,2,,3,15,17,3,4,,9906,1966
+ System,59,44,10429,,8,8,1,,,4,5,,33,2,,3,15,17,3,4,,8460,1969

@michaelnebel michaelnebel force-pushed the csharp/modelgenexcludeset branch from e867477 to 8cba926 Compare April 4, 2024 14:05
@michaelnebel
Copy link
Contributor Author

@hvitved : This might be of interest to you. The last DCA execution shows that we loose a couple of true positive alerts (but which could might as well have been false positives - as some of the paths that lead to the alerts are actually false positives). It all boils down to the generated models for ProcessStartInfo (you have recently excluded some of these by a manual neutral override). Where "any" constructor argument taints the entire object, which turns out to be a bad assumption for this particular class as it exposes many properties.

I still be believe that the changes in this PR is the way forward, but I will do some manual modelling of this class as it is pretty widely used.

@michaelnebel michaelnebel force-pushed the csharp/modelgenexcludeset branch from 09c29b9 to 55c0c2b Compare April 10, 2024 15:04
@michaelnebel michaelnebel marked this pull request as ready for review April 11, 2024 07:34
@michaelnebel michaelnebel requested a review from a team as a code owner April 11, 2024 07:34
@michaelnebel
Copy link
Contributor Author

DCA looks good.

@michaelnebel michaelnebel force-pushed the csharp/modelgenexcludeset branch from 099d4a5 to f799962 Compare April 12, 2024 09:35
@michaelnebel michaelnebel requested a review from hvitved April 12, 2024 14:01
@michaelnebel michaelnebel merged commit 58635bd into github:main Apr 16, 2024
21 checks passed
@michaelnebel michaelnebel deleted the csharp/modelgenexcludeset branch April 16, 2024 12:49
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.

2 participants