Analyzers for publish only functionality

[vitek-karas]

Problem statement

For example for application trimming the goal is to make the experience predictable, as such the plan is to employ multiple tools to provide feedback to the user about the application's compatibility with trimming. The same applies to single-file publishing, AOT publishing and potentially more.

These tools will run in different stages of the SDK pipeline. Roslyn analyzers will be added to provide immediate feedback about the source code during code editing and in inner build loops. There’s already the ILLink tool which performs trimming but also analyses the app as a whole and provides feedback about the entire app. In the future there might be a "global analysis" tool somewhere in between these two.

All these tools need to be enabled only when it makes sense. If the user has no plans to ever trim the application, it would be a bad experience to show warnings about trimming.

Similarly for single-file there's an existing Roslyn analyzer and we plan to add single-file analysis into ILLink tool.

Currently the SDK defines properties which turn on the respective feature during publish. For example PublishTrimmed=true turns on trimming of the application, PublishSingleFile=true turns on publishing as single-file and so on. These properties only take effect during dotnet publish.

Ability to declare intent

To be able to keep all of the various parts of SDK in sync and provide consistent experience there needs to be a mechanism through which user declares an intent to use a publishing feature. For example a way to declare the intent to trim the application. Such mechanism must be in place across the entire SDK pipeline and not just during the publish phase.

The proposal is to repurpose the existing properties which turn on various publishing modes like PublishTrimmed or PublishSingleFile. Currently these properties only take effect during publish. The new behavior would be that these properties declare the intent to use their respective feature in the app.

Note: We've already started on this with the Roslyn single-file analyzer which is enabled only when PublishSingleFile=true. For it to work well, the property needs to be set in project file so that it's effective in builds, IDE and publish all the same. This proposal is to turn this into a general approach and design the end-to-end experience around it.

This would mean:

• Unlike today, it would be much more common to specify these properties in the applications project file itself.
• The values defined in the project file would become defaults for publishing, but they could be overwritten by specifying new values for a specific publish run. This is how it already works today, it's just not very common to specify these properties in the project file.
• SDK would still need to guarantee that the properties do not directly affect results of actions like build or run. For example setting PublishSingleFile=true does not make the SDK to produce a single-file executable in dotnet build. This also is already the case today.
• It is expected that some behavior will change even for build and run. For example PublishTrimmed=true would disable startup hooks in the application by default, for all configurations, including dotnet build. This is to make the experience consistent and in line with the goal that for example the action of trimming the application doesn't change its behavior. (As oppose to making the application compatible with trimming may change its behavior. Such a change can mean a direct code change or configuration change, like in this example disabling a feature switch.).
  See Default values for feature switches in trimmed console apps #14475 for a proposal on changing default feature switch values when trimming is enabled.
• Various analysis tools would rely on these properties to enable/disable specific analysis for a given feature (trimming, single file, …).
• Current VS UI has these properties in the publishing workflow only, they would need to be promoted to project-wide workflow, but also stay in the publishing workflow as overrides.
• There's a downside in which the properties are called "Publish…" but now they would have noticeable effect outside of publish phase.

An example of declaring the intent to trim the app:

    <PropertyGroup>
        <PublishTrimmed>true</PublishTrimmed>
    </PropertyGroup>

Alternative solutions

• Introduce a new property which would only declare the intent to eventually use trimming (or single-file, …), but leave the existing property to define the actual action of performing the trimming (or bundling to single-file, …). Such property would be harder to discover and less likely for users to use correctly.

[vitek-karas]

/cc @eerhardt @agocke @LakshanF @sbomer

[LakshanF]

Perhaps this should be discussed separately, but it is interesting to understand more the predictable impact on "build", "run" commands with these publish options. There is already some tension between the desire to not change the existing behavior of build&run and some impact (for example, default "feature switch" behavior differences with trim) that you mention above. It will be good to be crisper on the principles we need to use when we change the default build and run experiences.

There is also the larger issue of the development experience via build and run diverging significantly with the published experience. A compelling case could be made that these 2 should be closer.

[wli3]

Let me know if you need anything from SDK team

[vitek-karas]

Just FYI: I updated the issue description to include the fact that project settings for these properties should be conditional, to allow command-line overrides.

I do agree that we're walking a bit of a fine line between keeping the "with or without trimming works the same" and "good defaults" solutions.

Maybe I'll describe what I think is a good place to be in with a "sample":

dotnet new console
<write some code>

dotnet build
./bin/Debug/net5.0/app

dotnet publish -r win-x64 -o ./scd
./scd/app

dotnet publish -r win-x64 --self-contained false -o ./fdd
./fdd/app

dotnet publish -r win-x64 -p:PublishSingleFile=true -o ./sf
./sf/app

dotnet publish -r win-x64 -p:PublishTrimmed=true -o ./trim
./trim/app

The above will run the app 5 times. I would like to get to a place where all 5 times the app behaves as close to "the same" as possible. But at the same time, we can have good defaults for trimming, single-file, self-contained and so on, meaning ideally no warnings from linker, small app size, ...

Trimming is the most problematic right now (AOT will be even worse in the future), because some features of .NET are inherently incompatible with it (startup-hooks is a good example).

So to this end I think we can describe it as:

• Developers can describe all of the environments/configurations where the application should be able to run. This description is done in the project file as it is part of the definition of the app.
• With that description, building or publishing the app for the environments/configuration declared should produce an app which behaves consistently (ideally the same) everywhere.

There are other aspects to this, not just trimming:

• AOT in the future (or already in Xamarin)
• platform specific TFMs. For example I can specify net5.0-windows as the TargetFramework in which case the app is allowed to call windows specific APIs (and will get a new assembly which exposes these APIs). Similarly for net5.0-android or net5.0-ios and so on.

We should have (and in some cases already have) analyzers which validate that the app uses only the allowed APIs. For example the platform specific APIs go deeper into actual version of the OS it runs on and which APIs the OS exposes. The analyzer then validates that the code correctly "lights up" based on actual OS/version values at runtime.

Note that the TFMs are declared in the project file, and then at publish time I can ask to build the app for a specific RID and it has to be compatible with the pre-declared TFMs. The analyzers try to make sure that the app will behave consistently across all of these TFMs (in this case it doesn't mean "the same", it means it will not try to call API which doesn't exist).

If I "constrain" myself to just TFM=net5.0, then the app can't use any platform specific APIs. I declare this restriction in the project file and then all of "build" and "publish" respect that restriction.

Trimming can be treated very similarly. In order to make the app compatible with trimming, I need to restrict it to only the set of behaviors (APIs, ...) which are compatible with trimming. The proposal is to do that in the project file by setting PublishTrimmed=true. After that any "build" or "publish" should respect that and behave consistently (in case of trimming the consistent ~= the same). Feature switches are a good example how to achieve this - they try to make sure the behavior is the same trimming or not. Linker and all of its smarts is another way achieve it - linker tries hard to not change the behavior of the app. If it can't do that, it issues a warning (just like the platform specific API analyzer above warns if API which will not be available is called).

Sorry for the long description - we need to work on this some more to distill this down to a short message, but I think the general idea/intent is as I describe above.

Maybe the Publish* name is unfortunate, since the meaning is somewhat different in the new world - and maybe we should go through the pain to rename it, to better fit the new usage patterns. Or introduce a new property and accept the issues with discoverability.

[sbomer]

I think it should be ok to set <PublishTrimmed>true</PublishTrimmed> unconditionally. Command-line properties override the project file, and the publish profiles get imported after the project.

What would make a new property harder to discover? Is it just that the existing VS UI uses PublishTrimmed? Either way we'll need to steer people towards using the project file instead of publish profiles. FWIW I already see lots of people doing that in the wild.

If we come up with a better name for the property, I don't think renaming it would hurt too much - we could keep PublishTrimmed working for back compat, maybe with a warning that suggests updating to the new name.

[agocke]

Command-line properties override the project file

Huh, I actually thought it was the reverse. Good to know you can write MSBuild for years and still fundamentally misunderstand the evaluation model.

[eerhardt]

One more aspect to consider here is that some app models will actually "trim" code during Build, not only during Publish. One that immediately comes to mind is Xamarin Android. See https://github.com/xamarin/xamarin-android/blob/master/Documentation/guides/OneDotNet.md#linker-illink and the next section dotnet cli in that doc.

[vitek-karas]

That's a good point @eerhardt . I think the proposed behavior actually plays nicely with it. If we get to a place where actually trimming the app doesn't change any behavior of the app, then it kind of doesn't matter if or when the trimming happens. And it's up to each vertical scenario to choose if or when to trim.

[sbomer]

I think the main functional asks here have been implemented. The current behavior is:

• PublishTrimmed
  • declares intent in the project file for apps
  • doesn't cause issues at build time
  • sets feature switches
  • enables the roslyn analyzer
• IsTrimmable
  • declares intent for libraries
  • enables the analyzer
  • adds an assembly-level "IsTrimmable" attribute

There were discussions of having IsTrimmable behave differently for apps (set feature switches, don't inject the "IsTrimmable" attribute), and making PublishTrimmed imply IsTrimmable, but I don't think it's a great idea to introduce a subtle difference in what IsTrimmable means.

Various analysis tools would rely on these properties to enable/disable specific analysis for a given feature (trimming, single file, …).

We still need to decide how analyzers should treat feature switches (see some discussion in #17201), but if we are happy with the "PublishTrimmed"/"IsTrimmable" options that could be discussed when we get there.

Current VS UI has these properties in the publishing workflow only, they would need to be promoted to project-wide workflow, but also stay in the publishing workflow as overrides.

The "project-wide" workflow would just be to edit the csproj (and I think we should encourage it).

There's a downside in which the properties are called "Publish…" but now they would have noticeable effect outside of publish phase.

This is still the case. There's still a question whether the name "PublishTrimmed' should be changed to reflect that it doesn't only impact publish. It might be worthwhile especially considering that xamarin sets it during Release builds.

(Even if we introduced a separate property or reused IsTrimmable to declare intent for apps we'd have this issue: the common case will be to just set "PublishTrimmed", which would still have build-time effects.)

[agocke]

Agreed -- I think this is broadly done. We can make individual improvements as necessary.