Skip to content

Improve match() error messages #7312

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 5 commits into from
Closed

Improve match() error messages #7312

wants to merge 5 commits into from

Conversation

Crell
Copy link
Contributor

@Crell Crell commented Jul 27, 2021

Fixes bug #81303 Error message for match() is misleading and unhelpful

Copy link
Member

@iluuu1994 iluuu1994 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Side note: I already did this in the original PR but removed it after Mate and Nikita noted that it was inconsistent with the current error messages. It also included some string sanitization (at least truncating, maybe more, I don't remember) but there are quite a few edge cases one can run into here.

@Crell
Copy link
Contributor Author

Crell commented Jul 29, 2021

Now truncates a string at 10 chars, adds an ellipsis, and shows it in quotes. Thanks to Sara and Joe for their help in dealing with C strings...

@krakjoe
Copy link
Member

krakjoe commented Jul 29, 2021

diff --git a/Zend/tests/match/043.phpt b/Zend/tests/match/043.phpt
index 0e50bae15b..fad43897b2 100644
--- a/Zend/tests/match/043.phpt
+++ b/Zend/tests/match/043.phpt
@@ -26,17 +26,12 @@ test(str_repeat("e\n", 100));
 
 ?>
 --EXPECT--
-Unhandled match value: 1
-Unhandled match value: 5.5
-Unhandled match value: "foo"
-Unhandled match value: true
-Unhandled match value: false
-Unhandled match value of type array
-Unhandled match value of type Beep
-Unhandled match value: "eeeeeeeeee..."
-Unhandled match value: "e
-e
-e
-e
-e
-..."
+Unhandled match int(1)
+Unhandled match float(5.5)
+Unhandled match "foo..."
+Unhandled match bool(true)
+Unhandled match bool(false)
+Unhandled match of type array
+Unhandled match of type Beep
+Unhandled match "eeeeeeeeeeeeeeeeeeeeeeeeeeeeeeee..."
+Unhandled match "e\ne\ne\ne\ne\ne\ne\ne\ne\ne\ne\ne\ne\ne\ne\ne\n..."
diff --git a/Zend/zend_execute.c b/Zend/zend_execute.c
index cc9fe3ce3d..68ba5e23b6 100644
--- a/Zend/zend_execute.c
+++ b/Zend/zend_execute.c
@@ -824,6 +824,56 @@ ZEND_COLD zend_never_inline void zend_verify_property_type_error(zend_property_i
 	zend_string_release(type_str);
 }
 
+static zend_always_inline zend_string* zend_match_unhandled_string(zend_string *input, size_t length) {
+	smart_str output = {
+		.s = NULL,
+		.a = 0
+	};
+
+	smart_str_append_escaped(&output, ZSTR_VAL(input), MIN(length, ZSTR_LEN(input)));
+	smart_str_appendl(&output, "...", sizeof("..."));
+
+	return output.s;
+}
+
+ZEND_COLD void zend_match_unhandled_error(zval *value)
+{
+	if (Z_TYPE_P(value) == IS_TRUE || Z_TYPE_P(value) == IS_FALSE) {
+		zend_throw_exception_ex(
+			zend_ce_unhandled_match_error, 0,
+			"Unhandled match bool(%s)", 
+			Z_TYPE_P(value) == IS_TRUE ? "true" : "false");
+		return;
+	}
+
+	if (Z_TYPE_P(value) < IS_STRING) {
+		zend_string *string = zval_get_string(value);
+
+		zend_throw_exception_ex(
+			zend_ce_unhandled_match_error, 0,
+			"Unhandled match %s(%s)",
+			zend_get_type_by_const(Z_TYPE_P(value)), ZSTR_VAL(string));
+		zend_string_release(string);
+		return;
+	}
+
+	if (Z_TYPE_P(value) == IS_STRING) {
+		zend_string *string = zend_match_unhandled_string(Z_STR_P(value), 32);
+
+		zend_throw_exception_ex(
+			zend_ce_unhandled_match_error, 0,
+			"Unhandled match \"%s\"",
+			ZSTR_VAL(string));
+		zend_string_release(string);
+		return;
+	}
+
+	zend_throw_exception_ex(
+		zend_ce_unhandled_match_error, 0,
+		"Unhandled match of type %s",
+		zend_zval_type_name(value));
+}
+
 ZEND_API ZEND_COLD void ZEND_FASTCALL zend_readonly_property_modification_error(
 		zend_property_info *info) {
 	zend_throw_error(NULL, "Cannot modify readonly property %s::$%s",
diff --git a/Zend/zend_execute.h b/Zend/zend_execute.h
index 2118e15ffd..f47235f05b 100644
--- a/Zend/zend_execute.h
+++ b/Zend/zend_execute.h
@@ -469,6 +469,8 @@ ZEND_COLD void zend_verify_property_type_error(zend_property_info *info, zval *p
 		} \
 	} while (0)
 
+ZEND_COLD void zend_match_unhandled_error(zval *value);
+
 END_EXTERN_C()
 
 #endif /* ZEND_EXECUTE_H */
diff --git a/Zend/zend_vm_def.h b/Zend/zend_vm_def.h
index 00a8c617dc..c29224dce5 100644
--- a/Zend/zend_vm_def.h
+++ b/Zend/zend_vm_def.h
@@ -8935,42 +8935,7 @@ ZEND_VM_COLD_CONST_HANDLER(197, ZEND_MATCH_ERROR, CONST|TMPVARCV, UNUSED)
 
 	SAVE_OPLINE();
 	op = GET_OP1_ZVAL_PTR_UNDEF(BP_VAR_R);
-
-	// For simpler types where there is a convenient stringified version, include that
-	// in the error message. Otherwise just show its type.
-	switch (Z_TYPE_P(op)) {
-		case IS_FALSE:
-			zend_throw_exception_ex(zend_ce_unhandled_match_error, 0, "Unhandled match value: false");
-			break;
-		case IS_TRUE:
-			zend_throw_exception_ex(zend_ce_unhandled_match_error, 0, "Unhandled match value: true");
-			break;
-		case IS_LONG:
-		case IS_DOUBLE: {
-			zend_string* stringified = zval_get_string(op);
-			zend_throw_exception_ex(zend_ce_unhandled_match_error, 0, "Unhandled match value: %s", ZSTR_VAL(stringified));
-			zend_string_release_ex(stringified, false);
-			break;
-		}
-		case IS_STRING: {
-			zend_string* stringified = zval_get_string(op);
-			// This number was chosen mostly arbitrarily. But in context,
-			// a match() on a string should only be using short strings so something
-			// longer is unlikely to happen to begin with.
-			const int max_strlen = 10;
-            if (ZSTR_LEN(stringified) > max_strlen) {
-				stringified = zend_string_realloc(stringified, max_strlen + 3, 0);
-				memcpy(&ZSTR_VAL(stringified)[max_strlen], "...", sizeof("..."));
-            }
-			zend_throw_exception_ex(zend_ce_unhandled_match_error, 0, "Unhandled match value: \"%s\"", ZSTR_VAL(stringified));
-			zend_string_release_ex(stringified, false);
-			break;
-		}
-		default:
-			zend_throw_exception_ex(zend_ce_unhandled_match_error, 0, "Unhandled match value of type %s", zend_zval_type_name(op));
-			break;
-		}
-
+	zend_match_unhandled_error(op);
 	HANDLE_EXCEPTION();
 }
 
diff --git a/Zend/zend_vm_execute.h b/Zend/zend_vm_execute.h
index d513a8d720..7b2675ee80 100644
--- a/Zend/zend_vm_execute.h
+++ b/Zend/zend_vm_execute.h
@@ -10507,42 +10507,7 @@ static ZEND_VM_COLD ZEND_OPCODE_HANDLER_RET ZEND_FASTCALL ZEND_MATCH_ERROR_SPEC_
 
 	SAVE_OPLINE();
 	op = RT_CONSTANT(opline, opline->op1);
-
-	// For simpler types where there is a convenient stringified version, include that
-	// in the error message. Otherwise just show its type.
-	switch (Z_TYPE_P(op)) {
-		case IS_FALSE:
-			zend_throw_exception_ex(zend_ce_unhandled_match_error, 0, "Unhandled match value: false");
-			break;
-		case IS_TRUE:
-			zend_throw_exception_ex(zend_ce_unhandled_match_error, 0, "Unhandled match value: true");
-			break;
-		case IS_LONG:
-		case IS_DOUBLE: {
-			zend_string* stringified = zval_get_string(op);
-			zend_throw_exception_ex(zend_ce_unhandled_match_error, 0, "Unhandled match value: %s", ZSTR_VAL(stringified));
-			zend_string_release_ex(stringified, false);
-			break;
-		}
-		case IS_STRING: {
-			zend_string* stringified = zval_get_string(op);
-			// This number was chosen mostly arbitrarily. But in context,
-			// a match() on a string should only be using short strings so something
-			// longer is unlikely to happen to begin with.
-			const int max_strlen = 10;
-            if (ZSTR_LEN(stringified) > max_strlen) {
-				stringified = zend_string_realloc(stringified, max_strlen + 3, 0);
-				memcpy(&ZSTR_VAL(stringified)[max_strlen], "...", sizeof("..."));
-            }
-			zend_throw_exception_ex(zend_ce_unhandled_match_error, 0, "Unhandled match value: \"%s\"", ZSTR_VAL(stringified));
-			zend_string_release_ex(stringified, false);
-			break;
-		}
-		default:
-			zend_throw_exception_ex(zend_ce_unhandled_match_error, 0, "Unhandled match value of type %s", zend_zval_type_name(op));
-			break;
-		}
-
+	zend_match_unhandled_error(op);
 	HANDLE_EXCEPTION();
 }
 
@@ -14068,42 +14033,7 @@ static ZEND_OPCODE_HANDLER_RET ZEND_FASTCALL ZEND_MATCH_ERROR_SPEC_TMPVARCV_UNUS
 
 	SAVE_OPLINE();
 	op = EX_VAR(opline->op1.var);
-
-	// For simpler types where there is a convenient stringified version, include that
-	// in the error message. Otherwise just show its type.
-	switch (Z_TYPE_P(op)) {
-		case IS_FALSE:
-			zend_throw_exception_ex(zend_ce_unhandled_match_error, 0, "Unhandled match value: false");
-			break;
-		case IS_TRUE:
-			zend_throw_exception_ex(zend_ce_unhandled_match_error, 0, "Unhandled match value: true");
-			break;
-		case IS_LONG:
-		case IS_DOUBLE: {
-			zend_string* stringified = zval_get_string(op);
-			zend_throw_exception_ex(zend_ce_unhandled_match_error, 0, "Unhandled match value: %s", ZSTR_VAL(stringified));
-			zend_string_release_ex(stringified, false);
-			break;
-		}
-		case IS_STRING: {
-			zend_string* stringified = zval_get_string(op);
-			// This number was chosen mostly arbitrarily. But in context,
-			// a match() on a string should only be using short strings so something
-			// longer is unlikely to happen to begin with.
-			const int max_strlen = 10;
-            if (ZSTR_LEN(stringified) > max_strlen) {
-				stringified = zend_string_realloc(stringified, max_strlen + 3, 0);
-				memcpy(&ZSTR_VAL(stringified)[max_strlen], "...", sizeof("..."));
-            }
-			zend_throw_exception_ex(zend_ce_unhandled_match_error, 0, "Unhandled match value: \"%s\"", ZSTR_VAL(stringified));
-			zend_string_release_ex(stringified, false);
-			break;
-		}
-		default:
-			zend_throw_exception_ex(zend_ce_unhandled_match_error, 0, "Unhandled match value of type %s", zend_zval_type_name(op));
-			break;
-		}
-
+	zend_match_unhandled_error(op);
 	HANDLE_EXCEPTION();
 }

I used 32, some class names are long ...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants