Add an analyzer for PublishSingleFile #3921

agocke · 2020-07-29T07:31:37Z

When publishing single-file applications, certain APIs are very likely
to return incorrect results. Most notably, asking for the Location of
any Assembly loaded from the single-file will return an empty string,
instead of a file path. To minimize suprise, this analyzer flags all
usages of the APIs most likely to break when used in single-file when
the PublishSingleFile property is set to true, and the
IncludeAllContentForSelfExtractProperty is not true (which provides the
legacy API behavior).

agocke · 2020-07-29T07:34:15Z

@vitek-karas @sbomer @mateoatr @tlakollo @samsp-msft Can you all take a look at this and tell me what you think? There are certainly some instances with potential false positives (as I included in my test case), but it seems like this mistake is so easy to make and this scenario is so new that it would be better to produce extra warnings up front.

When publishing single-file applications, certain APIs are very likely to return incorrect results. Most notably, asking for the Location of any Assembly loaded from the single-file will return an empty string, instead of a file path. To minimize suprise, this analyzer flags all usages of the APIs most likely to break when used in single-file when the PublishSingleFile property is set to true, and the IncludeAllContentForSelfExtractProperty is not true (which provides the legacy API behavior).

codecov · 2020-07-29T08:09:01Z

Codecov Report

Merging #3921 into master will decrease coverage by 0.00%.
The diff coverage is 99.18%.

@@            Coverage Diff             @@
##           master    #3921      +/-   ##
==========================================
- Coverage   95.62%   95.61%   -0.01%     
==========================================
  Files        1151     1151              
  Lines      254095   253903     -192     
  Branches    15223    15209      -14     
==========================================
- Hits       242977   242773     -204     
- Misses       9180     9182       +2     
- Partials     1938     1948      +10

vitek-karas · 2020-07-29T09:02:35Z

src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/MicrosoftNetCoreAnalyzersResources.resx

@@ -1422,4 +1422,13 @@
  <data name="DoNotUseOutAttributeStringPInvokeParametersTitle" xml:space="preserve">
    <value>Do not use 'OutAttribute' on string parameters for P/Invokes</value>
  </data>
+  <data name="AvoidAssemblyLocationInSingleFileDescription" xml:space="preserve">
+    <value>Accessing Assembly location or file path is not valid when publishing as a single-file.</value>


I don't like the word "valid" - it makes it sound like something completely wrong. Maybe just explain what it does:
"Property 'Assembly.Location' returns empty string when publishing the app as a single-file."

But I'm not a native speaker, so maybe your version actually sounds better...

Actually maybe we should include the fact that the assembly has to be part of the app:
"Property Assembly.Location returns empty string for any assembly from application when published as single-file"

Saying it returns empty string is better as it's then a condition the app can check against.

In that case we'll probably need individual diagnostic rules for each API to describe its behavior.

I do agree that having one for each is lot of work - on the other hand it might give us opportunity to include some actionable feedback in the messages (if there's any). For example for Assembly.Location we may point users to AppDomain.CurrentDomain.BaseDirectory as a reasonable replacement for certain cases.

vitek-karas · 2020-07-29T09:15:57Z

src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/Publish/AvoidAssemblyLocationInSingleFile.cs

+                    addIfNotNull(properties, tryGetSingleSymbol<IPropertySymbol>(assemblyType.GetMembers("CodeBase")));
+                    addIfNotNull(properties, tryGetSingleSymbol<IPropertySymbol>(assemblyType.GetMembers("EscapedCodeBase")));


These are now obsolete (dotnet/runtime#39261) - not sure what's the UX - will I get two warnings for the same thing?

Probably. I didn't know we were going to obsolete them, so I can remove this.

src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/MicrosoftNetCoreAnalyzersResources.resx

src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/Publish/AvoidAssemblyLocationInSingleFile.cs

Evangelink · 2020-07-29T14:18:12Z

src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/Publish/AvoidAssemblyLocationInSingleFile.cs

+                var assemblyType = compilation.GetOrCreateTypeByMetadataName(WellKnownTypeNames.SystemReflectionAssembly);
+                if (assemblyType is not null)


There is a TryGetOrCreateTypeByMetadataName method you could use here instead.

src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/Publish/AvoidAssemblyLocationInSingleFile.cs

Evangelink · 2020-07-29T14:22:27Z

...zers/UnitTests/Microsoft.NetCore.Analyzers/Publish/AvoidAssemblyLocationInSingleFileTests.cs

+}";
+            return VerifyDiagnosticsAsync(src,
+                // /0/Test0.cs(8,13): warning CA3000: Avoid `System.Reflection.Assembly.Location` when publishing as a single-file. Assemblies inside a single-file bundle do not have a file or file path. If the path to the app directory is needed, consider calling System.AppContext.BaseDirectory.
+                VerifyCS.Diagnostic().WithSpan(8, 13, 8, 23).WithArguments("System.Reflection.Assembly.Location"),


Could you please use the markup syntax instead? It looks like {|#0:a.Location|} (where 0 is the index) and then to test the arg you can do VerifyCS.Diagnostic.WithLocation(0).WithArgs....

This style of diagnostics verification is what Roslyn uses for all the compiler tests. Why invent something different here? I don't see the value making the source code harder to read.

…CoreAnalyzersResources.resx Co-authored-by: Amaury Levé <evangelink@gmail.com>

…dAssemblyLocationInSingleFile.cs Co-authored-by: Amaury Levé <evangelink@gmail.com>

src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/MicrosoftNetCoreAnalyzersResources.resx

src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/Publish/AvoidAssemblyLocationInSingleFile.cs

…CoreAnalyzersResources.resx Co-authored-by: Amaury Levé <evangelink@gmail.com>

…dAssemblyLocationInSingleFile.cs Co-authored-by: Amaury Levé <evangelink@gmail.com>

vitek-karas · 2020-07-30T10:02:42Z

src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/MicrosoftNetCoreAnalyzersResources.resx

+  </data>
+  <data name="AvoidAssemblyGetFilesInSingleFile" xml:space="preserve">
+    <value>Assemblies embedded in a single-file app cannot have additional files in the manifest.</value>


Maybe it would be better to similarly to Location describe the actual behavior. In this case the API is going to throw an exception (it already does for in-memory assemblies outside of single-file).

The issue at dotnet/runtime#38405 seems to imply the expected behavior is to return null or empty array.

We'll probably have to update that issue. More recent discussions:

Assembly.CodeBase: Assembly.CodeBase returns nonexistent path for assemblies from single-file runtime#40087 - the agreement is to actually throw PlatformNotSupported - it's "safer" and we're obsoleting these anyway, so making them work feels contraproductive.

Module.FullyQualifiedName Module.FullyQualifiedName throws in single-file app runtime#40103 - should return <Unknown>. This will make it work the same as memory-loaded assemblies. This also means that GetFile/GetFiles will throw (as it does in non-single-file for memory-loaded assemblies).

What about AssemblyName.CodeBase? AFAIK that's not even obsolete

I personally would expect Assembly.CodeBase and AssemblyName.CodeBase to behave the same. There has been discussion on it, but it sort of disappeared: dotnet/runtime#31127
I'll start it again.

sbomer · 2020-07-30T17:41:59Z

Is the plan to share warning codes with a future single-file analysis done by the linker? If so we should make sure to reserve IL3000/IL3001 in mono/linker to avoid conflicts - or if not we might want to use a different prefix here.

agocke · 2020-07-30T17:48:26Z

@sbomer Yeah this was basically a set up for that exact idea. That we could move warnings back and forth between the linker and this analyzer framework by re-using error codes. I agree, it seems like a good idea to reserve those codes in the linker. Do you know how I should do that?

sbomer · 2020-07-30T18:03:49Z

I filed dotnet/linker#1393 to document this - I think it's enough for now. If we wanted to we could add a check to ensure we don't use those warning numbers.

tlakollo · 2020-07-30T18:09:54Z

So I'm assuming this means 1XXX for linker and single file errors(?), 2XXX for linker warnings 3XXX for single file warnings

agocke · 2020-07-30T18:16:49Z

@tlakollo Yup, that was the idea.

mavasani · 2020-07-30T18:17:03Z

src/NetAnalyzers/Core/AnalyzerReleases.Unshipped.md

@@ -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)


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:

No two analyzers share the same diagnostic ID in an analyzer package in this repo

There are no gaps introduced in the diagnostic ID range

Diagnostic ID selected for a specific category falls in the allowed range for that category

mavasani · 2020-07-31T19:52:56Z

src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/Publish/AvoidAssemblyLocationInSingleFile.cs

+namespace Microsoft.NetCore.Analyzers.Publish
+{
+    /// <summary>
+    /// CA3000: Do not use Assembly.Location in single-file publish


Fix ID in doc comment?

mavasani · 2020-07-31T19:55:12Z

src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/Publish/AvoidAssemblyLocationInSingleFile.cs

+
+                return;
+
+                static bool contains<T, TComp>(List<T> list, T elem, TComp comparer)


We follow the Roslyn IDE conventions in this repo for local function names

Suggested change

static bool contains<T, TComp>(List<T> list, T elem, TComp comparer)

static bool Contains<T, TComp>(List<T> list, T elem, TComp comparer)

I'll do it, but I'd like to register my objection as the designer and author of local functions 😄

mavasani · 2020-07-31T19:57:17Z

src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/Publish/AvoidAssemblyLocationInSingleFile.cs

+                    return;
+                }
+
+                var properties = new List<IPropertySymbol>();


Will this data structure be thread-safe given we will get concurrent callbacks?

Might be best to use an ImmutableHashSet/ImmutableArray for these.

If you decide to use ImmutableHashSet, then you can use this extension method:

roslyn-analyzers/src/Utilities/Compiler/Extensions/ImmutableHashSetExtensions.cs

Line 95 in 194d144

public static void AddIfNotNull<T>(this ImmutableHashSet<T>.Builder builder, T? item)

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.

…dAssemblyLocationInSingleFile.cs Co-authored-by: Manish Vasani <mavasani@microsoft.com>

mavasani · 2020-07-31T20:03:22Z

src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/MicrosoftNetCoreAnalyzersResources.resx

+  </data>
+  <data name="AvoidAssemblyGetFilesInSingleFile" xml:space="preserve">
+    <value>'{0}' will throw for assemblies embedded in a single-file app.</value>


This resource string is used as both the title and message for IL3001. Title string should not have any string format arguments, so you probably want to use separate resource strings.

Actually I meant to re-use AvoidAssemblyLocationInSingleFileTitle, since the title actually makes sense for both. good catch.

mavasani · 2020-07-31T20:04:25Z

src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/Publish/AvoidAssemblyLocationInSingleFile.cs

+                    return false;
+                }
+
+                static TSymbol? tryGetSingleSymbol<TSymbol>(ImmutableArray<ISymbol> members) where TSymbol : class, ISymbol


Suggested change

static TSymbol? tryGetSingleSymbol<TSymbol>(ImmutableArray<ISymbol> members) where TSymbol : class, ISymbol

static TSymbol? TryGetSingleSymbol<TSymbol>(ImmutableArray<ISymbol> members) where TSymbol : class, ISymbol

mavasani · 2020-07-31T20:04:34Z

src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/Publish/AvoidAssemblyLocationInSingleFile.cs

+                    return candidate;
+                }
+
+                static void addIfNotNull<TSymbol>(List<TSymbol> properties, TSymbol? p) where TSymbol : class, ISymbol


Suggested change

static void addIfNotNull<TSymbol>(List<TSymbol> properties, TSymbol? p) where TSymbol : class, ISymbol

static void AddIfNotNull<TSymbol>(List<TSymbol> properties, TSymbol? p) where TSymbol : class, ISymbol

mavasani · 2020-07-31T20:07:12Z

src/NetAnalyzers/Core/AnalyzerReleases.Unshipped.md

@@ -43,6 +43,9 @@ CA2361 | Security | Disabled | DoNotUseDataSetReadXml, [Documentation](https://d
 CA2362 | Security | Disabled | DataSetDataTableInSerializableTypeAnalyzer, [Documentation](https://docs.microsoft.com/visualstudio/code-quality/ca2362)
 CA2363 | Security | Disabled | DataSetDataTableInSerializableTypeAnalyzer, [Documentation](https://docs.microsoft.com/visualstudio/code-quality/ca2363)

+IL3000 | Publish  | Warning  | DoNotUseAssemblyLocationInSingleFile, [Documentation](https://docs.microsoft.com/visualstudio/code-quality/il3000)


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?

mavasani

Overall LGTM, just have some minor nit suggestions.

agocke · 2020-08-03T19:05:43Z

@mavasani You can merge this if you want, I don't have commit privileges for this repo

mavasani · 2020-08-03T19:10:44Z

You can merge this if you want, I don't have commit privileges for this repo

Interesting, I though all roslyn team members have the permissions. Let me explicitly add you.

mavasani · 2020-08-03T19:11:50Z

@agocke Can you try again?

agocke · 2020-08-03T19:12:37Z

@mavasani I'm not a Roslyn team member any more 😄

carlossanlop · 2020-08-03T22:35:50Z

@agocke FYI, the change made to the file src/NetAnalyzers/Core/AnalyzerReleases.Unshipped.md added a space before the two new table rows, and markdown is not including them in the table:

https://github.com/dotnet/roslyn-analyzers/blob/master/src/NetAnalyzers/Core/AnalyzerReleases.Unshipped.md

agocke · 2020-08-04T02:08:53Z

Woops, fixed here #3944

agocke force-pushed the single-file branch from b3a7541 to ffb9f66 Compare July 29, 2020 07:40

vitek-karas reviewed Jul 29, 2020

View reviewed changes

Evangelink reviewed Jul 29, 2020

View reviewed changes

agocke and others added 4 commits July 29, 2020 14:29

Update src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/MicrosoftNet…

5920d19

…CoreAnalyzersResources.resx Co-authored-by: Amaury Levé <evangelink@gmail.com>

Update src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/MicrosoftNet…

24e739d

…CoreAnalyzersResources.resx Co-authored-by: Amaury Levé <evangelink@gmail.com>

Update src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/Publish/Avoi…

9347350

…dAssemblyLocationInSingleFile.cs Co-authored-by: Amaury Levé <evangelink@gmail.com>

Update src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/Publish/Avoi…

a9ca099

…dAssemblyLocationInSingleFile.cs Co-authored-by: Amaury Levé <evangelink@gmail.com>

Evangelink reviewed Jul 29, 2020

View reviewed changes

src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/MicrosoftNetCoreAnalyzersResources.resx Outdated Show resolved Hide resolved

src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/Publish/AvoidAssemblyLocationInSingleFile.cs Outdated Show resolved Hide resolved

agocke and others added 3 commits July 29, 2020 18:06

Update src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/MicrosoftNet…

26e5052

…CoreAnalyzersResources.resx Co-authored-by: Amaury Levé <evangelink@gmail.com>

Update src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/Publish/Avoi…

887f95f

…dAssemblyLocationInSingleFile.cs Co-authored-by: Amaury Levé <evangelink@gmail.com>

Respond to PR comments

d62018c

vitek-karas reviewed Jul 30, 2020

View reviewed changes

mavasani reviewed Jul 30, 2020

View reviewed changes

agocke added 2 commits July 30, 2020 13:31

Adjust warning message

7cbb603

Update resources and diagnostic ranges

1c1418f

vitek-karas mentioned this pull request Jul 30, 2020

Assembly.LoadFrom works on files inside the single-file bundle dotnet/runtime#40138

Closed

agocke marked this pull request as ready for review July 31, 2020 19:41

agocke requested a review from a team as a code owner July 31, 2020 19:41

mavasani reviewed Jul 31, 2020

View reviewed changes

Merge remote-tracking branch 'upstream/master' into single-file

24ebb30

mavasani reviewed Jul 31, 2020

View reviewed changes

Update src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/Publish/Avoi…

6b9ea80

…dAssemblyLocationInSingleFile.cs Co-authored-by: Manish Vasani <mavasani@microsoft.com>

mavasani reviewed Jul 31, 2020

View reviewed changes

mavasani approved these changes Jul 31, 2020

View reviewed changes

Respond to PR comments

be5822b

agocke merged commit 0c78e1e into dotnet:master Aug 3, 2020

agocke deleted the single-file branch August 3, 2020 19:13

MichalStrehovsky mentioned this pull request Sep 19, 2020

Undocumented linker warning IL3000 dotnet/linker#1498

Closed

LakshanF mentioned this pull request Sep 24, 2020

Single file documentation update dotnet/docs#20761

Merged

mateoatr mentioned this pull request Jan 8, 2021

Add ILLink Analyzers dotnet/sdk#15239

Merged

		addIfNotNull(properties, tryGetSingleSymbol<IPropertySymbol>(assemblyType.GetMembers("CodeBase")));
		addIfNotNull(properties, tryGetSingleSymbol<IPropertySymbol>(assemblyType.GetMembers("EscapedCodeBase")));

		var assemblyType = compilation.GetOrCreateTypeByMetadataName(WellKnownTypeNames.SystemReflectionAssembly);
		if (assemblyType is not null)


		return;

		static bool contains<T, TComp>(List<T> list, T elem, TComp comparer)

	static TSymbol? tryGetSingleSymbol<TSymbol>(ImmutableArray<ISymbol> members) where TSymbol : class, ISymbol
	static TSymbol? TryGetSingleSymbol<TSymbol>(ImmutableArray<ISymbol> members) where TSymbol : class, ISymbol

	static void addIfNotNull<TSymbol>(List<TSymbol> properties, TSymbol? p) where TSymbol : class, ISymbol
	static void AddIfNotNull<TSymbol>(List<TSymbol> properties, TSymbol? p) where TSymbol : class, ISymbol

Add an analyzer for PublishSingleFile #3921

Add an analyzer for PublishSingleFile #3921

Conversation

agocke commented Jul 29, 2020

agocke commented Jul 29, 2020

codecov bot commented Jul 29, 2020 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sbomer commented Jul 30, 2020

agocke commented Jul 30, 2020

sbomer commented Jul 30, 2020

tlakollo commented Jul 30, 2020

agocke commented Jul 30, 2020

mavasani Jul 30, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mavasani left a comment

Choose a reason for hiding this comment

agocke commented Aug 3, 2020

mavasani commented Aug 3, 2020

mavasani commented Aug 3, 2020

agocke commented Aug 3, 2020

carlossanlop commented Aug 3, 2020

agocke commented Aug 4, 2020

codecov bot commented Jul 29, 2020 •

edited

Loading

mavasani Jul 30, 2020 •

edited

Loading