Call error handler for E_COMPILE_WARNING #4758
Conversation
Instead don't throw them in the first place if we're not in PARSER_MODE().
|
This seems reasonable - I don't have any suggestions other changes to the source and this PR should fix my bug report. I'd really want the test repeated for I'm not familiar with why Optionally, tests could be added for The build should be re-triggered, it failed due to travis failing to download libraries. |
|
@nikic do you know why it was disabled before? |
We do call some PHP callbacks (e.g. E_DEPRECATED) from the compiler I wrote up comments on why I thought it was disabled before in https://bugs.php.net/bug.php?id=78537 and the comments for the original report.
|
Not sure what you mean here. What should I test additionally beyond what is already in https://github.com/php/php-src/pull/4758/files#diff-8aab17a14b90f17ed97ba4b696c31cd1?
I agree that having this as a warning does not make sense. I will change this for PHP 8, either to throw ParseError in the lexer, or return T_BAD_CHARACTER to the parser and make it trigger a parse error there.
There should already be tests for those error conditions together with token_get_all(), though I haven't checked all of them. |
|
I've opened #4767 to make "unexpected character" a parse error. |
Oh. I missed the second test when making that comment because it was files away from the other test. The tests look good. |
|
Overall I'm not happy with this change. While throwing exceptions is safe in the lexer, it can be problematic for compile warnings thrown during compilation itself. Nothing really terrible is going to happen, but because the exception will not be handled immediately, compilation might still proceed further than it should (say register a function, even though a warning was promoted to exception earlier in the code and should have prevented that). Now that the "unexpected character" case is a ParseError, and I've also made unterminated comments a ParseError in e0a4013, I think there is not a lot of motivation left for doing this change. |
|
As mentioned in my previous comment, I don't think doing this is a good idea anymore, so I'm going to close this PR. The relevant warnings are ParseError in PHP 8, so there isn't a lot of motivation for this anymore. |
Emit an E_COMPILE_WARNING if these are seen. (See php#4758) due to concerns about being unsafe to handle in user-space.) - E_COMPILE_WARNING is also emitted for "Octal escape sequence overflow" but it's been long enough to consider changing that. See phpGH-4758. - Making this proposal suddenly a ParseError in php 8.1 seems too soon. TODO: Warn for octal and binary literals as well. I expect this to behave the same on 32-bit and 64-bit builds (floats are 64 bits on both) For example, `0xffff_ffff_ffff_f400` overflows and becomes the **float** `0xffff_ffff_ffff_f000`. This will warn about that. Instead of using `0xffff_ffff_ffff_f400` with binary bitwise operands, most code should use the signed 64-bit int `~0xbff`.
Emit an E_COMPILE_WARNING if these are seen in hexadecimal, octal, or binary literals. - E_COMPILE_WARNING is also emitted for "Octal escape sequence overflow" but it's been long enough to consider changing that. See phpGH-4758. - Making this proposal suddenly a ParseError in php 8.1 seems too soon. I expect this to behave the same on 32-bit and 64-bit builds (floats are 64 bits on both) For example, `0xffff_ffff_ffff_f400` overflows and becomes the **float** `0xffff_ffff_ffff_f000`. This PR will warn about that. Instead of using `0xffff_ffff_ffff_f400` with binary bitwise operands, most code should use the signed 64-bit int `~0xbff`. - E.g. PHP code ported from cryptography algorithms or other C code doing bitwise operations.
Understandable - I'm able to work around any E_COMPILE_WARNINGS using error reporting level changes and error_get_last(), so I don't have a strong need to change this. It occurs to me that that's true for current warnings, but future changes to stricten lexing in PHP 8.x might go through a warning/deprecation phase to give code time to migrate before the next PHP major version. How should deprecations while lexing be approached in future PRs similar to the one linked? An E_LEXING_DEPRECATION seems way too specific, and E_DEPRECATION might be possible. There's also still an E_COMPILE_WARNING in the lexer for invalid octal literals in strings. |
Emit an E_COMPILE_WARNING if these are seen in hexadecimal, octal, or binary literals. - E_COMPILE_WARNING is also emitted for "Octal escape sequence overflow" but it's been long enough to consider changing that. See phpGH-4758. - Making this proposal suddenly a ParseError in php 8.1 seems too soon. I expect this to behave the same on 32-bit and 64-bit builds (floats are 64 bits on both) For example, `0xffff_ffff_ffff_f400` overflows and becomes the **float** `0xffff_ffff_ffff_f000`. This PR will warn about that. Instead of using `0xffff_ffff_ffff_f400` with binary bitwise operands, most code should use the signed 64-bit int `~0xbff`. - E.g. PHP code ported from cryptography algorithms or other C code doing bitwise operations.
Emit an E_COMPILE_WARNING if these are seen in hexadecimal, octal, or binary literals. - E_COMPILE_WARNING is also emitted for "Octal escape sequence overflow" but it's been long enough to consider changing that. See phpGH-4758. - Making this proposal suddenly a ParseError in php 8.1 seems too soon. I expect this to behave the same on 32-bit and 64-bit builds (floats are 64 bits on both) For example, `0xffff_ffff_ffff_f400` overflows and becomes the **float** `0xffff_ffff_ffff_f000`. This PR will warn about that. Instead of using `0xffff_ffff_ffff_f400` with binary bitwise operands, most code should use the signed 64-bit int `~0xbff`. - E.g. PHP code ported from cryptography algorithms or other C code doing bitwise operations.
Fixes bug #78537. Calling error handler from the compiler is considered safe nowadays.
The main complication is that token_get_all() currently discards exceptions thrown during lexing. However, if an error handler throws an exception, we wouldn't want to discard that. So instead this changes the lexer to only throw exceptions if PARSER_MODE() is enabled.
cc @TysonAndre