Support featuredefault="true"

To indicate that a substitution or descriptor should be applied in the
absence of a feature setting. Note that this is restricted to still
require the featurevalue attribute, to prevent the definition of
default substitutions that are never applied for any feature settings.

Also address PR feedback:
- Move the feature checks into a separate class
- #ifdef an implementation instead of callsites

docs/error-codes.md

@@ -9,11 +9,11 @@ error and warning codes.
 
 #### `IL1001`: Failed to process 'XML document location'. Feature 'feature' does not specify a "featurevalue" attribute
 
-- The substitution in 'XML document location' with feature value 'feature' does not use the `featurevalue` attribute. These attributes have to be used together.
+- The substitution or descriptor in 'XML document location' with feature value 'feature' does not use the `featurevalue` attribute. These attributes have to be used together.
 
 #### `IL1002`: Failed to process 'XML document location'. Unsupported non-boolean feature definition 'feature'
 
-- The substitution in 'XML document location' with feature value 'feature' sets the attribute `featurevalue` to a non-boolean value. Only boolean values are supported for this attribute.
+- The substitution or descriptor in 'XML document location' with feature value 'feature' sets the attribute `featurevalue` to a non-boolean value. Only boolean values are supported for this attribute.
 
 #### `IL1003`: Error processing 'XML document name': 'XmlException'
 
@@ -55,6 +55,10 @@ error and warning codes.
 
 - There was an error processing 'XML document location' xml file. The most likely reason for this is that the descriptor file has syntactical errors.
 
+#### `IL1014`: Failed to process 'XML document location`. Unsupported value for featuredefault attribute
+
+- The substitution or descriptor in 'XML document location' contains a 'featuredefault' attribute with an invalid value. This attribute only supports the true value, to indicate that this is the default behavior for a feature when a value is not given.
+
 ----
 ## Warning Codes
 

src/linker/Linker.Steps/BodySubstituterStep.cs

@@ -42,7 +42,7 @@ protected override void Process ()
 			ReadSubstitutions (_document);
 		}
 
-		bool ShouldProcessSubstitutions (XPathNavigator nav) => ResolveFromXmlStep.ShouldProcessElement (nav, Context, _xmlDocumentLocation);
+		bool ShouldProcessSubstitutions (XPathNavigator nav) => FeatureSettings.ShouldProcessElement (nav, Context, _xmlDocumentLocation);
 
 		void ReadSubstitutions (XPathDocument document)
 		{

src/linker/Linker.Steps/LinkAttributesStep.cs

@@ -600,6 +600,6 @@ static PropertyDefinition GetProperty (TypeDefinition type, string signature)
 			return null;
 		}
 
-		bool ShouldProcessElement (XPathNavigator nav) => ResolveFromXmlStep.ShouldProcessElement (nav, Context, _xmlDocumentLocation);
+		bool ShouldProcessElement (XPathNavigator nav) => FeatureSettings.ShouldProcessElement (nav, Context, _xmlDocumentLocation);
 	}
 }

src/linker/Linker.Steps/ResolveFromXmlStep.cs

@@ -72,7 +72,12 @@ public ResolveFromXmlStep (XPathDocument document, EmbeddedResource resource, As
 			_resourceAssembly = resourceAssembly ?? throw new ArgumentNullException (nameof (resourceAssembly));
 		}
 
-		bool ShouldProcessElement (XPathNavigator nav) => ShouldProcessElement (nav, Context, _xmlDocumentLocation);
+		bool ShouldProcessElement (XPathNavigator nav) => 
+#if FEATURE_ILLINK
+			FeatureSettings.ShouldProcessElement (nav, Context, _xmlDocumentLocation);
+#else
+			true;
+#endif
 
 		protected override void Process ()
 		{
@@ -752,28 +757,5 @@ public override string ToString ()
 		{
 			return "ResolveFromXmlStep: " + _xmlDocumentLocation;
 		}
-
-		public static bool ShouldProcessElement (XPathNavigator nav, LinkContext context, string documentLocation)
-		{
-			var feature = GetAttribute (nav, "feature");
-			if (string.IsNullOrEmpty (feature))
-				return true;
-
-			var value = GetAttribute (nav, "featurevalue");
-			if (string.IsNullOrEmpty (value)) {
-				context.LogError ($"Failed to process '{documentLocation}'. Feature {feature} does not specify a 'featurevalue' attribute", 1001);
-				return false;
-			}
-
-			if (!bool.TryParse (value, out bool bValue)) {
-				context.LogError ($"Failed to process '{documentLocation}'. Unsupported non-boolean feature definition {feature}", 1002);
-				return false;
-			}
-
-			if (context.FeatureSettings == null || !context.FeatureSettings.TryGetValue (feature, out bool featureSetting))
-				return false;
-
-			return bValue == featureSetting;
-		}
 	}
 }

src/linker/Linker/FeatureSettings.cs

@@ -0,0 +1,47 @@
+// Licensed to the .NET Foundation under one or more agreements.
+// The .NET Foundation licenses this file to you under the MIT license.
+// See the LICENSE file in the project root for more information.
+
+using System;
+using System.Xml.XPath;
+
+namespace Mono.Linker
+{
+	public static class FeatureSettings
+	{
+		public static bool ShouldProcessElement (XPathNavigator nav, LinkContext context, string documentLocation)
+		{
+			var feature = GetAttribute (nav, "feature");
+			if (string.IsNullOrEmpty (feature))
+				return true;
+
+			var value = GetAttribute (nav, "featurevalue");
+			if (string.IsNullOrEmpty (value)) {
+				context.LogError ($"Failed to process '{documentLocation}'. Feature '{feature}' does not specify a 'featurevalue' attribute", 1001);
+				return false;
+			}
+
+			if (!bool.TryParse (value, out bool bValue)) {
+				context.LogError ($"Failed to process '{documentLocation}'. Unsupported non-boolean feature definition '{feature}'", 1002);
+				return false;
+			}
+
+			var isDefault = GetAttribute (nav, "featuredefault");
+			bool bIsDefault = false;
+			if (!string.IsNullOrEmpty (isDefault) && (!bool.TryParse (isDefault, out bIsDefault) || !bIsDefault)) {
+				context.LogError ($"Failed to process '{documentLocation}'. Unsupported value for featuredefault attribute", 1014);
+				return false;
+			}
+
+			if (context.FeatureSettings == null || !context.FeatureSettings.TryGetValue (feature, out bool featureSetting))
+				return bIsDefault;
+
+			return bValue == featureSetting;
+		}
+
+		public static string GetAttribute (XPathNavigator nav, string attribute)
+		{
+			return nav.GetAttribute (attribute, String.Empty);
+		}
+	}
+}

test/Mono.Linker.Tests.Cases/FeatureSettings/FeatureDescriptors.cs

@@ -4,6 +4,9 @@
 
 namespace Mono.Linker.Tests.Cases.FeatureSettings
 {
+#if !NETCOREAPP
+	[IgnoreTestCase ("Feature settings in descriptors are not supported on Mono.")]
+#endif
 	[SetupLinkerDescriptorFile ("FeatureDescriptorsGlobalTrue.xml")]
 	[SetupLinkerDescriptorFile ("FeatureDescriptorsGlobalFalse.xml")]
 	[SetupLinkerDescriptorFile ("FeatureDescriptors.xml")]
@@ -20,6 +23,10 @@ public static void Main ()
 		{
 		}
 
+		[Kept]
+		static bool DefaultConditionTrue;
+		static bool DefaultConditionFalse;
+
 		[Kept]
 		static bool GlobalConditionTrue;
 		static bool GlobalConditionFalse;

test/Mono.Linker.Tests.Cases/FeatureSettings/FeatureDescriptors.xml

@@ -35,4 +35,16 @@
       <field name="AssemblyConditionTrue" />
     </type>
   </assembly>
+  <!-- Check that a feature condition can be used by default -->
+  <assembly fullname="test, Version=0.0.0.0, Culture=neutral, PublicKeyToken=null" feature="DefaultCondition" featurevalue="true" featuredefault="true">
+    <type fullname="Mono.Linker.Tests.Cases.FeatureSettings.FeatureDescriptors">
+      <field name="DefaultConditionTrue" />
+    </type>
+  </assembly>
+  <!-- Else case for the default condition -->
+  <assembly fullname="test, Version=0.0.0.0, Culture=neutral, PublicKeyToken=null" feature="DefaultCondition" featurevalue="false">
+    <type fullname="Mono.Linker.Tests.Cases.FeatureSettings.FeatureDescriptors">
+      <field name="DefaultConditionFalse" />
+    </type>
+  </assembly>
 </linker>

test/Mono.Linker.Tests.Cases/FeatureSettings/FeatureSubstitutions.cs

@@ -1,3 +1,4 @@
+using System;
 using Mono.Linker.Tests.Cases.Expectations.Assertions;
 using Mono.Linker.Tests.Cases.Expectations.Metadata;
 
@@ -20,6 +21,7 @@ static bool IsOptionalFeatureEnabled {
 		public static void Main ()
 		{
 			TestOptionalFeature ();
+			_ = IsDefaultFeatureEnabled;
 		}
 
 		[Kept]
@@ -47,5 +49,15 @@ static void UseOptionalFeature ()
 		static void UseFallback ()
 		{
 		}
+
+		[Kept]
+		static bool IsDefaultFeatureEnabled {
+			[Kept]
+			[ExpectedInstructionSequence (new[] {
+				"ldc.i4.1",
+				"ret",
+			})]
+			get => throw new NotImplementedException ();
+		}
 	}
 }

test/Mono.Linker.Tests.Cases/FeatureSettings/FeatureSubstitutions.xml

@@ -1,8 +1,19 @@
 <linker feature="OptionalFeature" featurevalue="false">
   <assembly fullname="test, Version=0.0.0.0, Culture=neutral, PublicKeyToken=null">
     <type fullname="Mono.Linker.Tests.Cases.FeatureSettings.FeatureSubstitutions">
-      <method signature="System.Boolean get_IsOptionalFeatureEnabled()" body="stub" value="false">
-      </method>
+      <method signature="System.Boolean get_IsOptionalFeatureEnabled()" body="stub" value="false" />
+    </type>
+  </assembly>
+  <!-- Check that a feature condition can be used by default -->
+  <assembly fullname="test, Version=0.0.0.0, Culture=neutral, PublicKeyToken=null" feature="DefaultFeature" featurevalue="true" featuredefault="true">
+    <type fullname="Mono.Linker.Tests.Cases.FeatureSettings.FeatureSubstitutions">
+      <method signature ="System.Boolean get_IsDefaultFeatureEnabled()" body="stub" value="true" />
+    </type>
+  </assembly>
+  <!-- Else case for the default condition -->
+  <assembly fullname="test, Version=0.0.0.0, Culture=neutral, PublicKeyToken=null" feature="DefaultFeature" featurevalue="false">
+    <type fullname="Mono.Linker.Tests.Cases.FeatureSettings.FeatureSubstitutions">
+      <method signature ="System.Boolean get_IsDefaultFeatureEnabled()" body="stub" value="false" />
     </type>
   </assembly>
 </linker>

test/Mono.Linker.Tests.Cases/FeatureSettings/FeatureSubstitutionsInvalid.cs

@@ -3,10 +3,11 @@
 
 namespace Mono.Linker.Tests.Cases.FeatureSettings
 {
-	[NoLinkedOutput]
 	[SetupLinkerSubstitutionFile ("FeatureSubstitutionsInvalid.xml")]
 	[SetupLinkerArgument ("--feature", "NoValueFeature", "true")]
-	[LogContains ("Feature NoValueFeature does not specify a 'featurevalue' attribute")]
+	[LogContains ("FeatureSubstitutionsInvalid.xml'. Feature 'NoValueFeature' does not specify a 'featurevalue' attribute")]
+	[LogContains ("FeatureSubstitutionsInvalid.xml'. Unsupported non-boolean feature definition 'NonBooleanFeature'")]
+	[LogContains ("FeatureSubstitutionsInvalid.xml'. Unsupported value for featuredefault attribute")]
 	[LogContains ("warning IL2012: Could not find field 'NonExistentField' in type 'Mono.Linker.Tests.Cases.FeatureSettings.FeatureSubstitutionsInvalid/Foo'")]
 	[LogContains ("warning IL2009: Could not find method 'NonExistentMethod' in type 'Mono.Linker.Tests.Cases.FeatureSettings.FeatureSubstitutionsInvalid/Foo'")]
 	public class FeatureSubstitutionsInvalid
@@ -15,25 +16,30 @@ public static void Main ()
 		{
 			NoValueFeatureMethod ();
 			NonBooleanFeatureMethod ();
-			BooleanFeatureMethod ();
+			InvalidDefaultFeatureMethod ();
 		}
 
 		[Kept]
-		static void NoValueFeatureMethod ()
-		{
-		}
+		[ExpectedInstructionSequence (new[] {
+			"ldnull",
+			"throw"
+		})]
+		static void NoValueFeatureMethod () => throw null;
 
 		[Kept]
-		static void NonBooleanFeatureMethod ()
-		{
-		}
+		[ExpectedInstructionSequence (new[] {
+			"ldnull",
+			"throw"
+		})]
+		static void NonBooleanFeatureMethod () => throw null;
 
 		[Kept]
-		static void BooleanFeatureMethod ()
-		{
-		}
+		[ExpectedInstructionSequence (new[] {
+			"ldnull",
+			"throw"
+		})]
+		static bool InvalidDefaultFeatureMethod () => throw null;
 
-		[Kept]
 		class Foo
 		{
 			int _field;

test/Mono.Linker.Tests.Cases/FeatureSettings/FeatureSubstitutionsInvalid.xml

@@ -1,13 +1,13 @@
 <linker>
-    <assembly fullname="test, Version=0.0.0.0, Culture=neutral, PublicKeyToken=null">
-      <type fullname="Mono.Linker.Tests.Cases.FeatureSettings.FeatureSubstitutionsInvalid">
-        <method signature="System.Boolean NoValueFeatureMethod()" body="stub" value="true" feature="NoValueFeature" />
-        <method signature="System.Boolean NonBooleanFeatureMethod()" body="stub" value="false" feature="NonBooleanFeature" featurevalue="nonboolean" />
-        <method signature="System.Boolean BooleanFeatureMethod()" body="stub" value="false" feature="BooleanFeature" featurevalue="true" />
-      </type>
-      <type fullname="Mono.Linker.Tests.Cases.FeatureSettings.FeatureSubstitutionsInvalid/Foo">
-        <field name="NonExistentField" />
-        <method signature="NonExistentMethod" />
-      </type>
-    </assembly>
+  <assembly fullname="test, Version=0.0.0.0, Culture=neutral, PublicKeyToken=null">
+    <type fullname="Mono.Linker.Tests.Cases.FeatureSettings.FeatureSubstitutionsInvalid">
+      <method signature="System.Boolean NoValueFeatureMethod()" body="stub" value="true" feature="NoValueFeature" />
+      <method signature="System.Boolean NonBooleanFeatureMethod()" body="stub" value="false" feature="NonBooleanFeature" featurevalue="nonboolean" />
+      <method signature="System.Boolean InvalidDefaultFeatureMethod()" body="stub" value="true" feature="InvalidDefaultFeatureMethod" featurevalue="true" featuredefault="false" />
+    </type>
+    <type fullname="Mono.Linker.Tests.Cases.FeatureSettings.FeatureSubstitutionsInvalid/Foo">
+      <field name="NonExistentField" />
+      <method signature="NonExistentMethod" />
+    </type>
+  </assembly>
 </linker>