nikic / nikic/PHP-Parser

Shadowing of parent instance fields in derived autogenerated parser classes

Open
#772 2 comments 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

Dominant language
PHP
Stars
17.5k
Forks
1.1k
PR merge metrics
No merged PRs in 30d

Description

A generated parser class declared instance fields that shadow the base class instance fields:

https://github.com/nikic/PHP-Parser/blob/f767b9fd9f9b7c2f411ca6f28e3bdf06e66a1754/lib/PhpParser/Parser/Php5.php#L20-L27

It looks like this pattern:

<?php

class Base {
    public static function getData1() {
        $x = new static();
        return $x->data;
    }

    public function getData2() { return $this->data; }

    protected $data = ['base'];
}

class Derived extends Base {
    protected $data = ['derived'];
}

var_dump(Base::getData1()); // ['base']
var_dump(Derived::getData1()); // ['derived']

$b = new Base();
$d = new Derived();
var_dump($b->getData2()); // ['base']
var_dump($d->getData2()); // ['derived']

Which is reported by some linters as a bad code (example: https://github.com/kalessil/phpinspectionsea/blob/master/docs/architecture.md#class-overrides-a-field-of-a-parent-class)

Since Nikita has divine knowledge of the PHP internals (❤️), I would like to ask what differences we would get from these two approaches:

  • Shadowing the parent instance field to override the default initializer (the code in question above)
  • Assigning a new value inside a constructor

There might be subtle differences internally, but it looks like the derived class can't access base class instance field (parent::$field would work only for static fields), nor can the base class see overridden fields in any way.

Another question is: is it OK to adjust the parser generator code to produce a constructor that initialized fields with new values instead of doing this kind of field shadowing?

Some context: I'm trying to build a php-parser with KPHP compiler.
There are a lot of things to do in order to achieve that inside the KPHP compiler, but this code
pattern looks a little bit odd and it's not obvious whether supporting it is a good thing.

Thank you for the attention. 👐


The code above could be re-written as:

<?php

class Base {
    protected $data = ['base'];
}

class Derived extends Base {
   // No more shadowing.
   //  protected $data = ['derived'];
  public function __construct() {
    $this->data = ['derived'];
  }
}

Contributor guide

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Start with lib/PhpParser/Parser/Php5.php lines 20-27 and trace the parser generator that produces these generated classes. Compare parent-field shadowing with constructor assignment in the relevant PHP behavior, then determine whether changing the generator is compatible; done means a documented decision and, if warranted, updated generated output and generator behavior.

Written by the indexing model from the issue text.

Assessment

Tech stack
php
Domain
compilers, devtools
Issue type
Refactor
Difficulty
5/5
Estimated time
Over a week
Activity status
Stale
Clarity
Needs clarification
Newbie friendliness
25/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.