Skip to content
This repository has been archived by the owner on Aug 24, 2022. It is now read-only.

Commit

Permalink
Fixes to enum semantics:
Browse files Browse the repository at this point in the history
  Bitwise operators on flags enums now produce an enum instance.
  A | B will always produce the same enum instance (A, B) instead of producing a new one every time. This ensures that all forms of equality tests work.
  Casting a nullable enum to an integer will not convert the null into something else.
Fix an unhandled exception in EvaluatorPool.
  • Loading branch information
kg committed Jul 11, 2012
1 parent 83ee171 commit 3c5d7c3
Show file tree
Hide file tree
Showing 15 changed files with 145 additions and 75 deletions.
12 changes: 6 additions & 6 deletions JSIL/AST/JSExpressionTypes.cs
Expand Up @@ -1692,7 +1692,7 @@ protected JSAsExpression (JSExpression inner, TypeReference newType)


public static JSExpression New (JSExpression inner, TypeReference newType, TypeSystem typeSystem) { public static JSExpression New (JSExpression inner, TypeReference newType, TypeSystem typeSystem) {
return NewInternal( return NewInternal(
inner, newType, typeSystem, inner, newType, typeSystem, false,
() => new JSAsExpression(inner, newType) () => new JSAsExpression(inner, newType)
); );
} }
Expand All @@ -1707,9 +1707,9 @@ protected JSCastExpression (JSExpression inner, TypeReference newType)
NewType = newType; NewType = newType;
} }


public static JSExpression New (JSExpression inner, TypeReference newType, TypeSystem typeSystem) { public static JSExpression New (JSExpression inner, TypeReference newType, TypeSystem typeSystem, bool force = false) {
return NewInternal( return NewInternal(
inner, newType, typeSystem, inner, newType, typeSystem, force,
() => new JSCastExpression(inner, newType) () => new JSCastExpression(inner, newType)
); );
} }
Expand All @@ -1726,14 +1726,14 @@ protected JSCastExpression (JSExpression inner, TypeReference newType)
return false; return false;
} }


internal static JSExpression NewInternal (JSExpression inner, TypeReference newType, TypeSystem typeSystem, Func<JSExpression> make) { internal static JSExpression NewInternal (JSExpression inner, TypeReference newType, TypeSystem typeSystem, bool force, Func<JSExpression> make) {
int rankCurrent, rankNew; int rankCurrent, rankNew;


var currentType = inner.GetActualType(typeSystem); var currentType = inner.GetActualType(typeSystem);
var currentDerefed = TypeUtil.FullyDereferenceType(currentType, out rankCurrent); var currentDerefed = TypeUtil.FullyDereferenceType(currentType, out rankCurrent);
var newDerefed = TypeUtil.FullyDereferenceType(newType, out rankNew); var newDerefed = TypeUtil.FullyDereferenceType(newType, out rankNew);


if (TypeUtil.TypesAreEqual(currentDerefed, newDerefed, false)) if (TypeUtil.TypesAreEqual(currentDerefed, newDerefed, false) && !force)
return inner; return inner;


if ((rankCurrent == rankNew) && (rankCurrent > 0)) { if ((rankCurrent == rankNew) && (rankCurrent > 0)) {
Expand All @@ -1744,7 +1744,7 @@ protected JSCastExpression (JSExpression inner, TypeReference newType)
} }


var newResolved = newDerefed.Resolve(); var newResolved = newDerefed.Resolve();
if ((newResolved != null) && newResolved.IsInterface) { if (!force && (newResolved != null) && newResolved.IsInterface) {
var currentResolved = currentDerefed.Resolve(); var currentResolved = currentDerefed.Resolve();


if (currentResolved != null) { if (currentResolved != null) {
Expand Down
9 changes: 0 additions & 9 deletions JSIL/ILBlockTranslator.cs
Expand Up @@ -277,15 +277,6 @@ IEnumerable<ILVariable> allVariables
) )
return new JSUntranslatableExpression(node); return new JSUntranslatableExpression(node);


if (TypeUtil.IsEnum(node.Arguments[0].InferredType ?? node.Arguments[0].ExpectedType)
|| TypeUtil.IsEnum(node.Arguments[1].InferredType ?? node.Arguments[1].ExpectedType)
) {
if (op == JSOperator.Equal)
op = JSOperator.EqualLoose;
else if (op == JSOperator.NotEqual)
op = JSOperator.NotEqualLoose;
}

var resultType = node.InferredType ?? node.ExpectedType; var resultType = node.InferredType ?? node.ExpectedType;
var result = new JSBinaryOperatorExpression( var result = new JSBinaryOperatorExpression(
op, lhs, rhs, resultType op, lhs, rhs, resultType
Expand Down
10 changes: 10 additions & 0 deletions JSIL/SpecialIdentifiers.cs
Expand Up @@ -178,6 +178,16 @@ public class JSILIdentifier : JSIdentifier {
); );
} }


public JSInvocationExpression ValueOfNullable (JSExpression nullableExpression) {
var valueType = nullableExpression.GetActualType(TypeSystem);
valueType = TypeUtil.StripNullable(valueType);

return JSInvocationExpression.InvokeStatic(
Dot("ValueOfNullable", valueType),
new[] { nullableExpression }, true
);
}

public JSInvocationExpression CreateInstanceOfType (TypeReference type) { public JSInvocationExpression CreateInstanceOfType (TypeReference type) {
return JSInvocationExpression.InvokeStatic( return JSInvocationExpression.InvokeStatic(
Dot(new JSFakeMethod("CreateInstanceOfType", type, new[] { TypeSystem.Object }, MethodTypes)), Dot(new JSFakeMethod("CreateInstanceOfType", type, new[] { TypeSystem.Object }, MethodTypes)),
Expand Down
13 changes: 10 additions & 3 deletions JSIL/Transforms/ExpandCastExpressions.cs
Expand Up @@ -48,6 +48,7 @@ public class ExpandCastExpressions : JSAstVisitor {
IntroduceEnumCasts.IsEnumOrNullableEnum(currentType) IntroduceEnumCasts.IsEnumOrNullableEnum(currentType)
) { ) {
var enumInfo = TypeInfo.Get(currentType); var enumInfo = TypeInfo.Get(currentType);
var isNullable = TypeUtil.IsNullable(currentType);


if (targetType.MetadataType == MetadataType.Boolean) { if (targetType.MetadataType == MetadataType.Boolean) {
EnumMemberInfo enumMember; EnumMemberInfo enumMember;
Expand All @@ -68,9 +69,15 @@ public class ExpandCastExpressions : JSAstVisitor {
)); ));
} }
} else if (TypeUtil.IsNumeric(targetType)) { } else if (TypeUtil.IsNumeric(targetType)) {
newExpression = JSInvocationExpression.InvokeMethod( if (isNullable) {
JS.valueOf(targetType), ce.Expression, null, true newExpression = JSIL.ValueOfNullable(
); ce.Expression
);
} else {
newExpression = JSInvocationExpression.InvokeMethod(
JS.valueOf(targetType), ce.Expression, null, true
);
}
} else if (targetType.FullName == "System.Enum") { } else if (targetType.FullName == "System.Enum") {
newExpression = ce.Expression; newExpression = ce.Expression;
} else { } else {
Expand Down
43 changes: 32 additions & 11 deletions JSIL/Transforms/IntroduceEnumCasts.cs
Expand Up @@ -16,6 +16,7 @@ public class IntroduceEnumCasts : JSAstVisitor {
public readonly JSSpecialIdentifiers JS; public readonly JSSpecialIdentifiers JS;


private readonly HashSet<JSOperator> LogicalOperators; private readonly HashSet<JSOperator> LogicalOperators;
private readonly HashSet<JSOperator> BitwiseOperators;


public IntroduceEnumCasts (TypeSystem typeSystem, JSSpecialIdentifiers js, TypeInfoProvider typeInfo, MethodTypeFactory methodTypes) { public IntroduceEnumCasts (TypeSystem typeSystem, JSSpecialIdentifiers js, TypeInfoProvider typeInfo, MethodTypeFactory methodTypes) {
TypeSystem = typeSystem; TypeSystem = typeSystem;
Expand All @@ -28,6 +29,12 @@ public class IntroduceEnumCasts : JSAstVisitor {
JSOperator.LogicalOr, JSOperator.LogicalOr,
JSOperator.LogicalNot JSOperator.LogicalNot
}; };

BitwiseOperators = new HashSet<JSOperator>() {
JSOperator.BitwiseAnd,
JSOperator.BitwiseOr,
JSOperator.BitwiseXor
};
} }


public static bool IsEnumOrNullableEnum (TypeReference tr) { public static bool IsEnumOrNullableEnum (TypeReference tr) {
Expand Down Expand Up @@ -89,21 +96,35 @@ public class IntroduceEnumCasts : JSAstVisitor {
var resultType = boe.GetActualType(TypeSystem); var resultType = boe.GetActualType(TypeSystem);
var resultIsEnum = IsEnumOrNullableEnum(resultType); var resultIsEnum = IsEnumOrNullableEnum(resultType);


if ((leftIsEnum || rightIsEnum) && LogicalOperators.Contains(boe.Operator)) { var eitherIsEnum = leftIsEnum || rightIsEnum;
if (leftIsEnum) {
var cast = JSInvocationExpression.InvokeMethod(
JS.valueOf(TypeSystem.Int32), boe.Left, null, true
);


boe.ReplaceChild(boe.Left, cast); if (LogicalOperators.Contains(boe.Operator)) {
} if (eitherIsEnum) {
if (leftIsEnum) {
var cast = JSInvocationExpression.InvokeMethod(
JS.valueOf(TypeSystem.Int32), boe.Left, null, true
);


if (rightIsEnum) { boe.ReplaceChild(boe.Left, cast);
var cast = JSInvocationExpression.InvokeMethod( }
JS.valueOf(TypeSystem.Int32), boe.Right, null, true
if (rightIsEnum) {
var cast = JSInvocationExpression.InvokeMethod(
JS.valueOf(TypeSystem.Int32), boe.Right, null, true
);

boe.ReplaceChild(boe.Right, cast);
}
}
} else if (BitwiseOperators.Contains(boe.Operator)) {
var parentCast = ParentNode as JSCastExpression;
if (resultIsEnum && ((parentCast == null) || (parentCast.NewType != resultType))) {
var cast = JSCastExpression.New(
boe, resultType, TypeSystem, true
); );


boe.ReplaceChild(boe.Right, cast); ParentNode.ReplaceChild(boe, cast);
VisitReplacement(cast);
} }
} }


Expand Down
17 changes: 15 additions & 2 deletions JSIL/TypeUtil.cs
Expand Up @@ -97,11 +97,24 @@ public static class TypeUtil {
} }
} }


public static bool IsNullable (TypeReference type) {
int temp;
type = FullyDereferenceType(type, out temp);

var git = type as GenericInstanceType;
if ((git != null) && (git.Name == "Nullable`1"))
return true;

return false;
}

public static TypeReference StripNullable (TypeReference type) { public static TypeReference StripNullable (TypeReference type) {
int temp;
type = FullyDereferenceType(type, out temp);

var git = type as GenericInstanceType; var git = type as GenericInstanceType;
if ((git != null) && (git.Name == "Nullable`1")) { if ((git != null) && (git.Name == "Nullable`1"))
return git.GenericArguments[0]; return git.GenericArguments[0];
}


return type; return type;
} }
Expand Down
32 changes: 25 additions & 7 deletions Libraries/JSIL.Core.js
Expand Up @@ -3671,12 +3671,11 @@ JSIL.EnumValue.prototype.toString = function () {
var name = names[i]; var name = names[i];
var nameValue = enumType[name].value; var nameValue = enumType[name].value;


if (nameValue) { if (nameValue === this.value) {
result.push(name);
} else if (nameValue) {
if ((this.value & nameValue) === nameValue) if ((this.value & nameValue) === nameValue)
result.push(name); result.push(name);
} else {
if (!this.value)
result.push(name);
} }
} }


Expand Down Expand Up @@ -3786,9 +3785,21 @@ JSIL.MakeEnum = function (fullName, isPublic, members, isFlagsEnum) {
valueProto, "__ThisTypeId__", result.__TypeId__ valueProto, "__ThisTypeId__", result.__TypeId__
); );


// Because there's no way to change the behavior of ==,
// we need to ensure that all calls to $MakeValue for a given value
// return the same instance.
// FIXME: Memory leak! Weak references would help here, but TC39 apparently thinks
// hiding GC behavior from developers is more important than letting them control
// memory usage.
var valueCache = {};

result.$MakeValue = function (value, name) { result.$MakeValue = function (value, name) {
// TODO: Cache value instances to reduce garbage creation? var result = valueCache[value];
return new valueType(value, name);
if (!result)
result = valueCache[value] = new valueType(value, name);

return result;
}; };


return result; return result;
Expand Down Expand Up @@ -6437,4 +6448,11 @@ JSIL.ImplementExternals("System.Enum", function ($) {
return enumType[enumType.__ValueToName__[value]]; return enumType[enumType.__ValueToName__[value]];
} }
); );
}); });

JSIL.ValueOfNullable = function (value) {
if (value === null)
return value;
else
return value.valueOf();
};
2 changes: 2 additions & 0 deletions Libraries/JSIL.XNACore.js
Expand Up @@ -5058,6 +5058,8 @@ JSIL.ImplementExternals("Microsoft.Xna.Framework.Graphics.SpriteBatch", function
} }


if (effects) { if (effects) {
effects = effects.valueOf();

if (effects & this.spriteEffects.FlipHorizontally) { if (effects & this.spriteEffects.FlipHorizontally) {
if (!needRestore) if (!needRestore)
this.$save(); this.$save();
Expand Down
1 change: 1 addition & 0 deletions Tests/ComparisonTests.cs
Expand Up @@ -239,6 +239,7 @@ public class ComparisonTests : GenericTestFixture {
@"TestCases\EnumCheckType.cs", @"TestCases\EnumCheckType.cs",
@"TestCases\EnumNullableArithmetic.cs", @"TestCases\EnumNullableArithmetic.cs",
@"TestCases\EnumAnonymousMethod.cs", @"TestCases\EnumAnonymousMethod.cs",
@"TestCases\CompareFlagsEnums.cs",
}, MakeDefaultProvider(), new AssemblyCache() }, MakeDefaultProvider(), new AssemblyCache()
); );
} }
Expand Down
10 changes: 8 additions & 2 deletions Tests/EvaluatorPool.cs
Expand Up @@ -130,11 +130,17 @@ public class Evaluator : IDisposable {
Process = Process.Start(psi); Process = Process.Start(psi);


ThreadPool.QueueUserWorkItem((_) => { ThreadPool.QueueUserWorkItem((_) => {
_StdOut = Process.StandardOutput.ReadToEnd(); try {
_StdOut = Process.StandardOutput.ReadToEnd();
} catch {
}
stdoutSignal.Set(); stdoutSignal.Set();
}); });
ThreadPool.QueueUserWorkItem((_) => { ThreadPool.QueueUserWorkItem((_) => {
_StdErr = Process.StandardError.ReadToEnd(); try {
_StdErr = Process.StandardError.ReadToEnd();
} catch {
}
stderrSignal.Set(); stderrSignal.Set();
}); });


Expand Down
29 changes: 0 additions & 29 deletions Tests/FailingTestCases/FlagsEnumOperators.cs

This file was deleted.

2 changes: 1 addition & 1 deletion Tests/FormattingTests.cs
Expand Up @@ -326,7 +326,7 @@ public class FormattingTests : GenericTestFixture {
public void FlagsEnumsWithZeroValues () { public void FlagsEnumsWithZeroValues () {
var generatedJs = GetJavascript( var generatedJs = GetJavascript(
@"SpecialTestCases\FlagsEnumsWithZeroValues.cs", @"SpecialTestCases\FlagsEnumsWithZeroValues.cs",
"B A\r\nB 0" "B A\r\nB A"
); );
try { try {
Assert.IsFalse(generatedJs.Contains("| $asm01.Program.SimpleEnum.E")); Assert.IsFalse(generatedJs.Contains("| $asm01.Program.SimpleEnum.E"));
Expand Down
12 changes: 8 additions & 4 deletions Tests/SpecialTestCases/FlagsEnumsWithZeroValues.cs
Expand Up @@ -10,11 +10,15 @@ public enum SimpleEnum {
E = 0 E = 0
} }


public static SimpleEnum ReturnEnum (SimpleEnum value) {
return value;
}

public static void Main (string[] args) { public static void Main (string[] args) {
const SimpleEnum a = SimpleEnum.B; const SimpleEnum b = SimpleEnum.B;
SimpleEnum b = SimpleEnum.E; SimpleEnum e = SimpleEnum.E;


Console.WriteLine("{0} {1}", a, b); Console.WriteLine("{0} {1}", b, e);
Console.WriteLine("{0} {1}", a & SimpleEnum.B, b & SimpleEnum.D); Console.WriteLine("{0} {1}", b & ReturnEnum(SimpleEnum.B), e & ReturnEnum(SimpleEnum.D));
} }
} }
26 changes: 26 additions & 0 deletions Tests/TestCases/CompareFlagsEnums.cs
@@ -0,0 +1,26 @@
using System;

public static class Program {
[Flags]
public enum SimpleEnum {
A = 1,
B = 2,
C = 4
}

public static SimpleEnum Or (SimpleEnum lhs, SimpleEnum rhs) {
return lhs | rhs;
}

public static void Main (string[] args) {
var a = SimpleEnum.A;
var b = SimpleEnum.B;

var c = Or(a, b);
var d = Or(b, a);

Console.WriteLine("{0} {1} {2} {3}", a, b, c, d);
Console.WriteLine("{0}", c == a ? "true" : "false");
Console.WriteLine("{0}", c == d ? "true" : "false");
}
}
2 changes: 1 addition & 1 deletion Tests/Tests.csproj
Expand Up @@ -103,7 +103,6 @@
<None Include="TestCases\GenericStaticConstructorOrdering2.cs" /> <None Include="TestCases\GenericStaticConstructorOrdering2.cs" />
<None Include="TestCases\StaticInitializersInGenericTypesSettingStaticFields2.cs" /> <None Include="TestCases\StaticInitializersInGenericTypesSettingStaticFields2.cs" />
<None Include="TestCases\EnumCheckType.cs" /> <None Include="TestCases\EnumCheckType.cs" />
<None Include="FailingTestCases\FlagsEnumOperators.cs" />
<None Include="TestCases\NullableArithmetic.cs" /> <None Include="TestCases\NullableArithmetic.cs" />
<None Include="TestCases\NullableComparison.cs" /> <None Include="TestCases\NullableComparison.cs" />
<None Include="TestCases\NullableComparisonWithCast.cs" /> <None Include="TestCases\NullableComparisonWithCast.cs" />
Expand Down Expand Up @@ -186,6 +185,7 @@
<None Include="XMLTestCases\WriteStartDocument.cs" /> <None Include="XMLTestCases\WriteStartDocument.cs" />
<None Include="XMLTestCases\WriteEndDocument.cs" /> <None Include="XMLTestCases\WriteEndDocument.cs" />
<None Include="SimpleTestCases\IncrementBaseProperty.cs" /> <None Include="SimpleTestCases\IncrementBaseProperty.cs" />
<None Include="TestCases\CompareFlagsEnums.cs" />
<Compile Include="XMLTests.cs" /> <Compile Include="XMLTests.cs" />
<Compile Include="DependencyTests.cs" /> <Compile Include="DependencyTests.cs" />
<None Include="TestCases\GenericParameterNameShadowing.cs" /> <None Include="TestCases\GenericParameterNameShadowing.cs" />
Expand Down

0 comments on commit 3c5d7c3

Please sign in to comment.