Skip to content

Fixed bug #75218 #2767

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 1 commit into from
Closed

Fixed bug #75218 #2767

wants to merge 1 commit into from

Conversation

nikic
Copy link
Member

@nikic nikic commented Sep 24, 2017

I've introduced a new CompileError type, from which ParseError inherits. These errors are not parse errors in the narrow sense of the term, even though they happen to be generated during parsing in our implementation. Additionally reusing the ParseError class for this purpose would change existing error messages (if the exception is not caught) from a "Fatal error:" to a "Parse error:" prefix, and also the error kind from E_COMPILE_ERROR to E_PARSE.

Bug #75218

I've introduced a new CompileError type, from which ParseError
inherits. These errors are not parse errors in the narrow sense
of the term, even though they happen to be generated during
parsing in our implementation. Additionally reusing the ParseError
class for this purpose would change existing error messages (if
the exception is not caught) from a "Fatal error:" to a "Parse
error:" prefix, and also the error kind from E_COMPILE_ERROR to
E_PARSE.
@nikic
Copy link
Member Author

nikic commented Sep 24, 2017

I'd mainly like to have feedback on the change to the exception hierarchy.

@TysonAndre
Copy link
Contributor

I'd love to get this into 7.3. Do you have anyone in mind as a reviewer? (I don't see any obvious issues, but I'm not a maintainer)

Something I didn't know about when filing the bug: This could also test token_get_all('<?php abstract final class X { }', true)) to test token_get_all having proper error handling, even if the implementation is refactored later.
(true indicating that it additionally validates the AST.)

Thoughts on the change to the exception hierarchy (Unimportant to me compared to avoiding the fatal error):

  • Some other checks are technically not parse errors in the AST, but post-parsing validation errors (e.g. verifying that the default value of a property is not a dynamic value (e.g. public $x = self::$y;)). Making those CompileError as well might make an error hierarchy more consistent.
  • I'm also fine with making everything a ParseError to start with, since I can't think of many differences in how a CompileError and a ParseError would be handled, even in applications such as analyzers, code style fixers, etc.

@nikic
Copy link
Member Author

nikic commented Jun 16, 2018

Merged as d04917c, with a note in UPGRADING.

I stuck with CompileError here to make sure error types & messages stay the same, and because I expect that we'll need this anyway when converting the compiler to exceptions.

@nikic nikic closed this Jun 16, 2018
zend_string *message = zval_get_string(GET_PROPERTY(&exception, ZEND_STR_MESSAGE));
zend_string *file = zval_get_string(GET_PROPERTY_SILENT(&exception, ZEND_STR_FILE));
zend_long line = zval_get_long(GET_PROPERTY_SILENT(&exception, ZEND_STR_LINE));

zend_error_helper(E_PARSE, ZSTR_VAL(file), line, "%s", ZSTR_VAL(message));
zend_error_helper(ce_exception == zend_ce_parse_error ? E_PARSE : E_COMPILE_ERROR,
Copy link
Contributor

@TysonAndre TysonAndre Sep 4, 2019

Choose a reason for hiding this comment

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

There's a surprising consequence of this: CompileError gets treated like a "Fatal error" instead of a "Catchable fatal error" when running with php -a.

the CompileError thrown by token_get_all() also terminates php -a

Perhaps E_COMPILE_ERROR can be treated the same way E_PARSE is in the below block. I'm not sure how to differentiate zend_error_helper from zend_error_noreturn and other similar functions.

// in main/main.c
			case E_ERROR:
			case E_CORE_ERROR:
			case E_COMPILE_ERROR:
			case E_USER_ERROR:
				error_type_str = "Fatal error";
				syslog_type_int = LOG_ERR;
				break;
php > throw new ParseError('x');
Parse error: x in php shell code on line 1
php > throw new CompileError('x');
Fatal error: x in php shell code on line 1
.... This terminates php -a

php > try {throw new CompileError('x');} catch (Throwable $_) { var_export($_); }
CompileError::__set_state(array(...

Filed https://bugs.php.net/bug.php?id=78489&thanks=4

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.

3 participants