Incorporate feedback from last review

Author: Immo Landwerth

accepted/2021/preview-features/preview-features.md

@@ -55,64 +55,13 @@ versions of .NET, just like they could in preview builds.
 
 ## Scenarios and User Experience
 
-### Enabling a preview runtime feature
-
-Maddy is building a Tye application. She reads in a blog post that the .NET
-runtime team is experimenting with a new GC mode, code named FancyGC.
-
-She wants to see how this improves the performance of her backend. She's glad to
-see that the new GC mode was shipped as part of .NET 6, so she doesn't need to
-figure out how to run a new version of .NET in her CI lab. So she creates a
-feature branch and enables the new GC mode simply by changing the service's
-project file as follows:
-
-```xml
-<Project Sdk="Microsoft.NET.Sdk">
-
-  <PropertyGroup>
-    <TargetFramework>net6.0</TargetFramework>
-    <EnablePreviewFeatures>True</EnablePreviewFeatures>
-  </PropertyGroup>
-
-</Project>
-```
-
-She then submits a draft PR against `main` to trigger their CI/CD pipeline which
-also runs some performance tests.
-
-### Enabling a preview language feature
-
-During lunch, Adriana discusses a design problem with one of the architects who
-mentions that the upcoming static interface members in C# might be able to solve
-this issue. Adriana does a quick web search and finds a blog post from the C#
-team that showcases how this feature might look like. She wonders how she can
-play with that. Optimistic, she copy & pastes the sample code into her project
-which immediately generates a compilation error:
-
-> The feature 'static interface members' is currently in Preview and
-> "unsupported". To use Preview features, use the 'preview' language version.
-
-The error displays a light-bulb in Visual Studio, suggesting to enable preview
-features, which modifies her project file as follows:
-
-```xml
-<Project Sdk="Microsoft.NET.Sdk">
-
-  <PropertyGroup>
-    <TargetFramework>net6.0</TargetFramework>
-    <EnablePreviewFeatures>True</EnablePreviewFeatures>
-  </PropertyGroup>
-
-</Project>
-```
-
-### Enabling a cross-cutting preview feature
+### Enabling a preview feature
 
 Ainsley is working for the insurance company Fabrikam. As part of their work,
 they are trying to figure out how they can integrate machine learning with
 ML.NET more tightly with their services, but they struggle with data ingestion
-because a lot of the heavy computations are happening inside of an old C++ base.
-When that code was written, .NET wasn't fast enough to do heavy numerical
+because a lot of the heavy computations are happening inside of an old C++ code
+base. When that code was written, .NET wasn't fast enough to do heavy numerical
 computations.
 
 As part of their research into this problem space, they notice a recent blog
@@ -157,9 +106,8 @@ the library into the production instance of Fabrikam.
 * We can deliver a reasonable experience even when a preview feature can't be
   supported in all the workloads, for example, by failing with a sensible error
   message.
-* We have a unified experience for enabling preview features, no matter whether
-  they are in a runtime, a compiler, a library, or span multiple parts of the
-  system.
+* We have a simple experience for enabling preview features that span multiple
+  parts of the system.
 * We don't have to do unnatural acts to expose a preview feature, by, for
   example, having to build a plug-in based system that allows swapping in and
   out files.
@@ -169,6 +117,8 @@ the library into the production instance of Fabrikam.
 * Allowing Microsoft to turn features on or off remotely, e.g. A/B testing
 * Designing a mechanism for shipping preview features as separate packages or
   installers
+* Modeling runtime-only preview features (see [What about runtime-only preview
+  features?](#what-about-runtime-only-preview-features))
 
 ## Stakeholders and Reviewers
 
@@ -227,27 +177,39 @@ This means you can look at a binary and tell whether it was built using preview
 features. This can be used for auditing and for detecting transitive
 dependencies ([discussed later](#api-analyzer)).
 
-### runtime.json
-
-Whether or not `EnablePreviewFeatures` was set to true for the application
-should be written into the `runtime.json` file such that the host/runtime can
-use it to make decisions. Alternatively, we could say this is encoded by the
-fact that the assembly the host considers the entry point has the
-`RequiresPreviewFeatures` assembly level attribute.
-
 ### Compilation context
 
 When `EnablePreviewFeatures` is true, the `LangVersion` property should be set
-to `Preview`. This avoids customers having to turn on preview features for the
-language separately.
+to `Preview` (unless the customer has explicitly set `LangVersion` in their
+project file already). This avoids customers having to turn on preview features
+for the language separately.
 
 In addition, the property `EnablePreviewFeatures` should be passed to the
 compilation context (akin to how `TargetFramework` is passed in today). An
 analyzer will use that to block use of APIs that are marked as preview as basing
-this off the language preview mode feels wrong.
+this off the language preview mode is nonsensical.
 
 ***Note:*** F# doesn't have this capability today.
 
+Since we use a separate property the user can have preview features off, but
+language is in preview mode. This would result in configuration where the user
+can use runtime features that are in preview.
+
+We have two options on how we can deal with this:
+
+1. One option is to use `RuntimeFeatures` and mark the field as
+   `[RequiresPreviewFeatures]` that causes the compiler to enforce the
+   containing assembly/type/member to be marked as `[RequiresPreviewFeatures]`
+   as well. Aleksey believes this check needs to be in-place regardless of
+   number of checks. Another option is to simply fail the build as an invalid
+   configuration.
+
+2. The other option is to simply fail the build with an error instructing the
+   customer that this configuration is invalid. This could be in MSBuild and
+   doesn't require work in the compiler.
+
+***OPEN QUESTION**: Do we want to use the first or the second mechanism?*
+
 ### API Analyzer
 
 We'll need an attribute to record whether or not the project was
@@ -290,6 +252,53 @@ capability to the compiler itself.
 
 ## Q & A
 
+### What about runtime-only preview features?
+
+We already have runtime-only preview features and the way we have shipped them
+is as environment variables, for example, [tiered JIT]:
+
+```text
+COMPlus_TieredCompilation=1
+```
+
+These can typically also be configured via `runtimeconfig.json`:
+
+```json
+{
+  "runtimeOptions": {
+    "configProperties": {
+      "System.Runtime.TieredCompilation": true
+    }
+  }
+}
+```
+
+We have decided that we scope this feature to only track the use of preview
+features that result in changes to the files distributed by the customer, such
+as using preview compiler features or preview APIs.
+
+Preview features that only impact the process, which is what runtime-only preview
+features are, don't need to be traced through the library ecosystem and thus
+aren't part of this work.
+
+Also, we don't think that it makes sense for libraries to declare a dependency
+on runtime-only preview features because those tend to be performance tweaks.
+
+The existing mechanism via environment variables and `runtimeconfig.json` is
+considered good enough. Also, for those features it is useful to be able to turn
+individual features on an off in order to troubleshoot. Restricting those
+features to the global preview mode is considered too restrictive an
+unnecessary, considering that they are application-level concerns.
+
+We discussed whether the environment variables or switches should include the
+name "preview" or "unsupported" but this would be akin to naming preview APIs
+differently which we generally don't want because it increases the friction for
+customers that want to move from the preview to the supported version. Also,
+historically we have not been able to rename them. We believe it sufficient to
+document which of these switches are supported and which ones are in preview.
+
+[tiered-jit]: https://devblogs.microsoft.com/dotnet/tiered-compilation-preview-in-net-core-2-1/
+
 ### Are preview features supported?
 
 No. The entire point of preview feature is to gather feedback, and make changes
@@ -419,74 +428,26 @@ Originally I used the term *experimental* but we decided to change it to
 * Overall, the goal is draw more people in and motivate them to try the
   features. The term *preview* seems more helpful than *experimental*.
 
-----------------------
-
-## Notes
-
-* Should we support multiple feature flags _as well_?
-    - If a feature is runtime only, we use environment variables today quite
-      successfully. Maybe we should only differentiate between features that
-      impact the binary on disk (compiler, APIs) vs. process lifetime. For
-      runtime features we often do perf work and we want to know whether it
-      helps or not.
-    - For runtime libraries, we generally don't believe a library can or should
-      declare a hard dependency on runtime only features, because they are often
-      perf tweaks.
-    - We don't believe we can change names of feature flags because historically
-      not been able to rename them.
-    - Maybe documenting them on docs.microsoft.com as unsupported is good
-      enough.
-    - We generally don't use `AppContext` switches for preview features, but we
-      use it for more than just compat switches, e.g. trimming.
-* Should we use `LangVersion` as the main switch?
-    - For language features, it's well established that people set this
-      property.
-    - The `LangVersion` property doesn't have quite the right name and it would
-      now cause transitivity requirements.
-    - Using this property might not work well for VB and F#
-    - The biggest counter argument is that the compiler can be in stable version
-      but the TFM you're targeting is not.
-    - If we use a separate property, then this means the user could have preview
-      features off, but language is in preview mode. This would result in
-      configuration where the user can use runtime features that are in preview.
-      One option is to use `RuntimeFeatures` and mark the field as
-      `[RequiresPreviewFeatures]` that causes the compiler to enforce the
-      containing assembly/type/member to be marked as
-      `[RequiresPreviewFeatures]` as well. Aleksey believes this check needs to
-      be in-place regardless of number of checks. Another option is to simply
-      fail the build as an invalid configuration.
-* Analyzer vs Compiler feature
-* VS releases
-* Multiple TFMs
-
-## From Jeremy
-
-For the compiler versioning (and other languages) vs "API impacted platform
-features" (or whatever you want to call them):
-
-* Maybe it's OK if C#-2025 can't compile certain syntaxes against .NET 6 with
-  experimental support .. e.g. we changed something in how generic math works
-  and they've decided "its been a couple releases, time to move on".
-* Conversely, even if the compiler supports that combination, if we changed
-  generic math then there may be a runtime break on assemblies compiled against
-  net6-preview vs net7, so an explicit experimental gesture is still a
-  requirement
-
-The three scenarios I commented on in chat may also be useful in addition to
-those points:
-
-* Preview language feature with experimental API: string8 str = 8"Hello, World";
-* Should generate the "I used experimental API, my callers need to accept that"
-  attribute.
-* Stable language syntax with experimental API
-    - `Utf8String str = new Utf8String("Hello, World");`
-* Should generate the attribute
-* Preview language features that would work on net5 or older: things like
-  autoprops or records or required partial members.
-* The attribute doesn't seem needed here.  Why should this necessarily dead-end
-  your NuGet package?
-
-(To be clear: I don't think we need to make anything be smart enough that
-attribute generation is tied to actual usage, if making the gesture emits it
-even when you don't need it that's OK (except when it isn't) -- I'm just
-pointing out they're two different axes)
+### Why didn't we use the `<LangVersion>Preview</LangVersion>`?
+
+There is an existing property that allows customers to opt into preview
+features of the language by setting this property in their project file:
+
+```xml
+<Project>
+
+  <PropertyGroup>
+    <LangVersion>Preview</LangVersion>
+  </PropertyGroup>
+
+</Project>
+```
+
+There are several issues with using this as the primary mechanism for enabling
+preview features for the .NET platform:
+
+* The `LangVersion` property doesn't have quite the right name and it would now
+  cause transitivity requirements.
+* Using this property might not work well for VB and F#
+* The biggest counter argument is that the compiler can be in stable version but
+  the TFM you're targeting is not.