Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Couple fixes for UseSystemResourceKeys #103463

Merged
merged 7 commits into from
Jun 17, 2024
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 7 additions & 1 deletion eng/ILLink.Substitutions.Resources.template
Original file line number Diff line number Diff line change
@@ -1,10 +1,16 @@
<linker>
<!-- System.Resources.UseSystemResourceKeys removes resource strings and instead uses the resource key as the exception message -->
<assembly fullname="{AssemblyName}" feature="System.Resources.UseSystemResourceKeys" featurevalue="true">
<!-- System.Resources.UseSystemResourceKeys removes resource strings and instead uses the resource key as the exception message -->
<resource name="{StringResourcesName}.resources" action="remove" />
<type fullname="System.SR">
<method signature="System.Boolean UsingResourceKeys()" body="stub" value="true" />
<method signature="System.Boolean GetUsingResourceKeysSwitchValue()" body="stub" value="true" />
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is #95948 as a proof that it works.

</type>
</assembly>
<assembly fullname="{AssemblyName}" feature="System.Resources.UseSystemResourceKeys" featurevalue="false" featuredefault="true">
<type fullname="System.SR">
<method signature="System.Boolean UsingResourceKeys()" body="stub" value="false" />
<method signature="System.Boolean GetUsingResourceKeysSwitchValue()" body="stub" value="false" />
</type>
</assembly>
</linker>
4 changes: 3 additions & 1 deletion eng/illink.targets
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@
<ILLinkRewritePDBs Condition="'$(ILLinkRewritePDBs)' == ''">true</ILLinkRewritePDBs>

<ILLinkResourcesSubstitutionIntermediatePath>$(IntermediateOutputPath)ILLink.Resources.Substitutions.xml</ILLinkResourcesSubstitutionIntermediatePath>
<GenerateResourcesSubstitutions Condition="'$(GenerateResourcesSubstitutions)' == '' and '$(StringResourcesPath)' != ''">true</GenerateResourcesSubstitutions>
<GenerateResourcesSubstitutions Condition="'$(GenerateResourcesSubstitutions)' == '' and '$(StringResourcesPath)' != '' and '$(OmitResources)' != 'true'">true</GenerateResourcesSubstitutions>
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OmitResources is an existing mechanism to prevent generating resx. It was a bug that we'd still generate a substitution file when this was set.

</PropertyGroup>

<ItemGroup>
Expand Down Expand Up @@ -222,6 +222,8 @@
<LinkerNoWarn>$(LinkerNoWarn);IL2008;IL2009;IL2121;IL2025;IL2035</LinkerNoWarn>
<ILLinkArgs>$(ILLinkArgs) --nowarn $(LinkerNoWarn)</ILLinkArgs>
<ILLinkArgs Condition="'$(ILLinkDisableIPConstProp)' == 'true'">$(ILLinkArgs) --disable-opt ipconstprop</ILLinkArgs>
<!-- Featuredefault for System.Resources.UseSystemResourceKeys would hardcode all libraries as not using resource keys, don't apply it -->
<ILLinkArgs Condition="'$(GenerateResourcesSubstitutions)' == 'true'">$(ILLinkArgs) --ignore-featuredefault System.Resources.UseSystemResourceKeys</ILLinkArgs>
</PropertyGroup>

<!-- Move the assembly into a subdirectory for ILLink -->
Expand Down
5 changes: 4 additions & 1 deletion src/libraries/Common/src/System/SR.cs
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,10 @@ namespace System
{
internal static partial class SR
{
private static readonly bool s_usingResourceKeys = AppContext.TryGetSwitch("System.Resources.UseSystemResourceKeys", out bool usingResourceKeys) ? usingResourceKeys : false;
private static readonly bool s_usingResourceKeys = GetUsingResourceKeysSwitchValue();

// This method is a target of ILLink substitution.
private static bool GetUsingResourceKeysSwitchValue() => AppContext.TryGetSwitch("System.Resources.UseSystemResourceKeys", out bool usingResourceKeys) ? usingResourceKeys : false;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this could be changed to use FeatureSwitchDefinition, and then instead of featuredefault, set the defaults in Microsoft.NET.ILLink.targets. Do you see any problems with that?

If possible I'd prefer to move away from the XML for feature switches. If we can do so here, I wonder if we still need #96539 at all.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also realized that featuredefault is potentially problematic because it could result in feature switches being substituted even without the corresponding AppContext setting. I'm starting to think we should avoid featuredefault entirely.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this could be changed to use FeatureSwitchDefinition, and then instead of featuredefault, set the defaults in Microsoft.NET.ILLink.targets. Do you see any problems with that?

Much simpler. Surprising nobody thought of this in the issue, we already had the facilities. FeatureSwitchDefinition didn't look necessary so I didn't add it - it wasn't clear to me what it would buy here.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FeatureSwitchDefinition could let us get rid of some XML, but isn't necessary.


// This method is used to decide if we need to append the exception message parameters to the message when calling SR.Format.
// by default it returns the value of System.Resources.UseSystemResourceKeys AppContext switch or false if not specified.
Expand Down
22 changes: 15 additions & 7 deletions src/libraries/Common/src/System/SR.vb
Original file line number Diff line number Diff line change
Expand Up @@ -11,15 +11,23 @@ Imports System.Resources
Namespace System

Friend NotInheritable Class SR
' This method is used to decide if we need to append the exception message parameters to the message when calling SR.Format.
' by default it returns false.
Private Shared ReadOnly s_usingResourceKeys As Boolean = GetUsingResourceKeysSwitchValue()

Private Shared Function GetUsingResourceKeysSwitchValue() As Boolean
Dim usingResourceKeys As Boolean
If (AppContext.TryGetSwitch("System.Resources.UseSystemResourceKeys", usingResourceKeys)) Then
Return usingResourceKeys
End If

Return False
End Function

' This method Is used to decide if we need to append the exception message parameters to the message when calling SR.Format.
' by default it returns the value of System.Resources.UseSystemResourceKeys AppContext switch Or false if Not specified.
' Native code generators can replace the value this returns based on user input at the time of native code generation.
' Marked as NoInlining because if this is used in an AoT compiled app that is not compiled into a single file, the user
' could compile each module with a different setting for this. We want to make sure there's a consistent behavior
' that doesn't depend on which native module this method got inlined into.
<Global.System.Runtime.CompilerServices.MethodImpl(Global.System.Runtime.CompilerServices.MethodImplOptions.NoInlining)>
' The trimming tools are also capable of replacing the value of this method when the application Is being trimmed.
Public Shared Function UsingResourceKeys() As Boolean
Return False
Return s_usingResourceKeys
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I synchronized SR.vb with SR.cs that we apparently neglected for years.

End Function

Friend Shared Function GetResourceString(ByVal resourceKey As String, Optional ByVal defaultString As String = Nothing) As String
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,8 @@
<!-- DesignTimeBuild requires all the TargetFramework Derived Properties to not be present in the first property group. -->
<PropertyGroup>
<TargetPlatformIdentifier>$([MSBuild]::GetTargetPlatformIdentifier('$(TargetFramework)'))</TargetPlatformIdentifier>
<GeneratePlatformNotSupportedAssemblyMessage Condition="'$(TargetPlatformIdentifier)' == ''">SR.DiagnosticsFileVersionInfo_PlatformNotSupported</GeneratePlatformNotSupportedAssemblyMessage>
<GeneratePlatformNotSupportedAssemblyMessage Condition="'$(TargetPlatformIdentifier)' == ''">SR.DiagnosticsFileVersionInfo_PlatformNotSupported</GeneratePlatformNotSupportedAssemblyMessage>
<OmitResources Condition="'$(GeneratePlatformNotSupportedAssemblyMessage)' == ''">true</OmitResources>
Copy link
Member Author

@MichalStrehovsky MichalStrehovsky Jun 14, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bugfix for a customer reported issue linked from top post that I ran into myself while working on this (not sure if it's needed in this PR or it could be separated out, but I sure don't want to rebuild libraries to see if I still get a warning).

The problem was that only the PlatformNotSupported flavor of the assembly needs resource strings, but we were generating the SR class/resources/substitution anyway. Customers were getting warnings for this (see the issue for the warning).

</PropertyGroup>

<ItemGroup Condition="'$(TargetPlatformIdentifier)' != ''">
Expand Down
8 changes: 8 additions & 0 deletions src/tools/illink/src/linker/Linker/Driver.cs
Original file line number Diff line number Diff line change
Expand Up @@ -470,6 +470,14 @@ protected int SetupContext (ILogger? customLogger = null)

continue;
}
case "--ignore-featuredefault": {
if (!GetStringParam (token, out string? featureName))
return -1;

context.SetIgnoredFeatureDefault (featureName);

continue;
}
case "--new-mvid":
//
// This is not same as --deterministic which calculates MVID
Expand Down
4 changes: 4 additions & 0 deletions src/tools/illink/src/linker/Linker/FeatureSettings.cs
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,10 @@ public static bool ShouldProcessElement (XPathNavigator nav, LinkContext context
return false;
}

if (bIsDefault && context.IgnoredFeatureDefaultSettings.Contains (feature)) {
bIsDefault = false;
}

if (!context.FeatureSettings.TryGetValue (feature, out bool featureSetting))
return bIsDefault;

Expand Down
9 changes: 9 additions & 0 deletions src/tools/illink/src/linker/Linker/LinkContext.cs
Original file line number Diff line number Diff line change
Expand Up @@ -136,6 +136,8 @@ public Pipeline Pipeline {

public Dictionary<string, bool> FeatureSettings { get; init; }

public HashSet<string> IgnoredFeatureDefaultSettings { get; init; }

public List<PInvokeInfo> PInvokes { get; private set; }

public string? PInvokesListFile;
Expand Down Expand Up @@ -218,6 +220,7 @@ protected LinkContext (Pipeline pipeline, ILogger logger, string outputDirectory
_isTrimmable = new Dictionary<AssemblyDefinition, bool> ();
OutputDirectory = outputDirectory;
FeatureSettings = new Dictionary<string, bool> (StringComparer.Ordinal);
IgnoredFeatureDefaultSettings = new HashSet<string> (StringComparer.Ordinal);

SymbolReaderProvider = new DefaultSymbolReaderProvider (false);

Expand Down Expand Up @@ -264,6 +267,12 @@ public void SetFeatureValue (string feature, bool value)
FeatureSettings[feature] = value;
}

public void SetIgnoredFeatureDefault (string feature)
{
Debug.Assert (!string.IsNullOrEmpty (feature));
IgnoredFeatureDefaultSettings.Add (feature);
}

public bool HasFeatureValue (string feature, bool value)
{
return FeatureSettings.TryGetValue (feature, out bool fvalue) && value == fvalue;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ namespace Mono.Linker.Tests.Cases.FeatureSettings
[SetupLinkerArgument ("--feature", "FieldCondition", "true")]
[SetupLinkerArgument ("--feature", "PropertyCondition", "false")]
[SetupLinkerArgument ("--feature", "EventCondition", "true")]
[SetupLinkerArgument ("--ignore-featuredefault", "IgnoredDefaultCondition")]
public class FeatureDescriptors
{
public static void Main ()
Expand All @@ -25,6 +26,9 @@ public static void Main ()
static bool DefaultConditionTrue;
static bool DefaultConditionFalse;

static bool IgnoredDefaultConditionTrue;
static bool IgnoredDefaultConditionFalse;

[Kept]
static bool GlobalConditionTrue;
static bool GlobalConditionFalse;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -47,4 +47,16 @@
<field name="DefaultConditionFalse" />
</type>
</assembly>
<!-- Check that a default feature condition can be ignored -->
<assembly fullname="test, Version=0.0.0.0, Culture=neutral, PublicKeyToken=null" feature="IgnoredDefaultCondition" featurevalue="true" featuredefault="true">
<type fullname="Mono.Linker.Tests.Cases.FeatureSettings.FeatureDescriptors">
<field name="IgnoredDefaultConditionTrue" />
</type>
</assembly>
<!-- Else case for the ignored default condition -->
<assembly fullname="test, Version=0.0.0.0, Culture=neutral, PublicKeyToken=null" feature="IgnoredDefaultCondition" featurevalue="false">
<type fullname="Mono.Linker.Tests.Cases.FeatureSettings.FeatureDescriptors">
<field name="IgnoredDefaultConditionFalse" />
</type>
</assembly>
</linker>
Loading