New Mojom parser: Don't allow enum variables to be assigned int values.
Thus far the new Mojom parser has allowed variables of enum type to be assigned integer values. This used to be allowed in .mojom files but that was changed a few months back and the new parser never picked up that change. This patch corrects that.
- Because IsAssignmentCompatible() now returns false in the case of assigning an int to an enum, it is necessary to not use IsAssignmentCompatible() in the case that a user-defined constant that resolves to an int is being used as an enum value initializer (because that is allowed). That explains the reorganization in UserValueRef.validateAfterResolution().
- We changed how assignments of literals to user-defined-types is handled. Previously we did some validation during parsing (for example we would check that the literal was not of type float) but left other validation for the later validation phase. We now due all validation during the later validation phase.
BUG=#46
R=azani@chromium.org
Review URL: https://codereview.chromium.org/1514173002 .
diff --git a/mojom/mojom_parser/mojom/types.go b/mojom/mojom_parser/mojom/types.go
index 89b3f29..7df3701 100644
--- a/mojom/mojom_parser/mojom/types.go
+++ b/mojom/mojom_parser/mojom/types.go
@@ -780,15 +780,8 @@
}
func (t *UserTypeRef) MarkTypeCompatible(assignment LiteralAssignment) bool {
- switch assignment.assignedValue.LiteralValueType() {
- case SimpleTypeDouble, SimpleTypeFloat, SimpleTypeBool:
- return false
- case StringLiteralType:
- if !assignment.assignedValue.IsDefault() {
- return false
- }
- }
-
+ // Just mark the assignment attempt and return true. We will validate it
+ // during the validation phase.
t.literalAssignment = &assignment
return true
}
@@ -954,12 +947,26 @@
return fmt.Errorf(message)
case *EnumValue:
// An EnumValue is being used as an EnumValue initializer.
- // Below we will check that the two EnumTypes match.
+ // We will only check that the two EnumTypes match.
// In ComputeEnumValueIntegers() in computed_data.go we will further validate.
+ if !v.assigneeSpec.Type.IsAssignmentCompatible(v.resolvedConcreteValue) {
+ var message string
+ switch v.resolvedDeclaredValue.(type) {
+ case *EnumValue:
+ // An enum value is being used directly as an initializer.
+ message = fmt.Sprintf("Illegal assignment: The enum value %s of type %s may not be used as an initializer for %s of type %s.",
+ v.identifier, concreteValue.enumType.fullyQualifiedName, v.assigneeSpec.Name, v.assigneeSpec.Type.TypeName())
+ default:
+ // A user-defined constant whose value is an enum value is being used as an initializer.
+ message = fmt.Sprintf("Illegal assignment: %s with the value %v may not be used as an initializer for %s of type %s.",
+ v.identifier, concreteValue.fullyQualifiedName, v.assigneeSpec.Name, v.assigneeSpec.Type.TypeName())
+ }
+ return fmt.Errorf(UserErrorMessage(v.scope.file, v.token, message))
+ }
default:
panic(fmt.Sprintf("Unexpected type %T", concreteValue))
}
-
+ return nil
}
if !v.assigneeSpec.Type.IsAssignmentCompatible(v.resolvedConcreteValue) {
@@ -986,17 +993,13 @@
switch v.resolvedDeclaredValue.(type) {
case *EnumValue:
// An enum value is being assigned directly to a variable.
- message = "Illegal assignment: The enum value %s of type %s may not be assigned to %s of type %s."
- if v.usedAsEnumValueInitializer {
- // An enum value is being used directly as an initializer.
- message = "Illegal assignment: The enum value %s of type %s may not be used as an initializer for %s of type %s."
- }
- message = fmt.Sprintf(message, v.identifier, concreteValue.enumType.fullyQualifiedName, v.assigneeSpec.Name, assigneeType.TypeName())
+ message = fmt.Sprintf("Illegal assignment: The enum value %s of type %s may not be assigned to %s of type %s.",
+ v.identifier, concreteValue.enumType.fullyQualifiedName, v.assigneeSpec.Name, assigneeType.TypeName())
default:
// A user-defined constant whose value is an enum value is being assigned to a variable.
message = "Illegal assignment: %s with the value %v may not be assigned to %s of type %s."
if v.usedAsEnumValueInitializer {
- // A user-defined constant whose value is an enum valu is being used as an initializer.
+ // A user-defined constant whose value is an enum value is being used as an initializer.
message = "Illegal assignment: %s with the value %v may not be used as an initializer for %s of type %s."
}
message = fmt.Sprintf(message, v.identifier, concreteValue.fullyQualifiedName, v.assigneeSpec.Name, assigneeType.TypeName())
diff --git a/mojom/mojom_parser/mojom/types_test.go b/mojom/mojom_parser/mojom/types_test.go
index 8853be5..92e4eb3 100644
--- a/mojom/mojom_parser/mojom/types_test.go
+++ b/mojom/mojom_parser/mojom/types_test.go
@@ -171,7 +171,8 @@
{NewArrayTypeRef(SimpleTypeInt32, 0, false), []bool{false, false, false, false, false, false, false, false, false, false, false, true}},
{NewMapTypeRef(SimpleTypeInt32, SimpleTypeInt64, false), []bool{false, false, false, false, false, false, false, false, false, false, false, true}},
{BuiltInType("handle"), []bool{false, false, false, false, false, false, false, false, false, false, false, false}},
- {userTypeRef, []bool{false, false, true, true, true, true, true, true, true, true, false, true}},
+ // Assignments to UserTypeRefs are not validated at all during parsing.
+ {userTypeRef, []bool{true, true, true, true, true, true, true, true, true, true, true, true}},
}
for _, c := range cases {
for i, v := range literalValues {
@@ -337,10 +338,10 @@
// An enum type may be the type of a map key
{NewResolvedEnumRef(true, false, nil), true},
{NewResolvedEnumRef(false, false, &stringLiteral), false},
- // Enums ma be assigned an integer literal
- {NewResolvedEnumRef(false, false, &intLiteral), true},
+ // Enums may not be assigned an integer literal
+ {NewResolvedEnumRef(false, false, &intLiteral), false},
{NewResolvedEnumRef(false, false, &floatLiteral), false},
- {NewResolvedEnumRef(false, false, &defaultLiteral), true},
+ {NewResolvedEnumRef(false, false, &defaultLiteral), false},
}
for i, c := range cases {
success := nil == c.typeRef.validateAfterResolution()
diff --git a/mojom/mojom_parser/mojom/user_defined_types.go b/mojom/mojom_parser/mojom/user_defined_types.go
index c701e01..1bacd00 100644
--- a/mojom/mojom_parser/mojom/user_defined_types.go
+++ b/mojom/mojom_parser/mojom/user_defined_types.go
@@ -504,17 +504,6 @@
}
func (e MojomEnum) IsAssignmentCompatibleWith(value LiteralValue) bool {
- if value.IsDefault() {
- return true
- }
- t := value.LiteralValueType()
- if t == SimpleTypeInt8 || t == SimpleTypeInt16 || t == SimpleTypeInt32 || t == SimpleTypeInt64 {
- // TODO(rudominer) Finish MojomEnum.IsAssignmentCompatibleWith()
- // We should check that the value is in the range of the enum fields.
- // Do we want to deprecate the ability to assign an integer to an
- // enum varialbe?
- return true
- }
return false
}
diff --git a/mojom/mojom_parser/mojom/user_defined_types_test.go b/mojom/mojom_parser/mojom/user_defined_types_test.go
index 8cacc1f..b0b83a4 100644
--- a/mojom/mojom_parser/mojom/user_defined_types_test.go
+++ b/mojom/mojom_parser/mojom/user_defined_types_test.go
@@ -61,7 +61,7 @@
{NewTestStruct("struct"), []bool{false, false, false, false, true}},
{NewTestInterface("interface"), []bool{false, false, false, false, false}},
{NewTestUnion("union"), []bool{false, false, false, false, true}},
- {NewTestEnum("enum"), []bool{false, false, true, false, true}},
+ {NewTestEnum("enum"), []bool{false, false, false, false, false}},
}
for _, c := range cases {
for i, v := range literalValues {
diff --git a/mojom/mojom_parser/parser/parser_test.go b/mojom/mojom_parser/parser/parser_test.go
index d07f426..1a7d028 100644
--- a/mojom/mojom_parser/parser/parser_test.go
+++ b/mojom/mojom_parser/parser/parser_test.go
@@ -711,34 +711,6 @@
endTestCase()
////////////////////////////////////////////////////////////
- // Test Case (Assign string to user-defined-type)
- ////////////////////////////////////////////////////////////
- startTestCase("")
- cases[testCaseNum].mojomContents = `
- struct Foo {
- Foo x = "hello";
- };
-
- `
- expectError("Illegal assignment")
- expectError("Field x of type Foo may not be assigned the value \"hello\" of type string.")
- endTestCase()
-
- ////////////////////////////////////////////////////////////
- // Test Case (Assign string to user-defined-type)
- ////////////////////////////////////////////////////////////
- startTestCase("")
- cases[testCaseNum].mojomContents = `
- struct Foo {
- Foo x = "hello";
- };
-
- `
- expectError("Illegal assignment")
- expectError("Field x of type Foo may not be assigned the value \"hello\" of type string.")
- endTestCase()
-
- ////////////////////////////////////////////////////////////
// Group 2: Assign to constant.
////////////////////////////////////////////////////////////
@@ -765,17 +737,6 @@
endTestCase()
////////////////////////////////////////////////////////////
- // Test Case (Assign boolean to user-defined type)
- ////////////////////////////////////////////////////////////
- startTestCase("")
- cases[testCaseNum].mojomContents = `
- const FooType Foo = true;
- `
- expectError("Illegal assignment")
- expectError("Constant Foo of type FooType may not be assigned the value true of type bool.")
- endTestCase()
-
- ////////////////////////////////////////////////////////////
// Execute all of the test cases.
////////////////////////////////////////////////////////////
for i, c := range cases {
diff --git a/mojom/mojom_parser/parser/resolution_test.go b/mojom/mojom_parser/parser/resolution_test.go
index 57c0cb9..1025231 100644
--- a/mojom/mojom_parser/parser/resolution_test.go
+++ b/mojom/mojom_parser/parser/resolution_test.go
@@ -642,6 +642,10 @@
test := singleFileTest{}
////////////////////////////////////////////////////////////
+ // Group 1: Left-hand-side is a struct
+ ////////////////////////////////////////////////////////////
+
+ ////////////////////////////////////////////////////////////
// Test Case: Use struct as constant type
////////////////////////////////////////////////////////////
{
@@ -657,7 +661,7 @@
}
////////////////////////////////////////////////////////////
- // Test Case: Assign struct as map key
+ // Test Case: Use struct as map key
////////////////////////////////////////////////////////////
{
contents := `
@@ -691,6 +695,46 @@
}
////////////////////////////////////////////////////////////
+ // Group 2: Left-hand-side is an enum
+ ////////////////////////////////////////////////////////////
+
+ ////////////////////////////////////////////////////////////
+ // Test Case: Assign an integer to an enum variable in a struct field.
+ ////////////////////////////////////////////////////////////
+ {
+ contents := `
+ enum Hats {
+ COWBOY,
+ TOP
+ };
+
+ struct Bar{
+ Hats my_hat = 1;
+ };
+ `
+ test.addTestCase(contents, []string{
+ "Illegal assignment",
+ "Field my_hat of type Hats may not be assigned the value 1 of type int8."})
+ }
+
+ ////////////////////////////////////////////////////////////
+ // Test Case: Assign an integer to an enum variable in a constant.
+ ////////////////////////////////////////////////////////////
+ {
+ contents := `
+ enum Hats {
+ COWBOY,
+ TOP
+ };
+
+ const Hats MY_HAT = 1;
+ `
+ test.addTestCase(contents, []string{
+ "Illegal assignment",
+ "Const MY_HAT of type Hats may not be assigned the value 1 of type int8."})
+ }
+
+ ////////////////////////////////////////////////////////////
// Execute all of the test cases.
////////////////////////////////////////////////////////////
for i, c := range test.cases {
