Move Roslyn.sln to latest analyzer packages, also including the newly… by mavasani · Pull Request #25179 · dotnet/roslyn

mavasani · 2018-03-02T00:54:55Z

… written preliminary Dataflow-analysis based Dispose rules.

Ask Mode template not completed

Customer scenario

What does the customer do to get into this situation, and why do we think this
is common enough to address for this release. (Granted, sometimes this will be
obvious "Open project, VS crashes" but in general, I need to understand how
common a scenario is)

Bugs this fixes

(either VSO or GitHub links)

Workarounds, if any

Also, why we think they are insufficient for RC vs. RC2, RC3, or RTW

Risk

This is generally a measure our how central the affected code is to adjacent
scenarios and thus how likely your fix is to destabilize a broader area of code

Performance impact

(with a brief justification for that assessment (e.g. "Low perf impact because no extra allocations/no complexity changes" vs. "Low")

Is this a regression from a previous update?

Root cause analysis

How did we miss it? What tests are we adding to guard against it in the future?

How was the bug found?

(E.g. customer reported it vs. ad hoc testing)

Test documentation updated?

If this is a new non-compiler feature or a significant improvement to an existing feature, update https://github.com/dotnet/roslyn/wiki/Manual-Testing once you know which release it is targeting.

… writtern prelimiary Dataflow analysis based Dispose rules.

mavasani · 2018-03-02T03:01:56Z

@agocke @jaredpar @jinujoseph - what is the latest version of Microsoft.NetCore.Compilers package that can be used? Current version used by Roslyn.sln (2.6.0-beta3-62308-01) does not have latest IOperation APIs, there doesn't seem to be any 2.6.0 or 2.6.1 version of this package and 2.8.0 version of the package seems to have an invalid Microsoft.NETCore.Compilers.props (see https://ci.dot.net/job/dotnet_roslyn/job/master/job/ubuntu_16_debug_prtest/8938/console)

sharwell · 2018-03-02T02:32:56Z

            var metadataCompilation = compilation.RemoveAllSyntaxTrees();

            ImmutableDictionary<AssemblyIdentity, AssemblyIdentity> assemblyReferenceIdentityMap;
+#pragma warning disable CA2000 // Dispose objects before losing scope - dispose ownership transfer to CreatePEAssemblyForAssemblyMetadata


📝 The construction of a disposable object occurs in AssemblyMetadata.Create.

💭 I'm not sure this can or should be treated as an ownership transfer. The CreatePEAssemblyForAssemblyMetadata method returns PEAssemblySymbol, which is not a disposable type. I would only expect factory methods to be candidates for ownership transfer if the constructed object is disposable.

📝 In addition, I believe this is a legitimate resource leak.

I believe this is a legitimate resource leak.

Ah, I assumed CreatePEAssemblyForAssemblyMetadata returns an IDisposable. This is indeed a resource leak. I will file a bug and add it to the suppression.

I'm not sure this can or should be treated as an ownership transfer. The CreatePEAssemblyForAssemblyMetadata method returns PEAssemblySymbol, which is not a disposable type. I would only expect factory methods to be candidates for ownership transfer if the constructed object is disposable.

FxCop does not treat factory methods OR constructor invocations as leading to an implicit dispose ownership transfer - it special cased very few cases, which we match. This means that we will have lot more false positives when these are actually dispose ownership transfers, but we will not miss out on flagging true violations where such a transfer does not happen (for example, we never assigned passed in disposable argument to a field or pass it further down). I think this is a general principle for dispose rules and it has been confirmed multiple times by customers - false positives is fine, but identifying as many true violations as possible is super valuable. This is the reason that dispose rules are by design most noisy and most valuable at the same time, which is somewhat ironical.

dotnet/roslyn-analyzers#1617 tracks how we can design this better, but I would not like to mask true leaks by assuming anything as a possible dispose ownership transfer.

but I would not like to mask true leaks by assuming anything as a possible dispose ownership transfer.

We can avoid masking by pairing the transfer heuristics - each case where signatures treat the code as a transfer of ownership, both the caller and callee agree this is the case. This will allow us to detect the majority cases without losing true positives.

We can avoid masking by pairing the transfer heuristics - each case where signatures treat the code as a transfer of ownership, both the caller and callee agree this is the case

This will not work. I have added the reasoning here so we capture all the arguments in that design issue.

sharwell · 2018-03-02T02:34:58Z

-                //
-                bool skipping = true;
-                foreach (var t in keyword.TrailingTrivia)
+                using (var triviaBuilder = new System.IO.StringWriter(System.Globalization.CultureInfo.InvariantCulture))


❔ What's our reason for not treating StringWriter as a special case?

Special casing disposable types as "okay to not dispose" due to an implementation detail about the current Dispose implementation on a type is not good design. There is nothing that prevents the future versions of the library implementing such types to change their implementation to hold onto to more disposable objects and/or change the implementation of Dispose method, hence I believe that all disposable objects should be disposed, even if we think it is a no-op.

sharwell · 2018-03-02T02:35:34Z

 namespace Microsoft.CodeAnalysis.CSharp
 {
-#pragma warning disable RS0010
+#pragma warning disable CA1200


❕ Please add the comment with the title of the suppressed rule (same for other cases)

Sure, will do.

sharwell · 2018-03-02T02:37:04Z

+// Project-level suppressions either have no target or are given 
+// a specific target and scoped to a namespace, type, member, etc.
+
+[assembly: System.Diagnostics.CodeAnalysis.SuppressMessage("Performance", "CA1819:Properties should not return arrays", Justification = "MSBuild has lot of properties returning arrays")]


❓ Why is this suppressed here instead of with <NoWarn> in the project file?

📝 I would be interested in seeing the cases where the warning was reported.

Why is this suppressed here instead of with in the project file

I guess just a preference and ease of application - a suppression code fix offered in IDE only covers adding global suppress message attribute, not a project level no-warn.

I would be interested in seeing the cases where the warning was reported.

See https://github.com/dotnet/roslyn/blob/master/src/Compilers/Core/MSBuildTask/ManagedCompiler.cs for an example. There are lots of violations in this project.

One advantage of <NoWarn> is it suppresses even the execution of the rule. I believe the project-level suppression will result in the rule still running followed by getting filtered out later.

I believe the project-level suppression will result in the rule still running followed by getting filtered out later.

AnalyzerDriver only executes an analyzer if DiagnosticAnalyzer.SupportedDiagnostics returns at least one supported rule that is effectively enabled in the compilation.

I don't necessarily have a preference in how we suppress warnings but I do prefer it be done consintently. I see a lot of NoWarn, manually suppression, assembly attributes and rulesets. Feel like we need to pick a way and go with it. Otherwise it's nearly impossible to determine what rules are in effect because you have to check so many different places to see if they are turned off.

sharwell · 2018-03-02T02:38:47Z

        public static Guid ReadAssemblyMvidOrEmpty(Stream stream)
        {
-            return ReadAssemblyMvidOrEmpty(new BinaryReader(stream));
+            using (var reader = new BinaryReader(stream, new UTF8Encoding(encoderShouldEmitUTF8Identifier: false, throwOnInvalidBytes: true), leaveOpen: true))


💭 I'm not confident this is an improvement.

Yes it is - see my previous comment on why I believe all disposable objects should be disposed. Nothing prevents future version of BinaryReader.Dispose to actually clean up some resources. We should honor IDisposable contract.

In that sense, it is an improvement, but it is certainly not more readable, hence my 💭

but it is certainly not more readable

I believe honoring an IDisposable contract that can lead to future leaks is more important then readability.

sharwell · 2018-03-02T12:46:30Z

                // this is unfortunate that if we give stream, compiler will just re-copy whole content to 
                // native memory again. this is a way to get around the issue by we getting native memory ourselves and then
                // give them pointer to the native memory. also we need to handle lifetime ourselves.
+#pragma warning disable CA2000 // Dispose objects before losing scope - dispose ownership transfer of metadata instance to s_lifetime


❗️ Ownership of a disposable object cannot be transferred to a ConditionalWeakTable, so the comment will need to be updated. In addition, this should should be reviewed as a leak because the ability to dispose of this object is lost in the call to GetReference, below.

Sure, will file a tracking issue.

sharwell · 2018-03-02T12:47:16Z


                // We don't deterministically release the resulting metadata since we don't know 
                // when we should. So we leave it up to the GC to collect it and release all the associated resources.
+#pragma warning disable CA2000 // Dispose objects before losing scope - escaped with GetReference call below, which wraps the metadata instance.


❗️ This should be reviewed as a leak, not a transfer (metadata.GetReference does not return a disposable type, so the ability to dispose of metadata is lost).

sharwell · 2018-03-02T12:49:07Z

            }

            ((IWorkspaceOptionService)this.Services.GetService<IOptionService>()).OnWorkspaceDisposed(this);
+            _serializationLock.Dispose();


📝 Another case of the always-frustrating SemaphoreSlim. I don't like how disposing of this type makes me worry that new race-condition bugs have been introduced.

I will revert and file a tracking issue.

sharwell · 2018-03-02T12:50:35Z

            }

            // otherwise, just return new solution
+#pragma warning disable CA2000 // Dispose objects before losing scope - dispose ownership transfer to the caller.


❓ How is this an ownership transfer? Solution is not disposable.

sharwell · 2018-03-02T12:55:02Z

            private static readonly TimeSpan ConnectRetryIntervalMs = TimeSpan.FromMilliseconds(20);

            private readonly string _name;
+#pragma warning disable CA2213 // Disposable fields should be disposed - _stream wraps _pipe and is disposed in Close.


💭 The code would be clearer to machines if _pipe was wrapped in DelegatingStream when passed to _stream, allowing both of these fields to be disposed in ClientDirectStream.Dispose.

I'm not not confident the pattern of overriding Close() and Dispose(bool) is correct, and it certainly isn't clear. It would be preferable to remove the override of Close() and place all of the cleanup logic in Dispose(bool) (which is already called by Close() in the base class).

tmat · 2018-03-02T17:38:56Z

            var peStream = FileUtilities.OpenFileStream(path);

            // prefetch image, close stream to avoid locking it:
+#pragma warning disable CA2000 // Dispose objects before losing scope - MetadataImageReference has dispose ownership


#pragma warning disable [](start = 0, length = 24)

Can we have just one suppression for the entire method instead of two? These #pragmas add a lot of clutter.

📝 I found the separation tremendously helpful during review. I would not have been able to complete the review with another approach.

I agree with @sharwell here - suppression for an entire method can cause future leaks in this method to escape. We are working towards reducing the false positives here (see dotnet/roslyn-analyzers#1617), so we will hopefully revert majority of these suppressions.

tmat · 2018-03-02T17:43:56Z

        public static Guid ReadAssemblyMvidOrEmpty(Stream stream)
        {
-            return ReadAssemblyMvidOrEmpty(new BinaryReader(stream));
+            using (var reader = new BinaryReader(stream, new UTF8Encoding(encoderShouldEmitUTF8Identifier: false, throwOnInvalidBytes: true), leaveOpen: true))


new UTF8Encoding(encoderShouldEmitUTF8Identifier: false, throwOnInvalidBytes: true) [](start = 57, length = 83)

Encoding.UTF8 would be sufficient. The code doesn't decode any strings.

Sure, thanks will change to Encoding.UTF8 - I tried to retain existing semantics as I am not an expert in this part of the code base.

tmat · 2018-03-02T18:04:36Z

I think the problem that throws the analyzer off in many cases is the following pattern:

public abstract Base : IDisposable { abstract void Dispose(); }

public ImplementationWithResources : Base { IDisposable _resource; override void Dispose() => _resource.Dispose(); }

public ImplementationWithoutResources : Base { override void Dispose() {} }

The analyzer flags the following, although there is no real leak here:

var x = new ImplementationWithoutResources();
// x is not disposed

This is the case e.g. with Stream and MemoryStream.

ModuleMetadata has even more complicated pattern, but it's a similar concept - depending on the flags that are passed to the underlying PEReader the PEReader either only operates on pure memory or holds on some resources. Depending on what flags the creator of PEReader passes it needs to, or doesn't need to dispose it.

tmat · 2018-03-02T20:00:20Z


            // ownership of the stream is passed on PEReader:
+#pragma warning disable CA2000 // Dispose objects before losing scope - ModuleMetadata has dispose ownership
            return new ModuleMetadata(new PEReader(peStream, options));


return new ModuleMetadata(new PEReader(peStream, options)); [](start = 12, length = 59)

Why is this tagged? The PEReader disposes the Stream and ModuleMetadata disposes PEReader.

This needs dotnet/roslyn-analyzers#1617 to precisely determine dispose ownership transfer without heuristics that can mask true leaks.

Should the comment link to the issue?

Yes, I plan to add a commit linking all contentious suppressions to tracking issue(s).

jasonmalinowski · 2018-03-03T02:14:05Z

@mavasani To your question about "which package version can be used", only rule with master is you can't consume anything newer than the 2.6.* line. If you need 2.7 or later, it needs to go into master-vs-deps.

agocke · 2018-03-05T03:25:15Z

@mavasani Also, there should be 2.7.0-* packages. I don't know what you even got when you selected 2.8.0. That's 15.7, I think, which hasn't shipped, so there's definitely no release package. Similarly, you'll need to wait for 15.6 to ship to get the first 2.7.0 release package.

jinujoseph · 2018-03-05T08:13:54Z

i believe @mavasani was looking for 2.6.0 the latest Ioperation API that we shipped, so master should have right package. whats missing then ?

agocke · 2018-03-05T08:47:51Z

@mavasani It looks like I made a typo in the package. Fixed here: #25220

agocke · 2018-03-05T19:42:08Z

@mavasani It sounds like, from @jaredpar, we can't update the toolset compiler used by Roslyn anyway, because of source build. This PR may not be possible.

mavasani · 2018-03-05T19:46:39Z

@agocke @jaredpar - We are fine with having this PR in any branch, not necessarily master. We would just like to get some dogfooding for latest analyzers. Can you please recommend which is the next best branch to target and the exact version of Microsoft.NetCore.Compilers NuGet package that I can use which includes all 2.6.0 Microsoft.CodeAnalysis public APIs? Tagging @jinujoseph as well.

mavasani · 2018-03-05T19:48:30Z

I am also slightly confused why this is not possible due to source build. We want to start dogfooding analyzers that are based on already shipped 2.6.0 version Microsoft.CodeAnalysis APIs, not any pre-release APIs.

agocke · 2018-03-05T20:14:27Z

@mavasani Unless we backport my fix and patch the released version of microsoft.netcore.compilers, there's no package that you can use that will work cross-platform.

agocke · 2018-03-05T20:15:32Z

@mavasani I also don't see any NuGet packages for the 15.6 release: https://www.nuget.org/packages/Microsoft.Net.Compilers/

mavasani · 2018-03-05T20:17:43Z

@agocke I am talking about 15.5 release, i.e. Microsoft.NetCore.Compilers package equivalent to https://www.nuget.org/packages/Microsoft.Net.Compilers/2.6.1

agocke · 2018-03-05T20:38:32Z

@mavasani That package does not, and will not, exist. Microsoft.NETCore.Compilers was only release-ready for dev15.6.

mavasani · 2018-03-05T20:46:56Z

That package does not, and will not, exist. Microsoft.NETCore.Compilers was only release-ready for dev15.6.

Interesting. Then why is this unshipped and unsupported 2.6.X version of Microsoft.NETCore.Compilers package being used on master branch? I believe any such dependency should be removed from master. It seems weird why an unshipped package is blocking us from moving us to an analyzer package based on shipped Microsoft.CodeAnalysis APIs.

agocke · 2018-03-05T21:21:28Z

@mavasani It was necessary for us to build on CoreCLR, since previous builds did not work properly. Now we can probably download a corresponding release of .NET CLI for our bootstrap.

jaredpar · 2018-04-02T15:35:11Z

+#pragma warning disable CA2000 // Dispose objects before losing scope
            if (!TryGetItemIDsAndRQName(workspace, changedDocumentIDs, symbol, out var visualStudioWorkspace, out hierarchyToItemIDsMap, out var rqname))
+#pragma warning restore CA2000 // Dispose objects before losing scope
            {


Please don't add an trivia between the if and opening {. I'd move it after {

sharwell · 2018-04-02T16:07:52Z

📝 I've requested this get added to the design meeting agenda

mavasani · 2018-04-02T22:40:54Z

Closing in favor of #25891. #25880 tracks enabling/auditing the dispose rules.

Move Roslyn.sln to latest analyzer packages, also including the newly…

c6e43f3

… writtern prelimiary Dataflow analysis based Dispose rules.

mavasani added the PR For Personal Review Only The PR doesn’t require anyone other than the developer to review it. label Mar 2, 2018

mavasani requested review from a team as code owners March 2, 2018 00:54

sharwell self-requested a review March 2, 2018 01:55

mavasani added 3 commits March 1, 2018 18:20

Suppress CA2000 - dispose ownership transfer

ad14927

Move to 2.6.0 version of Microsoft.NetCore.Compilers

e0f2523

Move to 2.8.0 version of netcore compilers and 2.6.1 of net compilers

2e58602

mavasani added 2 commits March 1, 2018 19:10

Trying version 2.9.0-beta1-62701-01 of netcore compilers

9d942b1

Trying version 2.6.1-beta3-62526-07

184ea69

sharwell suggested changes Mar 2, 2018

View reviewed changes

tmat reviewed Mar 2, 2018

View reviewed changes

agocke mentioned this pull request Mar 5, 2018

Fix typo in Microsoft.NETCore.Compilers props file #25220

Merged

DustinCampbell mentioned this pull request Apr 2, 2018

Major MSBuildWorkspace update #21670

Merged

8 tasks

jaredpar reviewed Apr 2, 2018

View reviewed changes

Use 2.7.0-beta3-62720-08 of MicrosoftNETCoreCompilersVersion

f728c6e

mavasani mentioned this pull request Apr 2, 2018

Enable and audit dispose rule suppressions in Roslyn.sln (CA2000 and CA2213) #25880

Closed

mavasani mentioned this pull request Apr 2, 2018

Move Roslyn.sln to newer analyzers (with Dispose rules disabled) #25891

Merged

mavasani closed this Apr 2, 2018

Conversation

mavasani commented Mar 2, 2018

Customer scenario

Bugs this fixes

Workarounds, if any

Risk

Performance impact

Is this a regression from a previous update?

Root cause analysis

How was the bug found?

Test documentation updated?

Uh oh!

mavasani commented Mar 2, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tmat commented Mar 2, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tmat Mar 2, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

tmat commented Mar 2, 2018 •

edited

Loading

tmat Mar 2, 2018 •

edited

Loading

agocke commented Mar 5, 2018 •

edited

Loading