[ndench]

I'm aware of the discussion in #8373 with the conclusion being that adding types to constants that could have inheritance applied will create a risky rule.

However, many codebases will have sufficient static analysis and test coverage to counteract this. We could easily support inheritance as a non-default configuration option. This would mean the rule is non-risky by default and provide users the ability to opt in to the risky part.

[samsonasik]

I prefer more verbose config, like allow_has_child, then, add check FamilyRelationsAnalyzer->getChildrenOfClassReflection() that collect children on the paths() provided in rector.php config, and check if it has constant:

// resolve current class Reflection ....
$classReflection = $this->reflectionResolver->resolveClassReflection($node);
if (! $classReflection instanceof \PHPStan\Reflection\ClassReflection)
    return null;
}

// verify has children with constant
if ($this->allow_has_child) {
    $childClasses = $this->familyRelationsAnalyzer->getChildrenOfClassReflection($classReflection);
    $constName = $this->getName($classConst->name);
    foreach ($childClasses as $childClass) {
        if ($childClass->hasConstant($constName)) {
             continue 2; // don't change, continue to inner loop...
        }
    }
}

Above is pseudo code, but I think you have the idea ;)

[ndench]

Thanks for the quick feedback @samsonasik!

I'm not entirely sure what you're getting at. Do you want it to only type the constant if a child class does not define the same constant?

[samsonasik]

Yes, that means constant overridden, which will make fatal error. After some thinking:

$childClass->hasConstant($constName);

seems will be always true on public or protected constant as it derived from parent, so I think you need to get real Class_ via AstResolver

foreach ($childClasses as $childClass) {
        $classLike = $this->astResolver->resolveClassFromClassReflection($childClass);
        if ($childClassLike instanceof \PhpParser\Node\Stmt\ClassLike) {
            continue;
        }
        
        foreach ($childClassLike->getConstants() as $constant) {
             foreach ($constant->const as $childConst) {
                    if ($this->nodeComparator->areNodesEquals($childConst, $constNode)) {
                          continue 3; // don't change, continue to inner loop...
                    }
             } 
        }
    }

When allow_has_child enabled:

• If child has constant, but not exists, it should be allowed when constant names are different, see https://3v4l.org/HcqRB
• if child has constant, and equal, it should not be allowed, even value type is equal, see
  - https://3v4l.org/SUXMa
  - https://3v4l.org/rFOhl

For note: the allow_has_child should be configurable with default false.

---

On personal note, I prefer the original behaviour for this.

[samsonasik]

Another way that might work is by use its ClassConstantReflection object itself, you can add fixture test with both parent and child in same fixture file, then you can check something like this:

foreach ($childClasses as $childClass) {
        if (! $childClass->hasConstant($constName)) {
            continue;
        }
          
        $constantChild =   $childClass->getConstant($constName);
        // debug here, check whether it own by child class or only deriving...
        if ($constantChild->getFileName() !== $classReflection->getFileName()) {
            continue 2; // don't change, continue to inner loop...
      } 
    }

[ndench]

Thanks for the help @samsonasik, I've implemented these changes now.

It will now only add the type if:

• there are no child classes
• the child classes do not override the constant

[samsonasik]

Thank you @ndench, I will check when I have a chance, and verify the possible improvement if needed.

This may not be our priority for now, so this may be here for a while for some thinking and/or discussion with @TomasVotruba in our next meeting, so you can create your custom rule for your needs for now.

[samsonasik]

@ndench FYI, the children class detection maybe dropped by PR:

• [Bc Break] [Alternative] Drop children class detection #5995

due to performance regression, so this feature won't work ...

[TomasVotruba]

Just looking into this...

[samsonasik]

the method removed at PR:

• [Bc Break] [Alternative] Drop children class detection #5995

so this change no longer valid,

Please rebase. You may can revert to original ignore_inheritance config ;)

[TomasVotruba]

First, thank you for your work on this PR.

Rector has currently access only to classes it has processed, so there is no safe way to decide if any class has child.
See rectorphp/rector#8439 for better description.

Another approach that allowed this had performance issues, rectorphp/rector#8638 (comment), so we had to remove it.

Saying that, we cannot safely detect child classes. It's known drawback based on parallel run, that we use here.
Your proposed feature is not possible to deliver now in reliable and performant way. Closing then.

Also, such situations would be better solved PHPStan. Having a rule to check type consistency across child + parent classes 👍

[github-actions]

This pull request has been automatically locked because it has been closed for 150 days. Please open a new PR if you want to continue the work.