rfc:engine_warnings
This is an old revision of the document!
PHP RFC: Reclassifying engine warnings
- Date: 2019-08-27
- Author: Nikita Popov nikic@php.net
- Status: Draft
- Target Version: PHP 8.0
Introduction
While newly introduced error conditions in the engine typically use Error exceptions, we have many old error conditions that use inappropriate severity levels for historical reasons. For example, accessing an undefined variable, while being a very severe programming error, only generates a notice. This RFC proposed to reevaluate existing error conditions and reclassify their severity level as appropriate.
Proposal
The following table contains a list of errors with notice or warning severity generated in the engine, excluding warnings that are generated by functions which have an optimized opcode implementation.
The table shows both the current error level, as well as the proposed level. A rationale for the proposed change (or non-change) is provided below a group of errors.
| Message | Current Level | Proposed Level |
|---|---|---|
| Undefined variable: %s | Notice | Error exception |
| Rationale: In most cases, an undefined variable is a severe programming error. The current low classification of this error is likely a legacy from the Dark Ages of register_globals, where it was more normal to have variables that are not always available. | ||
| Attempt to increment/decrement property '%s' of non-object | Warning | Error exception |
| Attempt to modify property '%s' of non-object | Warning | Error exception |
| Attempt to assign property '%s' of non-object | Warning | Error exception |
| Creating default object from empty value | Warning | Error exception |
| Rationale: These are all related errors that are generated when a property is accessed on a non-object inside a write context. If the non-object is “truthy” a warning is generated and the operation is ignored, if it is “falsy” an empty stdClass object is created instead. While auto-vivification is a core part of the language for arrays, the same is not the case for objects, and creating a property on a non-object is almost certainly a programming error rather than an intentional action. | ||
| Trying to get property '%s' of non-object | Notice | Warning |
| Undefined property: %s::$%s | Notice | Warning |
| Rationale: The first warning is for the same case as above, but for read contexts. The suggested classification is based on two facts: First, PHP is generally somewhat lenient about reading of “missing” data, and there is likely significant code around there using this fact, so changing this into an exception would be ill-advised. Second, apart from specific cases like JSON data in object representation, available object properties are generally known and fixed and accessing a non-existing property is a severe programming error, similar to accessing an undefined variable. As such, the current notice classification is too low. | ||
| Cannot add element to the array as the next element is already occupied | Warning | Error exception |
Rationale: This error condition occurs when trying to push to an array for which the PHP_INT_MAX key is already used. This error condition practically never occurs outside of specially crafted code, and implies data loss if it does. As such, it is changed into an exception. |
||
| Division by zero | Warning | DivisionByZeroError exception |
Rationale: During the development of PHP 7, two possible behaviors for division by zero were considered: Allowing it silently and returning floating-point NaN per IEEE 754 semantics (useful for numerical code), or generating an Error exception (useful for everything else). In the end we arrived at a compromise that combines the worst of both possibilities, which is to return NaN and generate a warning. This has caused quite a bit of confusion, because that % operation will throw a DivisionByZeroError instead. This proposal suggests to make / behave consistently and also throw DivisionByZeroError. TODO: bwoebi has arguments against this -- separate vote? |
||
| Cannot unset offset in a non-array variable | Warning | Error exception |
| Cannot use a scalar value as an array | Warning | Error exception |
| Trying to access array offset on value of type %s | Notice | Warning |
| Rationale: These diagnostics are generated when trying to use scalars as arrays. The first two occur in write contexts, the latter in read contexts. The latter was introduced in PHP 7.4 as a notice with express intention to elevate the severity in PHP 8.0. In line with the symmetrical case on objects, the write case is treated more severely here, as it usually implies data loss. | ||
| Only arrays and Traversables can be unpacked | Warning | TypeError exception |
| Invalid argument supplied for foreach() | Warning | TypeError exception |
| Rationale: These are simple type errors and should be treated as such. | ||
| Illegal offset type | Warning | TypeError exception |
| Illegal offset type in isset or empty | Warning | TypeError exception |
| Illegal offset type in unset | Warning | TypeError exception |
| Rationale: These are generated if an array or object is used as an array key. Once again this is a simple type error. | ||
| Indirect modification of overloaded element of %s has no effect | Notice | (Notice) |
| Indirect modification of overloaded property %s::$%s has no effect | Notice | (Notice) |
Rationale: These notices occur if __get() or offsetGet() return a non-reference, but are used in a write context. Because our detection of write context has false positives right now, these should remain notices until we can be sure that the diagnostic is always legitimate. |
||
| Object of class %s could not be converted to int/float/number | Notice | (Notice) |
Rationale: Comparison between objects and scalars currently works by casting the object to the appropriate type, which is why comparisons like $obj == 1 will currently also throw this notice, while they should not. Until this issue is resolved, the classification as notice should remain. |
||
| A non-numeric value encountered | Warning | (Warning) |
| A non well formed numeric value encountered | Notice | (Notice) |
| Rationale: The difference between these two warnings is whether a string is completely non-numeric, or whether it has a numeric prefix. This is a runtime issue based on the specific string value involved in an operation, which may be user-controlled. For this reason we don't promote to exceptions. | ||
| Accessing static property %s::$%s as non static | Notice | ??? |
Rationale: This notice is somewhat confusing in what it does: It is thrown when accessing $obj->staticProp but does not actually read the static property. Instead it will fall back to using the dynamic property named staticProp. There is more inconsistency in this area, in that accessing a protected static property on the object will generate an Error exception, even though it would not actually access that property. I'm not sure what to do here, both removing the notice completely and promoting it to an Error exception would be viable options to me. |
||
| Array to string conversion | Notice | Warning |
| Rationale: This is generally a bug (and the “Array” string you get is meaningless), but in many cases also not a particularly severe one. | ||
| Undefined offset: %d | Notice | (Notice) |
| Undefined index: %s | Notice | (Notice) |
| Rationale: I believe that especially in legacy code it is pretty common to ignore undefined index diagnostics rather than using explicit isset checks. I'm keeping it as a notice for that reason, but a warning might be preferable. | ||
| Resource ID#%d used as offset, casting to integer (%d) | Notice | Warning |
| Rationale: This is a meaningful operation, but should be performed with an explicit integer cast. Without that indication of intent, this is more likely a bug than not. | ||
| String offset cast occurred | Notice | Warning |
| Illegal string offset '%s' | Warning | (Warning) |
| Rationale: The former is thrown when using null/bool/float as a string offset, the latter if the string is not integral. Both of these should use the same severity. | ||
| Uninitialized string offset: %d | Notice | Warning |
| Illegal string offset: %d | Warning | (Warning) |
| Rationale: The former is used when reading an out-of-bounds string offset, the latter when writing to an out-of-bounds negative string offset (for positive offsets, the string is extended instead). The notice is promoted to a warning, as this is generally an algorithmic bug, and string offset access if far rarer than array offset access. | ||
| Cannot assign an empty string to a string offset | Warning | Error exception |
| Rationale: This indicates some kind of logic error. As this is a rare operation in the first place, throw an Error seems preferable here. | ||
| Only variables should be passed by reference | Notice | ??? |
| Only variable references should be returned by reference | Notice | ??? |
| Only variable references should be yielded by reference | Notice | ??? |
| Only variables should be assigned by reference | Notice | ??? |
| Attempting to set reference to non referenceable value | Notice | ??? |
| Parameter %d to %s%s%s() expected to be a reference, value given | Warning | ??? |
| Cannot pass by-reference argument %d of %s%s%s() by unpacking a Traversable, passing by-value instead | Warning | ??? |
| Rationale: The use of values where a reference is expected is currently wildly inconsistent, with everything from compiler errors, Error exceptions, warnings and notices being possible depending on the specific case. Generally passing a non-variable to a reference argument is a programming error, because it will not be possible to modify the passed value and the reference cannot serve its purpose. However, this is complicated by optional reference arguments (or return values for that matter), where it might be desirable to allow passing/returning a null literal. I'm not sure what the best classification here is. | ||
Backward Incompatible Changes
Conversion of noticed to warnings is fairly harmless, because both continue execution after the diagnostic has been generated. Conversion to exceptions implies that the current control flow will be aborted.
This may impact code that makes very liberal use of the error suppression operator @ or disables error reporting wholesale. The proposal does try to avoid changing notices that are more likely to be suppressed into exceptions.
Vote
Due to the number of error conditions involved here, this RFC will be voted as a single proposal, rather than voting on each individual change. If some of the proposed changes seem particularly contentious to me, I may split them off into a separate vote.
rfc/engine_warnings.1566921396.txt.gz · Last modified: 2019/08/27 15:56 by theodorejb