Skip to content

Commit

Permalink
Apply suggestions from code review
Browse files Browse the repository at this point in the history
Co-authored-by: Vitek Karas <10670590+vitek-karas@users.noreply.github.com>
  • Loading branch information
sbomer and vitek-karas authored Nov 22, 2023
1 parent e476f65 commit babf464
Showing 1 changed file with 3 additions and 3 deletions.
6 changes: 3 additions & 3 deletions accepted/2023/feature-switch-attributes.md
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@ if (RuntimeFeature.IsDynamicCodeSupported)
UseDynamicCode(); // OK, no warning
if (RuntimeFeature.IsDynamicCodeCompiled)
UseDynamicCode(); // OK, no warning in ILCompiler, but warns in analyzer
UseDynamicCode(); // OK, no warning in ILCompiler (for now this warns in the analyzer)
[RequiresDynamicCode("Uses dynamically generated code")]
static void UseDynamicCode() { }
Expand Down Expand Up @@ -76,15 +76,15 @@ The ILLink Roslyn analyzer has built-in support for treating `IsDynamicCodeSuppo

- Teach the ILLink Roslyn Analyzer about the substitution XML

We don't want to teach the analyzer to read the substitution XML. The analyzer is the first interaction that users typically have with trimming and AOT warnings. This should not be burdened by the XML format. Even if we did teach the analyzer about the XML, it would not solve the problem because the analyzer must not globally assume that `IsDynamicCodeSupported` is false as ILCompiler does.
We don't want to teach the analyzer to read the substitution XML. The analyzer is the first interaction that users typically have with trimming and AOT warnings. This should not be burdened by the XML format. Even if we did teach the analyzer about the XML, it would not solve the problem; for example, the analyzer must not globally assume that `IsDynamicCodeSupported` is false as ILCompiler does.

- Define a model with the full richness of the supported OS platform attributes

We will focus initially on a model where feature switches are booleans that return `true` if a feature is enabled. We aren't considering supporting version checks, or feature switches of the opposite polarity (where `true` means a feature is disabled/unsupported). We will consider what this might look like just enough to gain confidence that our model could be extended to support these cases in the future, but won't design this fully in the first iteration.

## Feature guard attribute

In order to treat a property as a guard for a feature that has a `Requires` attribute, there must be a semantic tie between the guard property and the attribute. ILLink and ILCompiler don't have this requirement because they run on apps, not libraries, so the desired warning behavior just falls out, thanks to branch elimination and the fact that `IsDynamicCodeSupported` is set to false from MSBuild.
In order to treat a property as a guard for a feature that has a `Requires` attribute, there must be a semantic tie between the guard property and the attribute. ILLink and ILCompiler don't have this requirement because they run on apps, not libraries, so the desired warning behavior just falls out, thanks to substitution XML, branch elimination and the fact that `IsDynamicCodeSupported` is set to false from MSBuild.

We could allow placing `FeatureGuardAttribute` on the property to indicate that it should act as a guard for a particular feature. The intention with any of these approaches is for the guard to prevent analyzer warnings:

Expand Down

0 comments on commit babf464

Please sign in to comment.