Port Dispose analyzers (DisposeObjectsBeforeLosingScope and DisposableFieldsShouldBeDisposed) by mavasani · Pull Request #35248 · dotnet/roslyn

mavasani · 2019-04-24T21:30:25Z

These analyzers are being ported from CA2000 and CA2213 FxCop analyzers, which are amongst the most popular FxCop analyzers and were considered valuable enough to add in the box in VS. Ported analyzers share the core dispose dataflow analysis computation with the analyzers in the roslyn-analyzers repo (Microsoft.CodeAnalysis.FlowAnalysis.Utilities NuGet package).

We will now flag disposable creations in methods that do not escape the method body (return value, out/ref params, assign to field/property, add to collection, etc.). Additionally, we also flag disposable fields of disposable types that are not disposed in the type's Dispose method.

This PR does not add any code fixes. We already have a code refactoring that offers wrapping a disposable creation within a using statement. I am hoping to extend that to handle more cases and also
possibly convert it to a code fix in follow-up PR(s).

(DisposableFieldsShouldBeDisposed) We will now flag disposable creations in methods that do not escape the method body (return value, out/ref params, assign to field/property, add to collection, etc.). Additionally, we also flag disposable fields of disposable types that are not disposed in the type's Dispose method. Ported analyzers share the core dataflow analysis computation with the analyzers in the roslyn-analyzers repo (Microsoft.CodeAnalysis.FlowAnalysis.Utilities). This PR does not add any code fixes. We already have a code refactoring that offers wrapping a disposable creation within a using statement. I am hoping to extend that to handle more cases and also possibly convert it to a code fix in follow-up PR(s).

mavasani · 2019-04-24T22:07:40Z

src/NuGet/VisualStudio/VS.ExternalAPIs.Roslyn.Package.csproj

      <!-- Include a few dependencies of Roslyn. These right now are consumed out of the ExternalAPIs NuGet package to do assembly consistency checking in the main VS
           repo. We should remove these and insert the packages directly, but for now we'll include these to limit the work needed to consume this package. -->
      <_File Include="$(ArtifactsBinDir)Roslyn.VisualStudio.Setup\$(Configuration)\net472\Humanizer.dll" TargetDir="" />
+      <_File Include="$(ArtifactsBinDir)Roslyn.VisualStudio.Setup\$(Configuration)\net472\Microsoft.CodeAnalysis.FlowAnalysis.Utilities.dll" TargetDir="" />


@dotnet/roslyn-infrastructure for help on insertion changes for the new dependency. Will the change in this file and the next file suffice to ensure this assembly is copied next to Features/Workspaces assembly in VS install? I verified the setup project changes in subsequent files work fine and the assembly is loaded correctly in the RoslynDev hive.

I think this file is for the roslyn external API package, so it probably not necessary to include this assembly here.

In reply to: 278337122 [](ancestors = 278337122)

Sure, I will revert this change.

@genlu - The comment about this say that the dependencies added here are needed to perform assembly dependency consistency checks in VSO repo. Are we sure not adding an entry here will not fail those dependency checks? Once you confirm, I can revert the change to this file.

hmm, I'm not sure how the check works. But it sounds like we should add them back. @jasonmalinowski?

Ideally if there's a new NuGet package you should be directly inserting that NuGet package. This is a bit of a hack which is a "well, we aren't, so we'll just stick the binary somewhere else".

The "consistency check" in this case is a tool that's looking at types to catch a minimum set of IVT breaks. So if you don't have a dependency of Roslyn inserted somewhere for it to look at...it won't be able to validate it. It's a suppressable warning by the tool, but better to include it if you can.

sharwell

Marking request changes so I get a chance to do a complete review

src/Features/Core/Portable/Microsoft.CodeAnalysis.Features.csproj

mavasani · 2019-05-03T18:32:15Z

Ping for reviews. I would like to get this in sooner in 16.2 cycle so there is enough bake time to catch any issues/false positives.

Dismissing after discussion

mavasani · 2019-05-06T18:03:25Z

Dismissing after discussion

Thanks.

@sharwell @heejaechang @genlu would you be able to review the analyzers and the added tests? Note that I plan to execute AnalyzerRunner with these analyzers on Roslyn.sln for dogfooding + measuring perf before merge. I will post the numbers when I have this data.

src/Setup/DevDivInsertionFiles/DevDivInsertionFiles.csproj

genlu · 2019-05-07T21:59:45Z

src/Setup/DevDivInsertionFiles/DevDivInsertionFiles.csproj

    <ExpectedDependency Include="Humanizer.Core"/>
    <ExpectedDependency Include="ICSharpCode.Decompiler"/>
    <ExpectedDependency Include="Microsoft.DiaSymReader"/>
+    <ExpectedDependency Include="Microsoft.CodeAnalysis.FlowAnalysis.Utilities"/>


Also, we need to figure out if and how do we want to ngen this new assembly. @tmat Can we use the same model currently used for "System.Reflection.Metadata" if we decide to insert it as part of roslyn?

heejaechang · 2019-05-10T07:56:59Z

@mavasani so, I believe a lot of this code is porting from roslyn-analyzer repro? can you mark which is completely new code and which is ported code?

src/EditorFeatures/CSharpTest/DisposeAnalysis/DisposableFieldsShouldBeDisposedTests.cs

mavasani · 2019-05-10T13:04:05Z

@mavasani so, I believe a lot of this code is porting from roslyn-analyzer repro? can you mark which is completely new code and which is ported code?

All the unit tests have been ported and the format has been changed to match the roslyn's test framework.
The analyzer code was cloned for both the analyzers beings added, but following changes were made:
1. We do not support all the varied DisposeAnalysisKinds that are supported in the analyzers repo through editorconfig configuration. Instead we just support the default non-exception paths analysis mode. We can add support for other modes in future if required.
2. CA2213 (DisposableFieldsShouldBeDisposed) is implemented using SymbolStart/End actions in this PR, but is a compilation end analyzer in the analyzers repo (master branch in that repo is still referencing 2.9.0 bits of Microsoft.CodeAnalysis, which does not have this new API. I am going to switch to SymbolStart/End actions in the analyzers repo in 3.x branch, which does have this API).
The core Dispose dataflow analysis is shared between the two implementations using the referenced FlowAnalysis Utilities assembly.

...Features/Core/Portable/DisposeAnalysis/DisposableFieldsShouldBeDisposedDiagnosticAnalyzer.cs

heejaechang · 2019-05-10T19:48:47Z

...Features/Core/Portable/DisposeAnalysis/DisposableFieldsShouldBeDisposedDiagnosticAnalyzer.cs

+                        symbolEndContext.ReportDiagnostic(diagnostic);
+                    }
+                }
+            }


do you remember whether this symbol analyzer now get deleted after this point? or all symbol analyzers alive until whole compilationwithAnalyzer get deleted?

Nested actions registered within SymbolStart callback can never be called after SymbolEnd action has been called. So yes, the driver will not maintain any state pertaining to Symbol associated with SymbolStart/End once its SymbolEnd action has been called.

...Features/Core/Portable/DisposeAnalysis/DisposableFieldsShouldBeDisposedDiagnosticAnalyzer.cs

heejaechang · 2019-05-10T20:08:02Z

...Features/Core/Portable/DisposeAnalysis/DisposableFieldsShouldBeDisposedDiagnosticAnalyzer.cs

+                void AnalyzeDisposeMethod()
+                {
+                    // Perform dataflow analysis to compute dispose value of disposable fields at the end of dispose method.
+                    if (_disposeAnalysisHelper.TryGetOrComputeResult(operationBlockStartContext.OperationBlocks, containingMethod,


so we also sharing whole dispose analysis itself on top of flow analysis engine?

Yes, that is the core piece being shared.

src/Features/Core/Portable/DisposeAnalysis/DisposeObjectsBeforeLosingScopeDiagnosticAnalyzer.cs

heejaechang · 2019-05-10T20:19:16Z

src/Workspaces/Core/Portable/Shared/Extensions/IMethodSymbolExtensions.cs

+        /// </summary>
+        private static bool HasDisposeMethodSignature(this IMethodSymbol method)
+        {
+            return method.Name == "Dispose" && method.MethodKind == MethodKind.Ordinary &&


const for "Dispose" or nameof(IDisposable.Dispose) ?

… assembly load lazy and revert reference add to the FlowAnalysis assembly in Workspaces (only referenced in Features now).

…rs#2501)

mavasani added this to the 16.2 milestone Apr 24, 2019

mavasani requested review from a team, genlu, heejaechang and sharwell April 24, 2019 21:30

mavasani requested a review from a team as a code owner April 24, 2019 21:30

mavasani commented Apr 24, 2019

View reviewed changes

sharwell previously requested changes Apr 24, 2019

View reviewed changes

src/Features/Core/Portable/Microsoft.CodeAnalysis.Features.csproj Show resolved Hide resolved

jinujoseph added Area-IDE Need Design Review The end user experience design needs to be reviewed and approved. labels Apr 24, 2019

mavasani removed the Need Design Review The end user experience design needs to be reviewed and approved. label May 6, 2019

genlu reviewed May 7, 2019

View reviewed changes

src/Setup/DevDivInsertionFiles/DevDivInsertionFiles.csproj Outdated Show resolved Hide resolved

genlu reviewed May 7, 2019

View reviewed changes