[dx] Reorder rule guards to bail cheaply before expensive analysis#8103
Open
TomasVotruba wants to merge 1 commit into
Open
[dx] Reorder rule guards to bail cheaply before expensive analysis#8103TomasVotruba wants to merge 1 commit into
TomasVotruba wants to merge 1 commit into
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Across 16 rules, an expensive analysis call (
getType/getNativeType,isObjectType,createFromNodeOrEmptydocblock parsing, or a subtree traversal) ran before a cheap, independent guard that could already return early. This reorders each so the cheap guard runs first — the expensive call is skipped whenever the cheap guard bails.All reorders are behavior-preserving: in each case the cheap guard does not depend on the expensive result, so moving it earlier changes only when we short-circuit, not whether.
Rules touched
Type resolution moved after a cheap name/instanceof/arg guard
Php74/ExportToReflectionFunctionRector—isName('export')+isFirstClassCallable()beforegetType($node->class)Php82/FilesystemIteratorSkipDotsRector—isset($args[1])beforeisObjectTypePhp84/AddEscapeArgumentRector— method-namein_arraybeforeisObjectTypePhp85/OrdSingleByteRector— single-charString_/Int_literal checks beforegetNativeTypePhp85/RemoveFinfoBufferContextArgRector—isName('buffer')short-circuits beforeisObjectTypein the||CodingStyle/ConsistentImplodeRector—count($args) !== 2beforegetType(also removes a latent undefined-index on malformed 0-arg calls)CodeQuality/LogicalToBooleanRector—getNativeTypeonly computed inside theinstanceof Assignbranch that uses itCodeQuality/InlineIfToExplicitIfRector—defined()FuncCall guard beforegetTypeCodeQuality/NumberCompareToMaxFuncCallRector—areIntegersCompared(2×getType) gated behind the structural pattern match, so non-comparisonBinaryOps bail freeDocblock parsing moved after a cheap guard
Php80/ClassPropertyAssignToConstructorPromotionRector— class-reflection guard beforecreateFromNodeOrEmptyTypeDeclarationDocblocks/AddReturnDocblockFromMethodCallDocblockRectorTypeDeclarationDocblocks/AddReturnDocblockForCommonObjectDenominatorRectorTypeDeclarationDocblocks/AddReturnDocblockForArrayDimAssignedObjectRector— nativereturnTypearray check before parsing the docblockTypeDeclarationDocblocks/AddReturnDocblockForJsonArrayRector— same, plus removes a duplicatecreateFromNodeOrEmptycall (parsed twice)TypeDeclarationDocblocks/AddParamArrayDocblockFromDataProviderRector— parse docblock only after a data provider is foundExpensive traversal moved after cheap structural match
DeadCode/RemoveNextSameValueConditionRector— next-statement existence/equality checks before the side-effect + cond-variable-usage traversals of the currentifTests
No behavior change, so existing fixtures are the proof. Full suites for every touched category pass:
rules-tests/{Php74,Php80,Php82,Php84,Php85,CodingStyle,CodeQuality,DeadCode,TypeDeclarationDocblocks}→ 2668 tests green. ECS + PHPStan level 8 clean.Note
A 17th candidate (
DeadCode/RemoveUselessTernaryRector) was rejected during review: itsgetNativeTyperesult is needed by an earlier check that legitimately runs before the structural guard, so reordering there would change behavior.