Undocumented linker warning IL3000

[mateoatr]

D:\tests\foo\Program.cs(1,26): warning IL3000: 'System.Reflection.Assembly.Location' 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'. [D:\tests\foo\foo.csproj]

Not sure where this warning is coming from, it doesn't seem to be coming out of the linker.
Repro:

System.Console.WriteLine(System.Reflection.Assembly.GetExecutingAssembly().Location);

Publish as single-file: dotnet publish /p:PublishSingleFile=true -r win-x64

[agocke]

It's a new analyzer I added to the SDK. Looks like it's working! Documented here: https://docs.microsoft.com/en-us/visualstudio/code-quality/il3000?view=vs-2019

[mateoatr]

Ah, yes, roslyn-analyzers. Should we keep track of these warning codes here as well?

[agocke]

We could mark them reserved, and maybe have a link to the authoritative location, but I don't think we should try to duplicate any info.

[marek-safar]

This is pretty bad! Didn't we spend time investigating that there will be no prefix collision to avoid exactly this situation? Using the same error code prefix for two very different tools in the same output will be confusing.

What about changing illinker messages prefix to TR while we still can?

[MichalStrehovsky]

I think the IL prefix was chosen on purpose: dotnet/roslyn-analyzers#3921 and I would expect we'll want to make sure the warning means the same thing in the linker. For the user it doesn't matter what tool this comes from if the fix is the same.

[MichalStrehovsky]

One thing that occurred to me that even though we would share error codes across analyzers and linker, the suppression mechanism is still different.

We might need to ask Roslyn team to do dotnet/roslyn#43639 sooner than "backlog". @agocke can you help with that?

[marek-safar]

That still does not make much sense to me. Why would we want to use IL prefix for warnings triggered by a different tool? I think it matters for users and developers to know which tool produced the warnings, your argument that it does not matter if the fix is same is like saying C#/VB/F# compilers should not use their own prefixes because if the error is missing assembly reference the fix is always same in VS.

We are not using this approach for any other analyzers (e.g. about incompatible APIs) why did we introduce it for single error code without conceptual agreement with other analyzers do similar checks?

[MichalStrehovsky]

If we use different error codes between analyzer and linker, once we introduce a Roslyn analyzer for reflection dataflow, all the places that currently suppress dataflow warnings will require a second annotation to also suppress the warning from the Roslyn analyzer. Here's the spots we have in the runtime repo, just as an example: https://github.com/dotnet/runtime/search?q=unconditionalsuppressmessage&unscoped_q=unconditionalsuppressmessage

The IL3000 is just another warning in the same category - this one is about single-file friendliness. If I understand it correctly, we still want to implement single-file-friendliness analysis in the linker too. So we would need two suppressions for that too.

E.g. I was just pinged on https://github.com/migueldeicaza/gui.cs/pull/900/files#diff-3895d7279494ba0a70c7409929524d17R266. The call to Marshal.GetHINSTANCE is single-file unfriendly, but it kind of works for the use they want to use it. Once we have warnings in place for all of single-file unfriendliness, this is going to warn from both the Roslyn analyzer and from linker. Should we really require the user to put two suppressions on this?

---

But on a different note, I don't think the Roslyn analyzer thing is the approach we want to take - we'll probably want to have something like RequiresUnreferencedCodeAttribute but for single-file-unfriendliness - any APIs that are unfriendly would be marked by that. There would be a single error code for all of "not friendly for single file". This is different from how the Roslyn analyzer does it, where it invents a new error code for each unfriendly API. Maybe we should revert that, if it's not too late.

[marek-safar]

This concept is more complicated than just few APIs for coreclr's single file and we should not set a precedent for something that we haven't designed and validated as the solution we'll ever use.

I suggest we undo the single-file analyzer attempt in .NET5 and rely on documentation only for such APIs and work on general solution for .NET6.

We ship unsupported APIs analyzer in .NET5 which is used to validate APIs friendliness for various workloads and it's not related to linker at all. It's Roslyn analyzer based and it checks for API X to be supported on configuration Y. I think it'd be a better to keep evolving this tool instead. It has its own limitations as well around flow analysis but why not address them consistently?

[agocke]

To give an update here: I think the appropriate comparison is between the analyzer support for warning about RequiresUnreferencedCode.

After having used the analyzer and linker, I don't think it's acceptable to remove either warning. They're both too useful and provide advantages the other doesn't.

I think the only other question was whether or not they should have the same diagnostic ID. The advantage seems clear: you can easily suppress both using UnconditionalSuppressMessage. I'm not clear on the disadvantage, or the advantage for having different warning identifiers.