[TASK] Add phpstan check for unneeded pseudo uncertain instanceof usage
An `instanceof Type` on `Type|null` is unneeded and is to be replaced by a null-check (or modern alternatives like optional chaning or the null coalescing operator) in order to avoid narrowing code branches unnecessarily. We call them "pseudo" uncertain checks there is no need to express uncertainty regarding the type in a condition where native type declarations define a specific type *or* null: It is `null` or `!null`. Definition of a pseudo uncertain instanceof check: `$foo instanceof Bar` is fully equivalent to `$foo !== null`, when `$foo` is defined (via native PHP types) to be `Bar|null`. ⇒ `instanceof` expresses pseudo uncertainty regarding the type. From what we have seen in previous gerrit discussions, there were two reasons why instanceof was preferred over null checks although being unneeded: 1) Cognitive load for an instanceof check is perceived to be lower in contrast to negated null (not null) conditions 2) Preparatory safe-guard against type of $foo being changed at sometime later 1) Cognitive load is a subjective term and the opinions actually differ a lot. Some developers prefer narrowing instanceof conditions because they claim the desired type for a certain code branch. Some others say that's a clear signal for code that needs refactoring and perceive a high cognitive load because they do not understand why the type is unnecessarily checked if it can only be null or not null. Lets call that: "reverse cognitive load". That means, this argument basically boils down to "congitive load" (for the good "then" case: inner code block) vs "reverse cognitive load" (for the bad "else" case: outer code block) ⇒ Due to being subjective "cognitive load" is not a good argument to base a decision upon. 2) The second argument is that an instanceof ensures a method that is to be called actually exists and doesn't lead to an error – that is a "preparatory safe-guard". This is true and works, but doesn't "answer" the question, what happens if the object is not an instance of the desired type (but not null). While preparatory safe-guards against the type of variable being changed sometime later was probably a pretty good idea for code that is not statically analyzed and had no native type declarations, but such checks effectively preclude that the type must/should never change (which might not be true!) and has no chance of actually detecting when that case (type change/extension) ever happens. All advantages offered by pseudo uncertain instanceof checks are accomplished with static code analysis as well, but with the added downside that an `instanceof` hardcodes our human static code analysis result, instead of letting the static analyzer do the job. To explain that: If the type of the variable under test is actually widened (like a union type, or change to a base class), it will never be automatically detected that there is an instanceof condition that restricts the type too narrowly. It will always be valid code from static code analysis perspective. In comparison to that, static analysis on null-checked variables will report invalid method calls or assignments not allowed by the (natively defined) types and will notify in case a type change requires the code to be adapted. We gain the advantage that the code will not be forgotten to be updated to a new type. That means !== null combined with static code analysis has the same level of being a safeguard against the bad cases, while instanceof silently transforms new "good"-cases into bugs, where !== null is a transparent and secure passthrough. Actually to make an uncertain instanceof robust, an elseif branch would be needed to be somehow notified about new good-cases without silently ignoring them: if ($foo instanceof Foo) { … } elseif ($foo !== null) { throw new MustNeverHappenException(…); } In other words an unneeded pseudo uncertain instanceof check is basically like a switch construct without a default case. Just to be explicit: Of course, instanceof is fine to be used when multiples types are to be expected and handled in different code branches. That means pseudo uncertain instanceof usage instead of null-checks is an antipattern for the following reasons: * It narrow code branches for the sake of less cognitive load * The cognitive load appears to be lower, but actually future-bad cases are overseen and are never auto-detectable in future – while null-checks will resolve to static analysis errors in case the input type is *ever* widened (which uncertain `instanceof` checks try to prepare for, but actually introduce future-bugs because of missing `else` cases) * It embraces deep nesting instead of early returns via null-checks * It embraces conditions over newer and more elegant PHP techniques like optional chaing * Tries to "help" the developer by explicitly anotating the current type of the variable under test ⇒ This is a clear sign of code smell, that needs to refactored into smaller chunks and methods and type autocompletion/information can be provided by IDEs when using proper types (which this change is about) anyway * Has zero advantages over static code analysis Resolves: #102140 Releases: main, 12.4 Change-Id: I10de41e9744a814c9e24255573b5a5eaf6fb8b0f Reviewed-on: https://review.typo3.org/c/Packages/TYPO3.CMS/+/80859 Tested-by:Andreas Kienast <a.fernandez@scripting-base.de> Tested-by:
core-ci <typo3@b13.com> Tested-by:
Oliver Hader <oliver.hader@typo3.org> Reviewed-by:
Oliver Hader <oliver.hader@typo3.org> Tested-by:
Benjamin Franzke <ben@bnf.dev> Reviewed-by:
Benjamin Franzke <ben@bnf.dev> Reviewed-by:
Andreas Kienast <a.fernandez@scripting-base.de> Reviewed-by:
Nikita Hovratov <nikita.h@live.de>
Showing
- Build/phpstan/phpstan.neon 11 additions, 0 deletionsBuild/phpstan/phpstan.neon
- Build/phpstan/src/Rules/Classes/UnneededInstanceOfRule.php 84 additions, 0 deletionsBuild/phpstan/src/Rules/Classes/UnneededInstanceOfRule.php
- composer.json 2 additions, 1 deletioncomposer.json
- typo3/sysext/adminpanel/Classes/Modules/Debug/QueryInformation.php 1 addition, 1 deletion...ext/adminpanel/Classes/Modules/Debug/QueryInformation.php
- typo3/sysext/backend/Classes/Controller/AjaxLoginController.php 4 additions, 1 deletion...sysext/backend/Classes/Controller/AjaxLoginController.php
- typo3/sysext/backend/Classes/Controller/PageTsConfig/PageTsConfigActiveController.php 1 addition, 2 deletions.../Controller/PageTsConfig/PageTsConfigActiveController.php
- typo3/sysext/backend/Classes/Controller/PageTsConfig/PageTsConfigIncludesController.php 6 additions, 14 deletions...ontroller/PageTsConfig/PageTsConfigIncludesController.php
- typo3/sysext/backend/Classes/Http/RouteDispatcher.php 1 addition, 1 deletiontypo3/sysext/backend/Classes/Http/RouteDispatcher.php
- typo3/sysext/backend/Classes/Security/ContentSecurityPolicy/CspAjaxController.php 1 addition, 1 deletion...sses/Security/ContentSecurityPolicy/CspAjaxController.php
- typo3/sysext/backend/Classes/Template/Components/Buttons/DropDown/AbstractDropDownItem.php 1 addition, 3 deletions...late/Components/Buttons/DropDown/AbstractDropDownItem.php
- typo3/sysext/backend/Classes/Template/Components/Buttons/DropDownButton.php 1 addition, 3 deletions...nd/Classes/Template/Components/Buttons/DropDownButton.php
- typo3/sysext/backend/Classes/Template/Components/Buttons/GenericButton.php 1 addition, 3 deletions...end/Classes/Template/Components/Buttons/GenericButton.php
- typo3/sysext/backend/Classes/View/BackendLayoutView.php 1 addition, 4 deletionstypo3/sysext/backend/Classes/View/BackendLayoutView.php
- typo3/sysext/core/Classes/Domain/Repository/PageRepository.php 3 additions, 5 deletions.../sysext/core/Classes/Domain/Repository/PageRepository.php
- typo3/sysext/core/Classes/Error/AbstractExceptionHandler.php 1 addition, 1 deletiontypo3/sysext/core/Classes/Error/AbstractExceptionHandler.php
- typo3/sysext/core/Classes/Error/ProductionExceptionHandler.php 3 additions, 4 deletions.../sysext/core/Classes/Error/ProductionExceptionHandler.php
- typo3/sysext/core/Classes/Imaging/IconFactory.php 8 additions, 10 deletionstypo3/sysext/core/Classes/Imaging/IconFactory.php
- typo3/sysext/core/Classes/Messaging/FlashMessageQueue.php 2 additions, 4 deletionstypo3/sysext/core/Classes/Messaging/FlashMessageQueue.php
- typo3/sysext/core/Classes/Routing/PageRouter.php 1 addition, 1 deletiontypo3/sysext/core/Classes/Routing/PageRouter.php
- typo3/sysext/core/Classes/TypoScript/PageTsConfigFactory.php 1 addition, 1 deletiontypo3/sysext/core/Classes/TypoScript/PageTsConfigFactory.php
Please register or sign in to comment