Add design doc for single-file unsupported attribute

[mateoatr]

This goes a little bit further into explaining the rationale for adding this new attribute. Note that the tool responsible of the interpretation of such attribute will primarily be the linker, which has the possibility to perform whole-program analysis.

[marek-safar]

This does not address the problem for library authors. How are the library authors going to know where to add the attribute?

[vitek-karas]

Agreed that we should expand on the experience for library authors - something like:

• Low level APIs will be annotated by us (CodeBase for example) - or will be directly recognized by analyzers
• Library authors will run analyzers on their code and potentially get warnings in places where the library code has issues (by getting warnings from the above)
• They can choose to either fix the library so that it works regardless of single-file
• Or if not possible or desirable, they can use the new attribute to annotate the affected functionality as problematic with single-file

The open question is how will library authors declare the intent to support single-file. This is similar to dotnet/sdk#14562, but libraries are not published typically.
For the purposes of this discussion I would simply say that the library developer can build/publish with /p:PublishSingleFile=true to get the diagnsotics.

[vitek-karas]

The experience for library authors will need to be discussed some more - as using the properties has some issues for libraries (unlike apps, where it is much more fitting). Related to dotnet/runtime#43540 and also dotnet/runtime#43543 (which has a similar problem for trimming).

[mateoatr]

How are the library authors going to know where to add the attribute?

We already have https://docs.microsoft.com/en-us/dotnet/core/deploying/single-file listing the different APIs that are not compatible with single-file. All these are public APIs that will be recognized by the single-file analyzer, so a method that calls any of these would trigger a warning and the developer should either find a way to fix it or annotate the caller method as SingleFileUnsupported.

I agree with @vitek-karas that the tricky part is how will library developers state the intent of wanting their app to be compatible with single-file. I think that for now we can move on with what's proposed in the linked issue, so developers can put PublishSingleFile=true in their csproj and the single-file analyzer will warn them whenever an incompatible API is used.

[marek-safar]

Can we include the list of APIs which will be annotated with this attribute in the proposal?
Is the plan to annotate also PNSE throwing APIs with this attribute (feels like an odd match) ?

[vitek-karas]

I'm not sure about the PNSE throwing APIs - I think it will depend on the specific API. If the main reason why they throw is the fact that the target platform/configuration is single-file, then I think it should be annotated with the attribute. If the main reason is something else, and it just so happens to also affect single-file-ness, maybe the existing platform support annotation should be used.

It also depends on where should PNSE be used. Do we really only use it for things which are just impossible to do on the specific platform (architecture or OS), or do we use it also for cases where the configuration of the app makes it impossible (single-file, trimming, AOT, ...).

[marek-safar]

I guess PNSE should really be RID/platform-specific, otherwise, there is NotSupportedException

[mateoatr]

Can we include the list of APIs which will be annotated with this attribute in the proposal?

I've added them explicitly in the proposal and mentioned them in this doc as the first thing we know we want to annotate with this new attribute.

[marek-safar]

Which single file are you talking about with this attribute? Is this single exe file on windows or single file .app on macOS or single file .apk on Android ?

[vitek-karas]

We need a better name probably - it should not talk about "single-file" but rather the capability... so something like Requires..... In which case I would image it would apply to any "bundle-like" technology, be it single-file, app packages and so on.

[mateoatr]

I saw @MichalStrehovsky brought up this issue too. I'm not sure what name would make the usage intent of the attribute more clear, maybe RequiresFilesOnDisk?

[marek-safar]

What will be the user experience then if developers publish for a single file like app where unannotated nuget calls into annotated nuget with the attribute?

[vitek-karas]

In Roslyn analyzer - none - no warning (as that analyzer won't be aware of the problem).
In linker (global analyzer) - a warning just like any other - there's no distinction between app/library code in that case.

[marek-safar]

Are we going to issue a warning with nuget name and link how to fix it or what the user experience be like? Can you include an example of what this will look like?

[mateoatr]

I think the only source for deciding what to put in the warning message should be the property value in the attribute. For the methods that we plan to annotate with this inside our code, I think the message could clearly state what to do and provide a link to a website that further explains why the method is not compatible with single-file and possible work arounds.

[marek-safar]

I think we need to cover more than the direct call case. My question was about user experience when the call chain is complex. Is linker going to propagate the error message as you suggested through the call chain or are the developers supposed to copy the message or something else?

[marek-safar]

Is suppression really the only way to mitigate the warning? What is the syntax to suppress the warning in external NuGet?

[vitek-karas]

Not sure how common it will be to try to disable the warning in external NuGet - most developers will simply give up at that point (as it's next to impossible for them to tell if the NuGet has a real issue or not).
This will not be a problem for Roslyn analyzers (since there's no external code being analyzed in that case).
For linker we can use the ILLink.LinkAttributes.xml and inject the UnconditionalSuppressMessageAttribute into the target NuGet assembly - not easy or simple, but it will work.

[marek-safar]

There are two questions

1. Is suppression and attribute propagation really the only two ways how to resolve this warning?

2. If I'm building a library that has a dependency on a different library which is not single-file safe how can I make my build warning-free?

Looking at the list of APIs we plan to annotate I suspect we will need some form of Capability logic API. Some of the APIs throw an exception and we will warn developers not to use them but don't give them APIs to do it conditionally, only with try/catch which the analyzer will not recognize.

[vitek-karas]

1. Yes - if that experience is not good enough we can definitely try to add some more, but right now that's the plan
2. The warnings are there for a reason - they typically detect a problem with the library. Fixing that does not mean suppression, sometimes it does, but not always. So if the user wants to ignore these, fine, there are well defined warning suppression mechanisms (like NoWarn).

[marek-safar]

Could we include an example how the fix would look like? Is the linker going to recognize code like bellow as safe or do I have to suppress it?

static string GetPath()
{
	try {
		return typeof(object).Assembly.CodeBase;
	} catch {
		return "";
	}
}

[mateoatr]

I've added an example that illustrates the scenario you point out. The linker (or global analyzer) will not recognize that code as safe, you will have to manually suppress it. I think this is a little bit of a bummer and would definitely like to have an API to guard calls to methods that are not compatible with a specific capability (e.g., single-file), but I think that's out of scope for this doc.

[marek-safar]

I think without capabilities API developers will simply suppress the warning as there is no easy way to add special handling and we'll be where we are today.

[marek-safar]

How is that possible? I'm writing a library that references another library that has SingleFileUnsupported on its API. Where is the library author going to see the warning?

[vitek-karas]

I would maybe rewrite the text here to not talk about libraries directly, just code in general. As in "one method calls another method".
The libraries/apps distinction and experiences should be mentioned as well, but they're not directly related to the exact mechanics of how the attribute works.

[mateoatr]

How is that possible? I'm writing a library that references another library that has SingleFileUnsupported on its API. Where is the library author going to see the warning?

If the API that the library author is calling is annotated with the new attribute, the single-file analyzer will show a warning that should be visible in the IDE. Unfortunately, the author will not see any warnings for cases where the marked method is called indirectly (at least not until we have a global analyzer for this scenarios).

Foo:

[SingleFileUnsupported]
A() { }

B() { A(); }

My app:

using Foo;

// Analyzer will not be able to see the call to A and warn
C() { B(); }

// A warning should be displayed
D() { A(); }

[marek-safar]

the single-file analyzer will show a warning that should be visible in the IDE

How are you going to achieve that? Every developer would have to edit their project and add an unknown custom analyzer, then enable a single-file rule to warn. This does not sound like something developers will find and do.

[vitek-karas]

I don't have an answer for libraries...
For apps, the idea is that specifying PublishSingleFile=true will enable the analyzer - no need to add it to the project, just add that property to the project. This is actually already the case in .NET 5 with the existing single-file Roslyn analyzer.

This would also work for libraries, but I agree that the property is somewhat confusing and not right for libraries, so we may need a different approach.

[marek-safar]

You still need to include the linker analyzer dll somewhere in the project or is it already referenced by default?

[vitek-karas]

The Roslyn analyzer is always "included" - it just disables itself based on some properties in MSBuild - no user interaction necessary. I would image we would do something similar for the global analyzer (be it linker or something else).

[vitek-karas]

Agreed that we should expand on the experience for library authors - something like:

• Low level APIs will be annotated by us (CodeBase for example) - or will be directly recognized by analyzers
• Library authors will run analyzers on their code and potentially get warnings in places where the library code has issues (by getting warnings from the above)
• They can choose to either fix the library so that it works regardless of single-file
• Or if not possible or desirable, they can use the new attribute to annotate the affected functionality as problematic with single-file

The open question is how will library authors declare the intent to support single-file. This is similar to dotnet/sdk#14562, but libraries are not published typically.
For the purposes of this discussion I would simply say that the library developer can build/publish with /p:PublishSingleFile=true to get the diagnsotics.

[vitek-karas]

The experience for library authors will need to be discussed some more - as using the properties has some issues for libraries (unlike apps, where it is much more fitting). Related to dotnet/runtime#43540 and also dotnet/runtime#43543 (which has a similar problem for trimming).

[vitek-karas]

It would be better to try to come up with some realistically looking samples - it makes it much easier to read and understand. For example the method could be "GetFileNextToAssembly" or something like that - to make it easy to understand what the function might be about, and also something which is obviously not compatible with single-file at least directly.

[mateoatr]

I've updated the examples given so that it's more clear when would the attribute be used.

[vitek-karas]

I would maybe rewrite the text here to not talk about libraries directly, just code in general. As in "one method calls another method".
The libraries/apps distinction and experiences should be mentioned as well, but they're not directly related to the exact mechanics of how the attribute works.

[vitek-karas]

Again - something a bit more realistic...

[vitek-karas]

Not sure how common it will be to try to disable the warning in external NuGet - most developers will simply give up at that point (as it's next to impossible for them to tell if the NuGet has a real issue or not).
This will not be a problem for Roslyn analyzers (since there's no external code being analyzed in that case).
For linker we can use the ILLink.LinkAttributes.xml and inject the UnconditionalSuppressMessageAttribute into the target NuGet assembly - not easy or simple, but it will work.

[vitek-karas]

In Roslyn analyzer - none - no warning (as that analyzer won't be aware of the problem).
In linker (global analyzer) - a warning just like any other - there's no distinction between app/library code in that case.

[vitek-karas]

We need a better name probably - it should not talk about "single-file" but rather the capability... so something like Requires..... In which case I would image it would apply to any "bundle-like" technology, be it single-file, app packages and so on.

[marek-safar]

This does not work for other single-file setups. We need a general way to define which capability link to SDK/msbuild targets. For example, browser-wasm will have this one and others always set (and cannot be unset), are we going to hardcode wasm-browser RID to linker?

[vitek-karas]

We could change the SDK to set PublishSingleFile=true for browser-wasm. Or we will need a new property. There's an existing discussion on basically this topic here: dotnet/sdk#14562.

[marek-safar]

We could change the SDK to set PublishSingleFile=true for browser-wasm.

Wouldn't that also trigger real publish single-file work/target which we do differently for wasm?

[vitek-karas]

If it would - then I consider that to be a bug in the SDK. In short dotnet publish -r browser-wasm -p:PublishSingleFile=true should already either fail or "ignore" the single-file setting (since it already does that anyway). If that's not the case we should fix it. If we do go with PublishSingleFile as the setting to drive this, then the wasm SDK should "ignore" it (meaning it's set to true by default anyway).

[vitek-karas]

The sample above uses Assembly.CodeBase, but the sample warning talks about Assembly.Location.

[vitek-karas]

(I can't seem to respond to comments on code which is not part of the PR anymore)

I think we need to cover more than the direct call case. My question was about user experience when the call chain is complex. Is linker going to propagate the error message as you suggested through the call chain or are the developers supposed to copy the message or something else?

Trying to deduce callchains is tricky. Not sure we should try to do that. First we should implement the simple solution which will effectively force developers to annotate all callers. Note that it doesn't necessarily mean copying the error message. The error message should talk in terms of the API it's on - so it should very likely change as it "bubbles up".

For Roslyn analyzer that's all we need - it's in the developer's own code, so manual propagation seems reasonable if necessary - and for libraries we actually want it (we want the developers to manually annotate public APIs in the library - if nothing else to provide the best message possible in the context of the library).

For global analyzer we could add a general feature where warnings from NuGets/libraries are grouped and reported "as one". So basically something like "Library 'MyHelper.dll' contains code which is possibly not compatible with running the application as single-file" (and then have a way to get to the full list of detailed warnings). It's still not NuGet level - but probably good enough. Mapping assemblies back to NuGet packages is a bit tricky - might be possible if we pass enough info from SDK (SDK very likely knows this mapping).

I think without capabilities API developers will simply suppress the warning as there is no easy way to add special handling and we'll be where we are today.

I just want to make sure I understand: "Capabilities API" means some simple way to detect that the app is running as single-file, right? Something like RuntimeFeature.IsDynamicCodeCompiled?

[marek-safar]

I just want to make sure I understand: "Capabilities API" means some simple way to detect that the app is running as single-file, right? Something like RuntimeFeature.IsDynamicCodeCompiled?

Yep, something like https://github.com/dotnet/runtime/blob/master/docs/workflow/trimming/feature-switches.md. Presumably, we'll add a feature switch too to trim the code if we add feature-like property.

[vitek-karas]

In .NET 5 there was a discussion about adding a capability API for single-file - but it was decided that we don't want one as we want almost all code to work the same regardless. That's why one doesn't exist, and also why there's no feature-switch like mechanism around it.

Given the recent development and the issues we're seeing around the new single-file, I tend to lean toward introducing a capability API like this one (after all, we're doing it for all other similar cases in the runtime/framework).

[MichalStrehovsky]

Mapping assemblies back to NuGet packages is a bit tricky - might be possible if we pass enough info from SDK

To get actionable output it would also need to map back to the NuGet package that the user explicitly added (NuGet X could be in the closure because the user added NuGet Y to their app - giving a warning about X is the same level of confusion as giving a warning about a random assembly - the user has no idea what linker is talking about). IMHO assembly level is fine and we can improve if there's feedback.

Given the recent development and the issues we're seeing around the new single-file, I tend to lean toward introducing a capability API like this one

Maybe I haven't seen all the feedback on this, but I've seen quite some on the .NET Native side - what scenarios are we thinking about where a capability check would be useful?

The "We have some recommendations for fixing common scenarios" at https://docs.microsoft.com/en-us/dotnet/core/deploying/single-file#api-incompatibility is a pretty exhaustive list of issues I saw - all of them can be solved by using an alternative API that works the same for single/multifile (i.e. there's no need to keep a separate codepath for the different deployment mode). What scenarios (I assume outside of these) are we thinking about where a capability check would be useful?

[mateoatr]

The attribute on this proposal (named RequiresAssemblyFiles) was merged in the runtime repo in dotnet/runtime#46824. Anotations in dotnet/runtime were made in dotnet/runtime#56196, and a Roslyn analyzer was added for this same attribute in #1722.