dfilter: Remove commute argument from semantic check

Take a more conservative, less flexible, maybe more elegant,
approach to type inference for now.

3 files changed, 97 insertions, 38 deletions

diff --git a/epan/dfilter/semcheck.c b/epan/dfilter/semcheck.c
index a3c259109f..a08cc6c7d5 100644
--- a/epan/dfilter/semcheck.c
+++ b/epan/dfilter/semcheck.c
@@ -47,8 +47,7 @@ check_arithmetic_LHS(dfwork_t *dfw, stnode_op_t st_op,
 static void
 check_relation(dfwork_t *dfw, stnode_op_t st_op,
 		FtypeCanFunc can_func, gboolean allow_partial_value,
-		stnode_t *st_node, stnode_t *st_arg1, stnode_t *st_arg2,
-		int commute);
+		stnode_t *st_node, stnode_t *st_arg1, stnode_t *st_arg2);
 
 static void
 semcheck(dfwork_t *dfw, stnode_t *st_node);
@@ -714,6 +713,80 @@ check_relation_LHS_FIELD(dfwork_t *dfw, stnode_op_t st_op,
 }
 
 static void
+check_relation_LHS_FVALUE(dfwork_t *dfw, stnode_op_t st_op _U_,
+		FtypeCanFunc can_func, gboolean allow_partial_value,
+		stnode_t *st_node,
+		stnode_t *st_arg1, stnode_t *st_arg2)
+{
+	sttype_id_t		type1, type2;
+	header_field_info	*hfinfo2 = NULL;
+	ftenum_t		ftype2;
+	fvalue_t		*fvalue;
+
+	LOG_NODE(st_node);
+
+	type2 = stnode_type_id(st_arg2);
+
+	if (IS_FIELD_ENTITY(type2)) {
+		hfinfo2 = sttype_field_hfinfo(st_arg2);
+		ftype2 = sttype_field_ftenum(st_arg2);
+
+		if (!can_func(ftype2)) {
+			FAIL(dfw, st_arg2, "%s (type=%s) cannot participate in specified comparison.",
+					stnode_todisplay(st_arg2), ftype_pretty_name(ftype2));
+		}
+	}
+	else if (type2 == STTYPE_STRING ||
+				type2 == STTYPE_LITERAL ||
+				type2 == STTYPE_CHARCONST ||
+				type2 == STTYPE_PCRE) {
+		FAIL(dfw, st_node, "Constant expression is invalid.");
+	}
+	else if (type2 == STTYPE_SLICE) {
+		ftype2 = check_slice(dfw, st_arg2, FT_NONE);
+
+		if (!can_func(ftype2)) {
+			FAIL(dfw, st_arg2, "%s (type=%s) cannot participate in specified comparison.",
+					stnode_todisplay(st_arg2), ftype_pretty_name(ftype2));
+		}
+	}
+	else if (type2 == STTYPE_FUNCTION) {
+		ftype2 = check_function(dfw, st_arg2, FT_NONE);
+
+		if (!can_func(ftype2)) {
+			FAIL(dfw, st_arg2, "return value of %s() (type=%s) cannot participate in specified comparison.",
+					sttype_function_name(st_arg2), ftype_pretty_name(ftype2));
+		}
+	}
+	else if (type2 == STTYPE_ARITHMETIC) {
+		ftype2 = check_arithmetic(dfw, st_arg2, FT_NONE);
+
+		if (!can_func(ftype2)) {
+			FAIL(dfw, st_arg2, "%s (type=%s) cannot participate in specified comparison.",
+					stnode_todisplay(st_arg2), ftype_pretty_name(ftype2));
+		}
+	}
+	else {
+		ws_assert_not_reached();
+	}
+
+	type1 = stnode_type_id(st_arg1);
+	if (type1 == STTYPE_STRING) {
+		fvalue = dfilter_fvalue_from_string(dfw, ftype2, st_arg1, hfinfo2);
+	}
+	else if (type1 == STTYPE_LITERAL) {
+		fvalue = dfilter_fvalue_from_literal(dfw, ftype2, st_arg1, allow_partial_value, hfinfo2);
+	}
+	else if (type1 == STTYPE_CHARCONST) {
+		fvalue = dfilter_fvalue_from_charconst(dfw, ftype2, st_arg1);
+	}
+	else {
+		ws_assert_not_reached();
+	}
+	stnode_replace(st_arg1, STTYPE_FVALUE, fvalue);
+}
+
+static void
 check_relation_LHS_SLICE(dfwork_t *dfw, stnode_op_t st_op,
 		FtypeCanFunc can_func _U_,
 		gboolean allow_partial_value,
@@ -812,8 +885,7 @@ check_relation_LHS_SLICE(dfwork_t *dfw, stnode_op_t st_op,
 static void
 check_relation_LHS_FUNCTION(dfwork_t *dfw, stnode_op_t st_op,
 		FtypeCanFunc can_func, gboolean allow_partial_value,
-		stnode_t *st_node, stnode_t *st_arg1, stnode_t *st_arg2,
-		int commute)
+		stnode_t *st_node, stnode_t *st_arg1, stnode_t *st_arg2)
 {
 	sttype_id_t		type2;
 	ftenum_t		ftype1, ftype2;
@@ -823,9 +895,7 @@ check_relation_LHS_FUNCTION(dfwork_t *dfw, stnode_op_t st_op,
 
 	ftype1 = check_function(dfw, st_arg1, FT_NONE);
 	if (ftype1 == FT_NONE) {
-		check_relation(dfw, st_op, can_func, allow_partial_value,
-				st_node, st_arg2, st_arg1, commute - 1);
-		return;
+		FAIL(dfw, st_arg1, "Constant expression is invalid on the LHS.");
 	}
 	if (!can_func(ftype1)) {
 		FAIL(dfw, st_arg1, "Function %s (type=%s) cannot participate in %s comparison.",
@@ -923,8 +993,7 @@ check_relation_LHS_FUNCTION(dfwork_t *dfw, stnode_op_t st_op,
 static void
 check_relation_LHS_ARITHMETIC(dfwork_t *dfw, stnode_op_t st_op,
 		FtypeCanFunc can_func, gboolean allow_partial_value,
-		stnode_t *st_node, stnode_t *st_arg1, stnode_t *st_arg2,
-		int commute)
+		stnode_t *st_node, stnode_t *st_arg1, stnode_t *st_arg2)
 {
 	sttype_id_t		type2;
 	ftenum_t		ftype1, ftype2;
@@ -934,11 +1003,8 @@ check_relation_LHS_ARITHMETIC(dfwork_t *dfw, stnode_op_t st_op,
 
 	ftype1 = check_arithmetic(dfw, st_arg1, FT_NONE);
 	if (ftype1 == FT_NONE) {
-		check_relation(dfw, st_op, can_func, allow_partial_value,
-				st_node, st_arg2, st_arg1, commute - 1);
-		return;
+		FAIL(dfw, st_arg1, "Constant expression is invalid on the LHS.");
 	}
-
 	if (!can_func(ftype1)) {
 		FAIL(dfw, st_arg1, "Result with type %s cannot participate in %s comparison.",
 				ftype_pretty_name(ftype1),
@@ -1030,17 +1096,10 @@ check_relation_LHS_ARITHMETIC(dfwork_t *dfw, stnode_op_t st_op,
 static void
 check_relation(dfwork_t *dfw, stnode_op_t st_op,
 		FtypeCanFunc can_func, gboolean allow_partial_value,
-		stnode_t *st_node, stnode_t *st_arg1, stnode_t *st_arg2,
-		int commute)
+		stnode_t *st_node, stnode_t *st_arg1, stnode_t *st_arg2)
 {
 	LOG_NODE(st_node);
 
-	if (commute < 0) {
-		/* We have already commuted the LHS with the RHS and still
-		   cannot assign a field type to any side of the relation. */
-		FAIL(dfw, st_node, "Constant expression is invalid.");
-	}
-
 	switch (stnode_type_id(st_arg1)) {
 		case STTYPE_FIELD:
 		case STTYPE_REFERENCE:
@@ -1053,20 +1112,17 @@ check_relation(dfwork_t *dfw, stnode_op_t st_op,
 			break;
 		case STTYPE_FUNCTION:
 			check_relation_LHS_FUNCTION(dfw, st_op, can_func,
-					allow_partial_value, st_node, st_arg1, st_arg2, commute);
+					allow_partial_value, st_node, st_arg1, st_arg2);
 			break;
 		case STTYPE_ARITHMETIC:
 			check_relation_LHS_ARITHMETIC(dfw, st_op, can_func,
-					allow_partial_value, st_node, st_arg1, st_arg2, commute);
+					allow_partial_value, st_node, st_arg1, st_arg2);
 			break;
 		case STTYPE_LITERAL:
 		case STTYPE_STRING:
 		case STTYPE_CHARCONST:
-			/* We cannot semantically check a relation with literals on the LHS because we
-			   don't have a type to assign them. Commute the LHS with the RHS and retry
-			   the relation semantic check. */
-			check_relation(dfw, st_op, can_func,
-					allow_partial_value,st_node, st_arg2, st_arg1, commute - 1);
+			check_relation_LHS_FVALUE(dfw, st_op, can_func,
+					allow_partial_value, st_node, st_arg1, st_arg2);
 			break;
 		default:
 			/* Should not happen. */
@@ -1110,7 +1166,7 @@ check_relation_contains(dfwork_t *dfw, stnode_t *st_node,
 			break;
 		case STTYPE_FUNCTION:
 			check_relation_LHS_FUNCTION(dfw, STNODE_OP_CONTAINS, ftype_can_contains,
-							TRUE, st_node, st_arg1, st_arg2, 0);
+							TRUE, st_node, st_arg1, st_arg2);
 			break;
 		case STTYPE_SLICE:
 			check_relation_LHS_SLICE(dfw, STNODE_OP_CONTAINS, ftype_can_contains,
@@ -1157,7 +1213,7 @@ check_relation_matches(dfwork_t *dfw, stnode_t *st_node,
 			break;
 		case STTYPE_FUNCTION:
 			check_relation_LHS_FUNCTION(dfw, STNODE_OP_MATCHES, ftype_can_matches,
-							TRUE, st_node, st_arg1, st_arg2, 0);
+							TRUE, st_node, st_arg1, st_arg2);
 			break;
 		case STTYPE_SLICE:
 			check_relation_LHS_SLICE(dfw, STNODE_OP_MATCHES, ftype_can_matches,
@@ -1238,13 +1294,13 @@ check_test(dfwork_t *dfw, stnode_t *st_node)
 		case STNODE_OP_ANY_EQ:
 		case STNODE_OP_ALL_NE:
 		case STNODE_OP_ANY_NE:
-			check_relation(dfw, st_op, ftype_can_eq, FALSE, st_node, st_arg1, st_arg2, 1);
+			check_relation(dfw, st_op, ftype_can_eq, FALSE, st_node, st_arg1, st_arg2);
 			break;
 		case STNODE_OP_GT:
 		case STNODE_OP_GE:
 		case STNODE_OP_LT:
 		case STNODE_OP_LE:
-			check_relation(dfw, st_op, ftype_can_cmp, FALSE, st_node, st_arg1, st_arg2, 1);
+			check_relation(dfw, st_op, ftype_can_cmp, FALSE, st_node, st_arg1, st_arg2);
 			break;
 		case STNODE_OP_CONTAINS:
 			check_relation_contains(dfw, st_node, st_arg1, st_arg2);
diff --git a/test/suite_dfilter/group_function.py b/test/suite_dfilter/group_function.py
index 517fa91f34..30e1c77de3 100644
--- a/test/suite_dfilter/group_function.py
+++ b/test/suite_dfilter/group_function.py
@@ -70,9 +70,10 @@ class case_dfunction_maxmin(unittest.TestCase):
         dfilter = 'max(5060, udp.dstport) == udp.srcport'
         checkDFilterCount(dfilter, 2)
 
-    def test_max_5(self, checkDFilterCount):
+    def test_max_5(self, checkDFilterFail):
+        error = 'Constant expression is invalid on the LHS'
         dfilter = 'max(5060, 5070) == udp.srcport'
-        checkDFilterCount(dfilter, 1)
+        checkDFilterFail(dfilter, error)
 
 @fixtures.uses_fixtures
 class case_dfunction_abs(unittest.TestCase):
diff --git a/test/suite_dfilter/group_syntax.py b/test/suite_dfilter/group_syntax.py
index 829f4e6d8c..89e442e539 100644
--- a/test/suite_dfilter/group_syntax.py
+++ b/test/suite_dfilter/group_syntax.py
@@ -237,9 +237,10 @@ class case_unary_minus(unittest.TestCase):
         dfilter = "tcp.window_size_scalefactor == +tcp.dstport"
         checkDFilterCount(dfilter, 0)
 
-    def test_unary_3(self, checkDFilterCount):
+    def test_unary_3(self, checkDFilterFail):
+        error = 'Constant expression is invalid on the LHS'
         dfilter = "-2 == tcp.dstport"
-        checkDFilterCount(dfilter, 0)
+        checkDFilterFail(dfilter, error)
 
     def test_unary_4(self, checkDFilterCount):
         dfilter = "tcp.window_size_scalefactor == -{tcp.dstport * 20}"
@@ -261,9 +262,10 @@ class case_arithmetic(unittest.TestCase):
         dfilter = "udp.dstport == 66+1"
         checkDFilterCount(dfilter, 2)
 
-    def test_add_4(self, checkDFilterCount):
+    def test_add_4(self, checkDFilterFail):
+        error = 'Constant expression is invalid on the LHS'
         dfilter = "1 + 2 == frame.number"
-        checkDFilterCount(dfilter, 1)
+        checkDFilterFail(dfilter, error)
 
     def test_add_5(self, checkDFilterFail):
         error = 'Constant expression is invalid'