Use FeatureSwitchDefinition in a few places #106564
Conversation
|
Note regarding the |
|
Note regarding the |
|
Tagging subscribers to this area: @dotnet/interop-contrib |
- IsReflectionExecutionAvaliable shouldn't have used FeatureSwitchDefinitionAttribute because it's not a property - Added missing FeatureSwitchDefinitionAttribute to MetadataUpdater.IsSupported
| [FeatureSwitchDefinition("System.Reflection")] | ||
| private static bool IsReflectionDisabled => false; |
There was a problem hiding this comment.
Doesn't this property reflect the negated value of that feature switch? Does this account for that somehow?
<type fullname="System.RuntimeType" feature="System.Reflection" featurevalue="false">
<method signature="System.Boolean get_IsReflectionDisabled()" body="stub" value="true" />
</type>There was a problem hiding this comment.
No, great catch, that one can't be handled by FeatureSwitchDefinition. I'm surprised this didn't cause test failures in ci. @MichalStrehovsky
There was a problem hiding this comment.
No, great catch, that one can't be handled by FeatureSwitchDefinition. I'm surprised this didn't cause test failures in ci. @MichalStrehovsky
We don't test things with reflection disabled since that mode is unsupported. So if this enabled reflection unconditionally, I'm not surprised it wasn't caught. I'd be surprised if it was the other way and nothing failed.
src/libraries/Microsoft.Extensions.DependencyInjection/src/ServiceProvider.cs
Show resolved
Hide resolved
…viceProvider.cs Co-authored-by: Jackson Schuster <36744439+jtschuster@users.noreply.github.com>
| @@ -24,6 +24,10 @@ | |||
| <Compile Include="$(CoreLibSharedDir)System\Runtime\Versioning\RequiresPreviewFeaturesAttribute.cs" /> | |||
| </ItemGroup> | |||
|
|
|||
| <ItemGroup Condition="'$(TargetFrameworkIdentifier)' != '.NETCoreApp' or $([MSBuild]::VersionLessThan('$(TargetFrameworkVersion)', '9.0'))"> | |||
There was a problem hiding this comment.
| <ItemGroup Condition="'$(TargetFrameworkIdentifier)' != '.NETCoreApp' or $([MSBuild]::VersionLessThan('$(TargetFrameworkVersion)', '9.0'))"> | |
| <ItemGroup Condition="!$([MSBuild]::IsTargetFrameworkCompatible('$(TargetFramework)', 'net9.0')"> |
?
There was a problem hiding this comment.
I took this condition from the matching section in src/System.Text.Json.csproj - I'd rather keep them the same in this change.
There was a problem hiding this comment.
Can we do the System.SR one as well (obviously in a separate PR)?
runtime/src/libraries/Common/src/System/SR.cs
Lines 10 to 13 in 269ea26
I'll look into that as a follow-up, thanks! |
|
/ba-g "timeout" |
This replaces much of our ILLink.Substitutions.xml with nearly equivalent FeatureSwitchDefinitionAttribute. The minor difference is that both true and false are substituted (when the feature switch is set accordingly), whereas some of our substitution xmls only defined a substitution for the false case. --------- Co-authored-by: Jackson Schuster <36744439+jtschuster@users.noreply.github.com>
This replaces much of our ILLink.Substitutions.xml with nearly equivalent FeatureSwitchDefinitionAttribute. The minor difference is that both true and false are substituted (when the feature switch is set accordingly), whereas some of our substitution xmls only defined a substitution for the false case.