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.

Sign up for GitHub

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

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

Closed
wants to merge 7 commits into from
+1,090 −759
Closed

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
c6e43f3
Move Roslyn.sln to latest analyzer packages, also including the newly…
mavasani Feb 22, 2018
ad14927
Suppress CA2000 - dispose ownership transfer
mavasani Mar 2, 2018
e0f2523
Move to 2.6.0 version of Microsoft.NetCore.Compilers
mavasani Mar 2, 2018
2e58602
Move to 2.8.0 version of netcore compilers and 2.6.1 of net compilers
mavasani Mar 2, 2018
9d942b1
Trying version 2.9.0-beta1-62701-01 of netcore compilers
mavasani Mar 2, 2018
184ea69
Trying version 2.6.1-beta3-62526-07
mavasani Mar 2, 2018
f728c6e
Use 2.7.0-beta3-62720-08 of MicrosoftNETCoreCompilersVersion
mavasani Apr 2, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 22 additions & 12 deletions build/Rulesets/NonShippingProject_BuildRules.ruleset
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,35 +2,45 @@
<RuleSet Name="Common diagnostic rules for all non-shipping projects" Description="Enables/disable rules specific to all non-shipping projects." ToolsVersion="14.0">
<Include Path=".\Roslyn_BuildRules.ruleset" Action="Default" />

<Rules AnalyzerId="Microsoft.ApiDesignGuidelines.Analyzers" RuleNamespace="Microsoft.ApiDesignGuidelines.Analyzers">
<Rules AnalyzerId="Microsoft.CodeQuality.Analyzers" RuleNamespace="Microsoft.CodeQuality.Analyzers">
<!-- For tests, the ConfigureAwait(true) is good enough. Either they are already running on a thread pool
thread where ConfigureAwait(false) does nothing, or we're running the workload from an STA thread
where we want to marshal the continuations back to it. -->
<Rule Id="CA2007" Action="None" />
</Rules>

<Rules AnalyzerId="System.Runtime.Analyzers" RuleNamespace="System.Runtime.Analyzers">
<!-- Avoid zero length allocations - suppress for non-shipping/test projects (originally RS0007) -->
<Rule Id="CA1825" Action="None" />
<!-- Do not use Enumerable methods on indexable collections - suppressed because we have lot of violations in non-shipping/test projects.

<!-- Do not use Enumerable methods on indexable collections (originally RS0014) - suppressed because we have lot of violations in non-shipping/test projects.
We can fix all violations once we have a code fix with fix all, and then remove this suppression.
-->
<Rule Id="RS0014" Action="None" />
<Rule Id="CA1826" Action="None" />

<!-- Mark constant field as 'const' instead of static readonly - not useful for tests -->
<Rule Id="CA1802" Action="None" />

<!-- Properties should not return arrays - not useful for tests -->
<Rule Id="CA1819" Action="None" />
</Rules>

<Rules AnalyzerId="Microsoft.CodeAnalysis.Analyzers" RuleNamespace="Microsoft.CodeAnalysis.Analyzers">
<Rule Id="RS1001" Action="None" />
<!-- CodeFix providers should override GetFixAllProvider - suppress for non-shipping/test projects (RS1016) -->
<Rule Id="RS1016" Action="None" />
</Rules>

<Rules AnalyzerId="System.Runtime.Analyzers" RuleNamespace="System.Runtime.Analyzers">
<!-- CodeFix providers should override GetFixAllProvider - suppress for non-shipping/test projects -->
<Rule Id="RS1016" Action="None" />

<!-- DiagnosticId must be unique across analyzers - suppress for non-shipping/test projects -->
<Rule Id="RS1019" Action="None" />

<!-- Do not use generic CodeAction.Create to create CodeAction - not useful for tests -->
<Rule Id="RS0005" Action="None" />
</Rules>

<Rules AnalyzerId="Microsoft.CodeQuality.Analyzers.QualityGuidelines" RuleNamespace="Microsoft.CodeQuality.Analyzers.QualityGuidelines">
<!-- Mark constant field as 'const' instead of static readonly - not useful for tests -->
<Rule Id="CA1802" Action="None" />
<Rules AnalyzerId="Microsoft.CodeQuality.Analyzers.Exp" RuleNamespace="Microsoft.CodeQuality.Analyzers.Exp">
<!-- Dispose rules turned off for non-shipping projects due to large number of violation count -->
<!-- https://github.com/dotnet/roslyn/issues/25129 tracks fixing/suppressing these violations and removing the below entries -->
<Rule Id="CA2000" Action="None" />
<Rule Id="CA2213" Action="None" />
<Rule Id="CA1063" Action="None" />
</Rules>
</RuleSet>
4 changes: 3 additions & 1 deletion build/Rulesets/Roslyn_BuildRules.ruleset
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@
<Rule Id="CA1055" Action="None" />
<Rule Id="CA1056" Action="None" />
<Rule Id="CA1061" Action="None" /> <!-- "do not hide base class methods": currently violations in the compiler -->
<Rule Id="CA1063" Action="None" /> <!-- https://github.com/dotnet/roslyn/issues/25134: Enable rule CA1063 (Implement IDisposable correctly) for Roslyn.sln -->
<Rule Id="CA1064" Action="None" />
<Rule Id="CA1065" Action="None" />
<Rule Id="CA1066" Action="None" />
Expand All @@ -44,6 +45,7 @@
<Rule Id="CA1814" Action="None" /> <!-- prefer jagged arrays to multidimensional: a silly piece of advice -->
<Rule Id="CA1815" Action="None" />
<Rule Id="CA1821" Action="Warning" />
<Rule Id="CA1822" Action="None" /> <!-- https://github.com/dotnet/roslyn/issues/25132: Enable rule CA1822 (Mark members as static) on Roslyn.sln -->
<Rule Id="CA1823" Action="None" /> <!-- https://github.com/dotnet/roslyn/issues/20404: Enable rule CA1823 (remove unused field) once we have a fixer for it -->
<Rule Id="CA1824" Action="None" /> <!-- mark assemblies with NeutralResourcesLanguageAttribute -->
<Rule Id="CA2007" Action="Warning" />
Expand Down Expand Up @@ -121,7 +123,7 @@
<Rule Id="RS0018" Action="Warning" />
</Rules>
<Rules AnalyzerId="XmlDocumentationComments.Analyzers" RuleNamespace="XmlDocumentationComments.Analyzers">
<Rule Id="RS0010" Action="Warning" />
<Rule Id="CA1200" Action="Warning" />
</Rules>
<Rules AnalyzerId="Microsoft.NetCore.Analyzers" RuleNamespace="Microsoft.NetCore.Analyzers">
<Rule Id="CA9999" Action="None" /> <!-- We know the analyzers will fail during a bootstrap build -->
Expand Down
6 changes: 3 additions & 3 deletions build/Targets/Packages.props
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,10 +55,10 @@
<MicrosoftMSXMLVersion>8.0.0.0-alpha</MicrosoftMSXMLVersion>
<!-- Using a private build of Microsoft.Net.Test.SDK to work around issue https://github.com/Microsoft/vstest/issues/373 -->
<MicrosoftNETTestSdkVersion>15.6.0-dev</MicrosoftNETTestSdkVersion>
<MicrosoftNetCompilersVersion>2.6.0</MicrosoftNetCompilersVersion>
<MicrosoftNetRoslynDiagnosticsVersion>2.6.0-beta2</MicrosoftNetRoslynDiagnosticsVersion>
<MicrosoftNetCompilersVersion>2.6.1</MicrosoftNetCompilersVersion>
<MicrosoftNetRoslynDiagnosticsVersion>2.6.1-beta1-62702-01</MicrosoftNetRoslynDiagnosticsVersion>
<MicrosoftNetCoreILAsmVersion>2.0.0</MicrosoftNetCoreILAsmVersion>
<MicrosoftNETCoreCompilersVersion>2.6.0-beta3-62308-01</MicrosoftNETCoreCompilersVersion>
<MicrosoftNETCoreCompilersVersion>2.7.0-beta3-62720-08</MicrosoftNETCoreCompilersVersion>
<MicrosoftNETCoreVersion>5.0.0</MicrosoftNETCoreVersion>
<MicrosoftNETCoreAppVersion>2.0.0</MicrosoftNETCoreAppVersion>
<MicrosoftNETCorePlatformsVersion>2.0.0</MicrosoftNETCorePlatformsVersion>
Expand Down
2 changes: 2 additions & 0 deletions src/Compilers/CSharp/Portable/Emitter/EditAndContinue/PEDeltaAssemblyBuilder.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -104,7 +104,9 @@ private static EmitBaseline.MetadataSymbols GetOrCreateMetadataSymbols(EmitBasel
var metadataCompilation = compilation.RemoveAllSyntaxTrees();

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

📝 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

var metadataAssembly = metadataCompilation.GetBoundReferenceManager().CreatePEAssemblyForAssemblyMetadata(AssemblyMetadata.Create(originalMetadata), MetadataImportOptions.All, out assemblyReferenceIdentityMap);
#pragma warning restore CA2000 // Dispose objects before losing scope
var metadataDecoder = new MetadataDecoder(metadataAssembly.PrimaryModule);
var metadataAnonymousTypes = GetAnonymousTypeMapFromMetadata(originalMetadata.MetadataReader, metadataDecoder);
var metadataSymbols = new EmitBaseline.MetadataSymbols(metadataAnonymousTypes, metadataDecoder, assemblyReferenceIdentityMap);
Expand Down
84 changes: 43 additions & 41 deletions src/Compilers/CSharp/Portable/Parser/DirectiveParser.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -276,59 +276,61 @@ private DirectiveTriviaSyntax ParseErrorOrWarningDirective(SyntaxToken hash, Syn
bool isError = keyword.Kind == SyntaxKind.ErrorKeyword;
if (isActive)
{
var triviaBuilder = new System.IO.StringWriter(System.Globalization.CultureInfo.InvariantCulture);
int triviaWidth = 0;

// whitespace and single line comments are trailing trivia on the keyword, the rest
// of the error message is leading trivia on the eod.
//
bool skipping = true;
foreach (var t in keyword.TrailingTrivia)
using (var triviaBuilder = new System.IO.StringWriter(System.Globalization.CultureInfo.InvariantCulture))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

{
if (skipping)
int triviaWidth = 0;

// whitespace and single line comments are trailing trivia on the keyword, the rest
// of the error message is leading trivia on the eod.
//
bool skipping = true;
foreach (var t in keyword.TrailingTrivia)
{
if (t.Kind == SyntaxKind.WhitespaceTrivia)
if (skipping)
{
continue;
if (t.Kind == SyntaxKind.WhitespaceTrivia)
{
continue;
}

skipping = false;
}

skipping = false;
t.WriteTo(triviaBuilder, leading: true, trailing: true);
triviaWidth += t.FullWidth;
}

t.WriteTo(triviaBuilder, leading: true, trailing: true);
triviaWidth += t.FullWidth;
}

foreach (var node in eod.LeadingTrivia)
{
node.WriteTo(triviaBuilder, leading: true, trailing: true);
triviaWidth += node.FullWidth;
}
foreach (var node in eod.LeadingTrivia)
{
node.WriteTo(triviaBuilder, leading: true, trailing: true);
triviaWidth += node.FullWidth;
}

//relative to leading trivia of eod
//could be negative if part of the error text comes from the trailing trivia of the keyword token
int triviaOffset = eod.GetLeadingTriviaWidth() - triviaWidth;
//relative to leading trivia of eod
//could be negative if part of the error text comes from the trailing trivia of the keyword token
int triviaOffset = eod.GetLeadingTriviaWidth() - triviaWidth;

string errorText = triviaBuilder.ToString();
eod = this.AddError(eod, triviaOffset, triviaWidth, isError ? ErrorCode.ERR_ErrorDirective : ErrorCode.WRN_WarningDirective, errorText);
string errorText = triviaBuilder.ToString();
eod = this.AddError(eod, triviaOffset, triviaWidth, isError ? ErrorCode.ERR_ErrorDirective : ErrorCode.WRN_WarningDirective, errorText);

if (isError)
{
if (errorText.Equals("version", StringComparison.Ordinal))
if (isError)
{
Assembly assembly = typeof(CSharpCompiler).GetTypeInfo().Assembly;
string version = CommonCompiler.GetAssemblyFileVersion(assembly);
eod = this.AddError(eod, triviaOffset, triviaWidth, ErrorCode.ERR_CompilerAndLanguageVersion, version,
this.Options.SpecifiedLanguageVersion.ToDisplayString());
}
else
{
const string versionMarker = "version:";
if (errorText.StartsWith(versionMarker, StringComparison.Ordinal) &&
LanguageVersionFacts.TryParse(errorText.Substring(versionMarker.Length), out var languageVersion))
if (errorText.Equals("version", StringComparison.Ordinal))
{
ErrorCode error = this.Options.LanguageVersion.GetErrorCode();
eod = this.AddError(eod, triviaOffset, triviaWidth, error, "version", new CSharpRequiredLanguageVersion(languageVersion));
Assembly assembly = typeof(CSharpCompiler).GetTypeInfo().Assembly;
string version = CommonCompiler.GetAssemblyFileVersion(assembly);
eod = this.AddError(eod, triviaOffset, triviaWidth, ErrorCode.ERR_CompilerAndLanguageVersion, version,
this.Options.SpecifiedLanguageVersion.ToDisplayString());
}
else
{
const string versionMarker = "version:";
if (errorText.StartsWith(versionMarker, StringComparison.Ordinal) &&
LanguageVersionFacts.TryParse(errorText.Substring(versionMarker.Length), out var languageVersion))
{
ErrorCode error = this.Options.LanguageVersion.GetErrorCode();
eod = this.AddError(eod, triviaOffset, triviaWidth, error, "version", new CSharpRequiredLanguageVersion(languageVersion));
}
}
}
}
Expand Down
8 changes: 5 additions & 3 deletions src/Compilers/CSharp/Portable/Parser/Directives.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,9 +65,11 @@ public bool IncrementallyEquivalent(Directive other)
// Can't be private as it's called by DirectiveStack in its GetDebuggerDisplay()
internal string GetDebuggerDisplay()
{
var writer = new System.IO.StringWriter(System.Globalization.CultureInfo.InvariantCulture);
_node.WriteTo(writer, false, false);
return writer.ToString();
using (var writer = new System.IO.StringWriter(System.Globalization.CultureInfo.InvariantCulture))
{
_node.WriteTo(writer, false, false);
return writer.ToString();
}
}

internal string GetIdentifier()
Expand Down
4 changes: 2 additions & 2 deletions src/Compilers/CSharp/Portable/SymbolDisplay/ObjectDisplay.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@

namespace Microsoft.CodeAnalysis.CSharp
{
#pragma warning disable RS0010
#pragma warning disable CA1200
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, will do.

/// <summary>
/// Displays a value in the C# style.
/// </summary>
Expand All @@ -19,7 +19,7 @@ namespace Microsoft.CodeAnalysis.CSharp
/// the Formatter project and we don't want it to be public there.
/// </remarks>
/// <seealso cref="T:Microsoft.CodeAnalysis.VisualBasic.Symbols.ObjectDisplay"/>
#pragma warning restore RS0010
#pragma warning restore CA1200
internal static class ObjectDisplay
{
/// <summary>
Expand Down
4 changes: 2 additions & 2 deletions src/Compilers/CSharp/Portable/SymbolDisplay/SymbolDisplay.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,12 +7,12 @@

namespace Microsoft.CodeAnalysis.CSharp
{
#pragma warning disable RS0010
#pragma warning disable CA1200
/// <summary>
/// Displays a symbol in the C# style.
/// </summary>
/// <seealso cref="T:Microsoft.CodeAnalysis.VisualBasic.Symbols.SymbolDisplay"/>
#pragma warning restore RS0010
#pragma warning restore CA1200
public static class SymbolDisplay
{
/// <summary>
Expand Down
4 changes: 2 additions & 2 deletions src/Compilers/CSharp/Portable/Symbols/TypeSymbolExtensions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -938,13 +938,13 @@ internal static bool IsValidV6SwitchGoverningType(this TypeSymbol type, bool isT
return false;
}

#pragma warning disable RS0010
#pragma warning disable CA1200
/// <summary>
/// Returns true if the type is one of the restricted types, namely: <see cref="T:System.TypedReference"/>,
/// <see cref="T:System.ArgIterator"/>, or <see cref="T:System.RuntimeArgumentHandle"/>.
/// or a ref-like type.
/// </summary>
#pragma warning restore RS0010
#pragma warning restore CA1200
internal static bool IsRestrictedType(this TypeSymbol type,
bool ignoreSpanLikeTypes = false)
{
Expand Down
8 changes: 8 additions & 0 deletions src/Compilers/Core/MSBuildTask/GlobalSuppressions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@

// This file is used by Code Analysis to maintain SuppressMessage
// attributes that are applied to this project.
// 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")]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

❓ 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


6 changes: 5 additions & 1 deletion src/Compilers/Core/MSBuildTask/MvidReader.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
using System;
using System.Diagnostics;
using System.IO;
using System.Text;

namespace Microsoft.CodeAnalysis.BuildTasks
{
Expand All @@ -12,7 +13,10 @@ public static class MvidReader

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))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💭 I'm not confident this is an improvement.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

but it is certainly not more readable

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

{
return ReadAssemblyMvidOrEmpty(reader);
}
}

private static Guid ReadAssemblyMvidOrEmpty(BinaryReader reader)
Expand Down
Loading