-
Notifications
You must be signed in to change notification settings - Fork 470
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
Add an analyzer for PublishSingleFile #3921
Changes from 10 commits
ffb9f66
5920d19
24e739d
9347350
a9ca099
26e5052
887f95f
d62018c
7cbb603
1c1418f
24ebb30
6b9ea80
be5822b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -39,6 +39,9 @@ CA2354 | Security | Disabled | DataSetDataTableInIFormatterSerializableObjectGra | |
CA2355 | Security | Disabled | DataSetDataTableInSerializableObjectGraphAnalyzer, [Documentation](https://docs.microsoft.com/visualstudio/code-quality/ca2355) | ||
CA2356 | Security | Disabled | DataSetDataTableInWebSerializableObjectGraphAnalyzer, [Documentation](https://docs.microsoft.com/visualstudio/code-quality/ca2356) | ||
|
||
IL3000 | Publish | Warning | DoNotUseAssemblyLocationInSingleFile, [Documentation](https://docs.microsoft.com/visualstudio/code-quality/il3000) | ||
IL3001 | Publish | Warning | DoNotUseAssemblyGetFilesInSingleFile, [Documentation](https://docs.microsoft.com/visualstudio/code-quality/il3001) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Consider adding a new range row to https://github.com/dotnet/roslyn-analyzers/blob/master/src/Utilities/Compiler/DiagnosticCategoryAndIdRanges.txt. This is an additional file which is used by a meta-analyzer on the repo that enforces few things:
|
||
|
||
### Changed Rules | ||
Rule ID | New Category | New Severity | Old Category | Old Severity | Notes | ||
--------|--------------|--------------|--------------|--------------|------- | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,17 +1,17 @@ | ||
<?xml version="1.0" encoding="utf-8"?> | ||
<root> | ||
<!-- | ||
Microsoft ResX Schema | ||
<!-- | ||
Microsoft ResX Schema | ||
|
||
Version 2.0 | ||
The primary goals of this format is to allow a simple XML format | ||
that is mostly human readable. The generation and parsing of the | ||
various data types are done through the TypeConverter classes | ||
|
||
The primary goals of this format is to allow a simple XML format | ||
that is mostly human readable. The generation and parsing of the | ||
various data types are done through the TypeConverter classes | ||
associated with the data types. | ||
|
||
Example: | ||
|
||
... ado.net/XML headers & schema ... | ||
<resheader name="resmimetype">text/microsoft-resx</resheader> | ||
<resheader name="version">2.0</resheader> | ||
|
@@ -26,36 +26,36 @@ | |
<value>[base64 mime encoded string representing a byte array form of the .NET Framework object]</value> | ||
<comment>This is a comment</comment> | ||
</data> | ||
There are any number of "resheader" rows that contain simple | ||
|
||
There are any number of "resheader" rows that contain simple | ||
name/value pairs. | ||
Each data row contains a name, and value. The row also contains a | ||
type or mimetype. Type corresponds to a .NET class that support | ||
text/value conversion through the TypeConverter architecture. | ||
Classes that don't support this are serialized and stored with the | ||
|
||
Each data row contains a name, and value. The row also contains a | ||
type or mimetype. Type corresponds to a .NET class that support | ||
text/value conversion through the TypeConverter architecture. | ||
Classes that don't support this are serialized and stored with the | ||
mimetype set. | ||
The mimetype is used for serialized objects, and tells the | ||
ResXResourceReader how to depersist the object. This is currently not | ||
|
||
The mimetype is used for serialized objects, and tells the | ||
ResXResourceReader how to depersist the object. This is currently not | ||
extensible. For a given mimetype the value must be set accordingly: | ||
Note - application/x-microsoft.net.object.binary.base64 is the format | ||
that the ResXResourceWriter will generate, however the reader can | ||
|
||
Note - application/x-microsoft.net.object.binary.base64 is the format | ||
that the ResXResourceWriter will generate, however the reader can | ||
read any of the formats listed below. | ||
|
||
mimetype: application/x-microsoft.net.object.binary.base64 | ||
value : The object must be serialized with | ||
value : The object must be serialized with | ||
: System.Runtime.Serialization.Formatters.Binary.BinaryFormatter | ||
: and then encoded with base64 encoding. | ||
|
||
mimetype: application/x-microsoft.net.object.soap.base64 | ||
value : The object must be serialized with | ||
value : The object must be serialized with | ||
: System.Runtime.Serialization.Formatters.Soap.SoapFormatter | ||
: and then encoded with base64 encoding. | ||
|
||
mimetype: application/x-microsoft.net.object.bytearray.base64 | ||
value : The object must be serialized into a byte array | ||
value : The object must be serialized into a byte array | ||
: using a System.ComponentModel.TypeConverter | ||
: and then encoded with base64 encoding. | ||
--> | ||
|
@@ -1422,4 +1422,13 @@ | |
<data name="DoNotUseOutAttributeStringPInvokeParametersTitle" xml:space="preserve"> | ||
<value>Do not use 'OutAttribute' on string parameters for P/Invokes</value> | ||
</data> | ||
</root> | ||
<data name="AvoidAssemblyLocationInSingleFileTitle" xml:space="preserve"> | ||
<value>Avoid using accessing Assembly file path when publishing as a single-file</value> | ||
</data> | ||
<data name="AvoidAssemblyLocationInSingleFileMessage" xml:space="preserve"> | ||
agocke marked this conversation as resolved.
Show resolved
Hide resolved
|
||
<value>'{0}' always returns an empty string for assemblies embedded in a single-file app. If the path to the app directory is needed, consider calling 'System.AppContext.BaseDirectory'.</value> | ||
</data> | ||
<data name="AvoidAssemblyGetFilesInSingleFile" xml:space="preserve"> | ||
<value>'{0}' will throw for assemblies embedded in a single-file app.</value> | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This resource string is used as both the title and message for There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Actually I meant to re-use AvoidAssemblyLocationInSingleFileTitle, since the title actually makes sense for both. good catch. |
||
</data> | ||
</root> |
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
@@ -0,0 +1,158 @@ | ||||||
// Copyright (c) Microsoft. All Rights Reserved. Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. | ||||||
|
||||||
using System; | ||||||
using System.Collections.Generic; | ||||||
using System.Collections.Immutable; | ||||||
using Analyzer.Utilities; | ||||||
using Analyzer.Utilities.Extensions; | ||||||
using Microsoft.CodeAnalysis; | ||||||
using Microsoft.CodeAnalysis.Diagnostics; | ||||||
using Microsoft.CodeAnalysis.Operations; | ||||||
|
||||||
namespace Microsoft.NetCore.Analyzers.Publish | ||||||
{ | ||||||
/// <summary> | ||||||
/// CA3000: Do not use Assembly.Location in single-file publish | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fix ID in doc comment? |
||||||
/// </summary> | ||||||
[DiagnosticAnalyzer(LanguageNames.CSharp, LanguageNames.VisualBasic)] | ||||||
public sealed class AvoidAssemblyLocationInSingleFile : DiagnosticAnalyzer | ||||||
{ | ||||||
public const string IL3000 = nameof(IL3000); | ||||||
public const string IL3001 = nameof(IL3001); | ||||||
|
||||||
internal static DiagnosticDescriptor LocationRule = DiagnosticDescriptorHelper.Create( | ||||||
IL3000, | ||||||
new LocalizableResourceString(nameof(MicrosoftNetCoreAnalyzersResources.AvoidAssemblyLocationInSingleFileTitle), | ||||||
MicrosoftNetCoreAnalyzersResources.ResourceManager, typeof(MicrosoftNetCoreAnalyzersResources)), | ||||||
new LocalizableResourceString(nameof(MicrosoftNetCoreAnalyzersResources.AvoidAssemblyLocationInSingleFileMessage), | ||||||
MicrosoftNetCoreAnalyzersResources.ResourceManager, typeof(MicrosoftNetCoreAnalyzersResources)), | ||||||
DiagnosticCategory.Publish, | ||||||
RuleLevel.BuildWarning, | ||||||
description: null, | ||||||
isPortedFxCopRule: false, | ||||||
isDataflowRule: false); | ||||||
|
||||||
internal static DiagnosticDescriptor GetFilesRule = DiagnosticDescriptorHelper.Create( | ||||||
IL3001, | ||||||
new LocalizableResourceString(nameof(MicrosoftNetCoreAnalyzersResources.AvoidAssemblyGetFilesInSingleFile), | ||||||
MicrosoftNetCoreAnalyzersResources.ResourceManager, typeof(MicrosoftNetCoreAnalyzersResources)), | ||||||
new LocalizableResourceString(nameof(MicrosoftNetCoreAnalyzersResources.AvoidAssemblyGetFilesInSingleFile), | ||||||
MicrosoftNetCoreAnalyzersResources.ResourceManager, typeof(MicrosoftNetCoreAnalyzersResources)), | ||||||
DiagnosticCategory.Publish, | ||||||
RuleLevel.BuildWarning, | ||||||
description: null, | ||||||
isPortedFxCopRule: false, | ||||||
isDataflowRule: false); | ||||||
|
||||||
public override ImmutableArray<DiagnosticDescriptor> SupportedDiagnostics => ImmutableArray.Create(LocationRule, GetFilesRule); | ||||||
|
||||||
public override void Initialize(AnalysisContext context) | ||||||
{ | ||||||
context.EnableConcurrentExecution(); | ||||||
context.ConfigureGeneratedCodeAnalysis(GeneratedCodeAnalysisFlags.ReportDiagnostics); | ||||||
|
||||||
context.RegisterCompilationStartAction(context => | ||||||
{ | ||||||
var compilation = context.Compilation; | ||||||
var isSingleFilePublish = context.Options.GetMSBuildPropertyValue( | ||||||
MSBuildPropertyOptionNames.PublishSingleFile, compilation, context.CancellationToken); | ||||||
if (!string.Equals(isSingleFilePublish?.Trim(), "true", StringComparison.OrdinalIgnoreCase)) | ||||||
{ | ||||||
return; | ||||||
} | ||||||
var includesAllContent = context.Options.GetMSBuildPropertyValue( | ||||||
MSBuildPropertyOptionNames.IncludeAllContentForSelfExtract, compilation, context.CancellationToken); | ||||||
if (string.Equals(includesAllContent?.Trim(), "true", StringComparison.OrdinalIgnoreCase)) | ||||||
{ | ||||||
return; | ||||||
} | ||||||
|
||||||
var properties = new List<IPropertySymbol>(); | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Will this data structure be thread-safe given we will get concurrent callbacks? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Might be best to use an ImmutableHashSet/ImmutableArray for these. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If you decide to use ImmutableHashSet, then you can use this extension method:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This should be safe given that threading model. ImmutableHashSet is probably a bad idea for lists of < 8 elements. I can move to ImmutableArray instead. |
||||||
var methods = new List<IMethodSymbol>(); | ||||||
|
||||||
if (compilation.TryGetOrCreateTypeByMetadataName(WellKnownTypeNames.SystemReflectionAssembly, out var assemblyType)) | ||||||
{ | ||||||
// properties | ||||||
addIfNotNull(properties, tryGetSingleSymbol<IPropertySymbol>(assemblyType.GetMembers("Location"))); | ||||||
|
||||||
// methods | ||||||
methods.AddRange(assemblyType.GetMembers("GetFile").OfType<IMethodSymbol>()); | ||||||
methods.AddRange(assemblyType.GetMembers("GetFiles").OfType<IMethodSymbol>()); | ||||||
} | ||||||
|
||||||
if (compilation.TryGetOrCreateTypeByMetadataName(WellKnownTypeNames.SystemReflectionAssemblyName, out var assemblyNameType)) | ||||||
{ | ||||||
addIfNotNull(properties, tryGetSingleSymbol<IPropertySymbol>(assemblyNameType.GetMembers("CodeBase"))); | ||||||
addIfNotNull(properties, tryGetSingleSymbol<IPropertySymbol>(assemblyNameType.GetMembers("EscapedCodeBase"))); | ||||||
} | ||||||
|
||||||
context.RegisterOperationAction(operationContext => | ||||||
{ | ||||||
var access = (IPropertyReferenceOperation)operationContext.Operation; | ||||||
var property = access.Property; | ||||||
if (!contains(properties, property, SymbolEqualityComparer.Default)) | ||||||
{ | ||||||
return; | ||||||
} | ||||||
|
||||||
operationContext.ReportDiagnostic(access.CreateDiagnostic(LocationRule, property)); | ||||||
}, OperationKind.PropertyReference); | ||||||
|
||||||
context.RegisterOperationAction(operationContext => | ||||||
{ | ||||||
var invocation = (IInvocationOperation)operationContext.Operation; | ||||||
var targetMethod = invocation.TargetMethod; | ||||||
if (!contains(methods, targetMethod, SymbolEqualityComparer.Default)) | ||||||
{ | ||||||
return; | ||||||
} | ||||||
|
||||||
operationContext.ReportDiagnostic(invocation.CreateDiagnostic(GetFilesRule, targetMethod)); | ||||||
}, OperationKind.Invocation); | ||||||
|
||||||
return; | ||||||
|
||||||
static bool contains<T, TComp>(List<T> list, T elem, TComp comparer) | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We follow the Roslyn IDE conventions in this repo for local function names
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yuck :) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'll do it, but I'd like to register my objection as the designer and author of local functions 😄 |
||||||
where TComp : IEqualityComparer<T> | ||||||
{ | ||||||
foreach (var e in list) | ||||||
{ | ||||||
if (comparer.Equals(e, elem)) | ||||||
{ | ||||||
return true; | ||||||
} | ||||||
} | ||||||
return false; | ||||||
} | ||||||
|
||||||
static TSymbol? tryGetSingleSymbol<TSymbol>(ImmutableArray<ISymbol> members) where TSymbol : class, ISymbol | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
{ | ||||||
TSymbol? candidate = null; | ||||||
foreach (var m in members) | ||||||
{ | ||||||
if (m is TSymbol tsym) | ||||||
{ | ||||||
if (candidate is null) | ||||||
{ | ||||||
candidate = tsym; | ||||||
} | ||||||
else | ||||||
{ | ||||||
return null; | ||||||
} | ||||||
} | ||||||
} | ||||||
return candidate; | ||||||
} | ||||||
|
||||||
static void addIfNotNull<TSymbol>(List<TSymbol> properties, TSymbol? p) where TSymbol : class, ISymbol | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
{ | ||||||
if (p is not null) | ||||||
{ | ||||||
properties.Add(p); | ||||||
} | ||||||
} | ||||||
}); | ||||||
} | ||||||
} | ||||||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we want to create issue(s) on https://github.com/MicrosoftDocs/visualstudio-docs/issues to add documentation for the rules at https://docs.microsoft.com/visualstudio/code-quality/il3000 and https://docs.microsoft.com/visualstudio/code-quality/il3001?