Don't add values to enums that can't be parsed (#1836)

* Don't add values to enums that can't be parsed

* Fix test case

* Oops

Author: Saalvage

src/Generator/Library.cs

@@ -103,22 +103,29 @@ public static Enumeration GetEnumWithMatchingItem(this ASTContext context, strin
         /// as well as all previously discovered <see cref="Enumeration.Items"/>. The intent is to
         /// preserve sign information about the values held in <paramref name="enum"/>.
         /// </remarks>
-        public static Enumeration.Item GenerateEnumItemFromMacro(this Enumeration @enum,
-            MacroDefinition macro)
+        public static bool TryGenerateEnumItemFromMacro(this Enumeration @enum,
+            MacroDefinition macro, out Enumeration.Item item)
         {
-            var (value, type) = ParseEnumItemMacroExpression(macro, @enum);
+            if (!TryParseEnumItemMacroExpression(macro, @enum, out var result))
+            {
+                item = null;
+                return false;
+            }
+
+            var (value, type) = result;
             if (type > @enum.BuiltinType.Type)
             {
                 @enum.BuiltinType = new BuiltinType(type);
                 @enum.Type = @enum.BuiltinType;
             }
-            return new Enumeration.Item
+            item = new Enumeration.Item
             {
                 Name = macro.Name,
                 Expression = macro.Expression,
                 Value = value,
                 Namespace = @enum
             };
+            return true;
         }
 
         static object EvaluateEnumExpression(string expr, Enumeration @enum)
@@ -154,7 +161,7 @@ static object EvaluateEnumExpression(string expr, Enumeration @enum)
         /// evaluator biases towards returning <see cref="int"/> values: it doesn't attempt to
         /// discover that a value could be stored in a <see cref="byte"/>.
         /// </returns>
-        static (ulong value, PrimitiveType type) ParseEnumItemMacroExpression(MacroDefinition macro, Enumeration @enum)
+        static bool TryParseEnumItemMacroExpression(MacroDefinition macro, Enumeration @enum, out (ulong value, PrimitiveType type) item)
         {
             var expression = macro.Expression;
 
@@ -171,38 +178,40 @@ static object EvaluateEnumExpression(string expr, Enumeration @enum)
                 // Verify we have some sort of integer type. Enums can have negative values: record
                 // that fact so that we can set the underlying primitive type correctly in the enum
                 // item.
-                switch (System.Type.GetTypeCode(eval.GetType()))
+                item = System.Type.GetTypeCode(eval.GetType()) switch
                 {
                     // Must unbox eval which is typed as object to be able to cast it to a ulong.
-                    case TypeCode.SByte: return ((ulong)(sbyte)eval, PrimitiveType.SChar);
-                    case TypeCode.Int16: return ((ulong)(short)eval, PrimitiveType.Short);
-                    case TypeCode.Int32: return ((ulong)(int)eval, PrimitiveType.Int);
-                    case TypeCode.Int64: return ((ulong)(long)eval, PrimitiveType.LongLong);
+                    TypeCode.SByte => ((ulong)(sbyte)eval, PrimitiveType.SChar),
+                    TypeCode.Int16 => ((ulong)(short)eval, PrimitiveType.Short),
+                    TypeCode.Int32 => ((ulong)(int)eval, PrimitiveType.Int),
+                    TypeCode.Int64 => ((ulong)(long)eval, PrimitiveType.LongLong),
 
-                    case TypeCode.Byte: return ((byte)eval, PrimitiveType.UChar);
-                    case TypeCode.UInt16: return ((ushort)eval, PrimitiveType.UShort);
-                    case TypeCode.UInt32: return ((uint)eval, PrimitiveType.UInt);
-                    case TypeCode.UInt64: return ((ulong)eval, PrimitiveType.ULongLong);
+                    TypeCode.Byte => ((byte)eval, PrimitiveType.UChar),
+                    TypeCode.UInt16 => ((ushort)eval, PrimitiveType.UShort),
+                    TypeCode.UInt32 => ((uint)eval, PrimitiveType.UInt),
+                    TypeCode.UInt64 => ((ulong)eval, PrimitiveType.ULongLong),
 
-                    case TypeCode.Char: return ((char)eval, PrimitiveType.UShort);
+                    TypeCode.Char => ((char)eval, PrimitiveType.UShort),
                     // C++ allows booleans as enum values - they're translated to 1, 0.
-                    case TypeCode.Boolean: return ((bool)eval ? 1UL : 0UL, PrimitiveType.UChar);
+                    TypeCode.Boolean => ((bool)eval ? 1UL : 0UL, PrimitiveType.UChar),
 
-                    default: throw new Exception($"Expression {expression} is not a valid expression type for an enum");
-                }
+                    _ => throw new Exception($"Expression {expression} is not a valid expression type for an enum")
+                };
+
+                return true;
             }
             catch (Exception ex)
             {
-                // TODO: This should just throw, but we have a pre-existing behavior that expects malformed
-                // macro expressions to default to 0, see CSharp.h (MY_MACRO_TEST2_0), so do it for now.
+                // TODO: There should be a toggle to just throw here instead.
 
                 // Like other paths, we can however, write a diagnostic message to the console.
 
                 Diagnostics.PushIndent();
                 Diagnostics.Warning($"Unable to translate macro '{macro.Name}' to en enum value: {ex.Message}");
                 Diagnostics.PopIndent();
 
-                return (0, PrimitiveType.Int);
+                item = default;
+                return false;
             }
         }
 
@@ -311,16 +320,18 @@ TranslationUnit GetUnitFromItems(List<Enumeration.Item> list)
                 if (@enum.Items.Exists(it => it.Name == macro.Name))
                     continue;
 
-                var item = @enum.GenerateEnumItemFromMacro(macro);
-                @enum.AddItem(item);
-                macro.Enumeration = @enum;
+                if (@enum.TryGenerateEnumItemFromMacro(macro, out var item))
+                {
+                    @enum.AddItem(item);
+                    macro.Enumeration = @enum;
+                }
             }
 
             if (@enum.Items.Count <= 0)
                 return @enum;
 
             // The value of @enum.BuiltinType has been adjusted on the fly via
-            // GenerateEnumItemFromMacro. However, its notion of PrimitiveType corresponds with
+            // TryGenerateEnumItemFromMacro. However, its notion of PrimitiveType corresponds with
             // what the ExpressionEvaluator returns. In particular, the expression "1" will result
             // in PrimitiveType.Int from the expression evaluator. Narrow the type to account for
             // types narrower than int.

tests/dotnet/CSharp/CSharp.Tests.cs

@@ -1524,18 +1524,17 @@ public void TestMyMacroTestEnum()
     [Test]
     public void TestMyMacro2TestEnum()
     {
-        var a = (MyMacroTest2Enum)0;
-        var b = (MyMacroTest2Enum)1;
-        var c = (MyMacroTest2Enum)0x2;
-        var d = (MyMacroTest2Enum)(1 << 2);
-        var e = (MyMacroTest2Enum)(b | c);
-        var f = (MyMacroTest2Enum)(b | c | d);
-        var g = (MyMacroTest2Enum)(1 << 3);
-        var h = (MyMacroTest2Enum)((1 << 4) - 1);
-        Assert.IsTrue(a == MyMacroTest2Enum.MY_MACRO_TEST2_0 && b == MyMacroTest2Enum.MY_MACRO_TEST2_1 &&
-                    c == MyMacroTest2Enum.MY_MACRO_TEST2_2 && d == MyMacroTest2Enum.MY_MACRO_TEST2_3 &&
-                    e == MyMacroTest2Enum.MY_MACRO_TEST2_1_2 && f == MyMacroTest2Enum.MY_MACRO_TEST2_1_2_3 &&
-                    g == MyMacroTest2Enum.MY_MACRO_TEST2_4 && h == MyMacroTest2Enum.MY_MACRO_TEST2ALL);
+        var a = (MyMacroTest2Enum)1;
+        var b = (MyMacroTest2Enum)0x2;
+        var c = (MyMacroTest2Enum)(1 << 2);
+        var d = (MyMacroTest2Enum)(a | b);
+        var e = (MyMacroTest2Enum)(a | b | c);
+        var f = (MyMacroTest2Enum)(1 << 3);
+        var g = (MyMacroTest2Enum)((1 << 4) - 1);
+        Assert.IsTrue(a == MyMacroTest2Enum.MY_MACRO_TEST2_1 && b == MyMacroTest2Enum.MY_MACRO_TEST2_2 &&
+                    c == MyMacroTest2Enum.MY_MACRO_TEST2_3 && d == MyMacroTest2Enum.MY_MACRO_TEST2_1_2 &&
+                    e == MyMacroTest2Enum.MY_MACRO_TEST2_1_2_3 && f == MyMacroTest2Enum.MY_MACRO_TEST2_4 &&
+                    g == MyMacroTest2Enum.MY_MACRO_TEST2ALL);
     }
 
     [Test]