Improve match() error messages by Crell · Pull Request #7312 · php/php-src

Crell · 2021-07-27T20:33:29Z

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

Zend/zend_vm_def.h

Zend/tests/match/043.phpt

iluuu1994

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.

Zend/tests/match/037.phpt

Zend/zend_vm_def.h

Crell · 2021-07-29T19:36:35Z

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 · 2021-07-29T21:27:07Z

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 ...

Crell added 3 commits July 27, 2021 13:17

Improve match() error messages.

091d7c3

Update tests.

d1975d7

Add new test for error messages.

00fc67b

kamil-tekiela reviewed Jul 27, 2021

View reviewed changes

Zend/zend_vm_def.h Show resolved Hide resolved

kamil-tekiela reviewed Jul 27, 2021

View reviewed changes

Zend/zend_vm_def.h Outdated Show resolved Hide resolved

Code style fixes.

2c58480

bwoebi reviewed Jul 27, 2021

View reviewed changes

Zend/tests/match/043.phpt Outdated Show resolved Hide resolved

iluuu1994 requested changes Jul 28, 2021

View reviewed changes

Zend/tests/match/037.phpt Outdated Show resolved Hide resolved

Zend/zend_vm_def.h Show resolved Hide resolved

krakjoe added Feature Waiting on Author labels Jul 28, 2021

Quote and truncate string values in match errors.

905c8b2

krakjoe mentioned this pull request Jul 29, 2021

Fix bug #81303 improve match errors #7318

Closed

krakjoe closed this Jul 29, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve match() error messages#7312

Improve match() error messages#7312
Crell wants to merge 5 commits intophp:masterfrom
Crell:match-error

Crell commented Jul 27, 2021 •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

Uh oh!

iluuu1994 left a comment

Uh oh!

Uh oh!

Uh oh!

Crell commented Jul 29, 2021

Uh oh!

krakjoe commented Jul 29, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Conversation

Crell commented Jul 27, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

iluuu1994 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Crell commented Jul 29, 2021

Uh oh!

krakjoe commented Jul 29, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Crell commented Jul 27, 2021 •

edited

Loading