Enable single file analyzer in the runtime

[tlakollo]

Delete ruleset for single file related warnings IL3000 and IL3001
Add ILLinkRoslyn analyzer and EnableSingleFileAnalyzer flag
Resolve warnings on single file dangerous patterns by:

• Annotating with RequiresAssemblyFiles
• Suppressing the warning
• Fixing the issue on code
• Opening an issue to track fix and either annotate/suppress the current behavior

[ghost]

Tagging subscribers to this area: @agocke, @vitek-karas, @VSadov
See info in area-owners.md if you want to be subscribed.

Issue Details

---

Delete ruleset for single file related warnings IL3000 and IL3001
Add ILLinkRoslyn analyzer and EnableSingleFileAnalyzer flag
Resolve warnings on single file dangerous patterns by:

• Annotating with RequiresAssemblyFiles
• Suppressing the warning
• Fixing the issue on code
• Opening an issue to track fix and either annotate/suppress the current behavior

Author:	tlakollo
Assignees:	tlakollo
Labels:	area-Single-File
Milestone:	-

[tlakollo]

Sending to CI to catch possible misses caused by:

• Code for an OS different from Windows
• Mono builds

Also, APICompat didn't fail on my computer although I added several Custom Attributes so I'm letting the CI run directly on the changes

[vitek-karas]

This should be propagated to the public API (Default property) - I would expect the analyzer to generate some warnings around this. If not it's a bug in the analyzer which we should fix.

[tlakollo]

I tried a little bit today to debug this, seems like a more complicated scenario than I expected. The analyzer is only able to see System.Lazy.Value.get but I have not found a way to connect it with the function is calling (LoadDefault). Also, I tested a similar scenario to see how does the linker behaves using RequiresUnreferencedCode and I found that they both have the same testing hole and don't produce the warning.

public static void Main ()
{
  TestCallsLazyInitializer ();
}

public static Lazy<RequiresUnreferencedCodeCapability> lazyRUC = new Lazy<RequiresUnreferencedCodeCapability> (InitRUC);

[RequiresUnreferencedCode ("Message for --RequiresUnreferencedCodeWithLazyInit--")]
public static RequiresUnreferencedCodeCapability InitRUC ()
{
	RequiresUnreferencedCodeCapability objectRUC = new RequiresUnreferencedCodeCapability ();
	return objectRUC;
}

[ExpectedWarning ("IL2026", "--RequiresUnreferencedCodeWithLazyInit--")]
public static void TestCallsLazyInitializer ()
{
	RequiresUnreferencedCodeCapability objectRUC = lazyRUC.Value;
}

[MichalStrehovsky]

That looks like dotnet/linker#1912 and related.

We should get a warning in the static constructor, when the Lazy instance is getting constructed.

[tlakollo]

With dotnet/linker#1964 now the Lazy call showed a warning, but since it warns on an implicit static constructor (I cannot annotate the static constructor unless I implement it) and then gets assigned to a field (which is a target that RequiresAssembly files don't support) I couldn't mark it with RequiresAssemblyFiles. I ended up refactoring the code and not make use of the Lazy call.

[vitek-karas]

@eerhardt What is your take on some of the cases where we suppress warnings and file issues on them. I'm not a fan since those are real problems we're hiding. On the other hand propagating these warnings feels like it would affect too large of an API surface.
For trimming we took the approach of warnings anyway I think, so I don't want to set a bad precedent if we want to stick to that behavior.

[vitek-karas]

What would happen if we didn't suppress this - how much would it spread (propagating the attribute to affected public APIs).
The functionality will not work in single-file - so silencing it until it gets fixed is wrong. The warnings are there for this specific reason.

[tlakollo]

Adding [RequiresAssemblyFiles] in this function doesn't propagate to other places. I think is a limitation in the analyzer, because the attribute is on an override, but the methods call a virtual function in LicenseContext.cs
If propagated to the callers, then GetCurrentContextInfo uses LicenseUsageMode.Designtime and it's called by COM so it doesn't propagate more.
The other caller is GetLicense which uses LicenseUsageMode.Runtime, for me is uncertain if that will result in calling the override in DesigntimeLicenseContext. But that method has about 10 different call sites that would need to be annotated most of them in System.ComponentModel.LicenseManger.

[vitek-karas]

and it's called by COM so it doesn't propagate more.

Well - it doesn't propagate in the analyzer - but functionally it does propagate - so this is the killer argument for me. It would basically mean we should potentially annotate the entire COM this way.

Regarding the analyzer: We should add a similar check to what RequiresUnreferencedCode has (at least in the linker) - that is, all methods in a virtual override chain should have the same annotations. So if an override has the attribute, the base method should have it as well, and vice-versa. If this is not the case, please file a bug for this.

[tlakollo]

I opened dotnet/linker#1985 to track work for virtual calls in the analyzers

[tlakollo]

Since this PR has been passing a couple of times the only question that I have is why API compat doesn't warn on these new attributes, I didn't add any code to the ref assemblies shouldn't this suppose to break the build?
cc @safern

[safern]

Since this PR has been passing a couple of times the only question that I have is why API compat doesn't warn on these new attributes, I didn't add any code to the ref assemblies shouldn't this suppose to break the build?

Yes this should be failing. Let me try and reproduce this locally and debug it to see what's going on.

[safern]

It seems like the APICompat issue was a regression I introduced when adding support to diff attributes on type parameters and method parameters. I put up a fix for this regression: dotnet/arcade#7376

[safern]

@tlakollo the version of API Compat containing my fix is being updated in: #52440. So once that is merged you should be unblocked to do a build rebasing on top of main and getting errors of attribute miss matches.

[tlakollo]

I changed the attributes from UnconditionalSuppressMessage to SuppressMessage since there is no reason at this moment to have the extra IL, since the analysis is being conducted via analyzers is not necessary.

[tlakollo]

For the System.IO.IsolatedStorage I will need review and guidance on how to test that my changes don't change the way the app behaves for both single-file and non single-file
Adding @JeremyKuhne @adamsitnik @carlossanlop @jozkee @jeffhandley

[MichalStrehovsky]

I changed the attributes from UnconditionalSuppressMessage to SuppressMessage since there is no reason at this moment to have the extra IL, since the analysis is being conducted via analyzers is not necessary.

This means NativeAOT will not be able to implement the single file warnings since the attributes need to be in IL as per #50894 (comment). SuppressMessage is no different from #pragma warning disable.

[vitek-karas]

This means NativeAOT will not be able to implement the single file warnings

Can we rely on the analyzers instead?
So far the single-file analysis can be done locally (there's nothing which requires global view), which makes it much more suitable for analyzers. The disadvantages are:

• It's "viral", in that any single component which is not single-file compatible can break the app, and so only global analyzers can tell if the app as a whole is compatible
• Analyzers are C#/VB only

For now we've been thinking about this as very similar to all the other analyzers - most specifically the platform analyzer, which is basically identical ("viral" as well). And the outcome was that unless we can solve this for all of the analyzers, it makes little sense to try to solve it for one specific one alone.

(Note: Trimming is different in that it needs to run on IL and needs to be global)

I'm a bit split on this one - it would help in NativeAOT with a more predictable experience... but to be fully predictable we would also have to support global analysis for things like platform compatibility and so on...

[MichalStrehovsky]

I look at the single file analyzer as a stopgap until we have a better spot for this. I'll just quote from #50894 (comment).

"From my past experience, every time someone hit an issue with APIs like Assembly.Location and couldn't figure it out, it was in a third party NuGet. If they hit it in their own code, they could usually just debug into it. E.g. the CoreRT repo is full of them; here's one: dotnet/corert#7756."

Basically you get a lot more payoff from the analyzer warning if it warns you about code that is not yours. The null from Assembly.Location results in so many ArgumentNullException that I always start my investigations with that - and as someone fluent with ILSpy I can usually debug it in 2 minutes. Here's another grpc/grpc#18188 loooong thread with a lot of angry/frustrated people. It would be much better if we could just issue a warning that the NuGet package they're using is not compatible with single file deployments than trying to get a message across a busy thread with a lot of frustrated people and package maintainers uninterested in fixing the issue of their users.

[MichalStrehovsky]

(The interesting part of the GRPC thread starts at grpc/grpc#18188 (comment) where what was a .NET Native bug that we fixed made forward progress to hit a GRPC bug)

[vitek-karas]

OK - makes sense. I still hate the fact that we're solving these things as one-offs (platform analyzer has this problem as well, without any solution) - also this will only work in native AOT - JITed single-file will not show any warning and break anyway.
But I guess it makes sense to use UnconditionalSuppressMessage in the hopes that one day we will have a general global analyzer for these things (which would then use the attribute).

@tlakollo Sorry for the back and forth on this - can you please revert back to using UnconditionalSuppressMessage - and we should also stick with the code-fixer for it as well I guess. While there's very little use for it today (Native AOT is the only one), hopefully this will make it future proof.

[tlakollo]

I was able to test that executing in runtime\src\libraries\System.IO.IsolatedStorage\tests the command dotnet build -t:test -p:TestSingleFile=true the end result with the previous code would be 235 failures out of 267 test with the same failure signature
235

[FAIL] System.IO.IsolatedStorage.RemoveTests.RemoveUserStoreForDomain
  System.NotSupportedException : CodeBase is not supported on assemblies loaded from a single-file bundle.
     at System.Reflection.RuntimeAssembly.get_CodeBase() in System.Private.CoreLib.dll:token 0x6004e08+0x1b
     at System.IO.IsolatedStorage.Helper.GetDefaultIdentityAndHash(Object& identity, String& hash, Char separator) in System.IO.IsolatedStorage.dll:token 0x60000af+0x24
     at System.IO.IsolatedStorage.IsolatedStorage.InitStore(IsolatedStorageScope scope, Type domainEvidenceType, Type assemblyEvidenceType) in System.IO.IsolatedStorage.dll:token 0x60000a6+0x1a
     at System.IO.IsolatedStorage.IsolatedStorageFile..ctor(IsolatedStorageScope scope) in System.IO.IsolatedStorage.dll:token 0x600002a+0x13
     at System.IO.IsolatedStorage.IsolatedStorageFile.GetStore(IsolatedStorageScope scope) in System.IO.IsolatedStorage.dll:token 0x6000050+0x1
     at System.IO.IsolatedStorage.IsolatedStorageFile.GetUserStoreForDomain() in System.IO.IsolatedStorage.dll:token 0x600004b+0x1
     at System.IO.IsolatedStorage.RemoveTests.RemoveUserStoreForDomain() in A:\Repos\SingleFileAnalyzerOnRuntime\runtime\src\libraries\System.IO.IsolatedStorage\tests\System\IO\IsolatedStorage\RemoveTests.cs:line 45
  Finished System.IO.IsolatedStorage.Tests, Version=6.0.0.0, Culture=neutral, PublicKeyToken=cc7b13ffcd2ddd51

After the code change the 267 test passed, although I'm not 100% sure that there is a test that exercises the particular scenario, I will update this PR with a change in runtime/src/libraries/tests.proj to enable running System.IO.IsolatedStorage tests as a single file.