[PHP] wrong code generated with empty constructor #4723

Closed
mockey opened this Issue Dec 14, 2015 · 10 comments

Projects

None yet

3 participants

@mockey
Contributor
mockey commented Dec 14, 2015

Check this:

class Test {
  static function main() {
    var t = new Test1();
    t.test();
  }
}

class Test1 {
  public function new() {}
  public function test() trace("test");
}

With an empty constructor the generator adds if(!php_Boot::$skip_constructor), which actually belongs to the constuctor I think, to the body of the test function. While this still sort of works, you get an error with dynamic function test because the dynamic method declarations in the constructor are left out, too.

This code:

class Test1 {
  public function new() {}
  public dynamic function test() trace("test");
  function test2 trace("test2");
}

generates:

public function test2() {
  if(!isset($this->test)) $this->test = array(new _hx_lambda(array(&$this), "Test1_0"), 'execute');
  if(!php_Boot::$skip_constructor) {
...

for function test2. So the code that belongs in the constructor is somehow shifted to the first real function, when the constructor is empty. The error must be somewhere here:
https://github.com/HaxeFoundation/haxe/blob/development/genphp.ml#L1304
but I cannot figure out what's going on there OCaml-wise.

@Simn Simn closed this in f95d340 Jan 1, 2016
@mockey
Contributor
mockey commented Jan 6, 2016

This is still not quite right. The code is not shifted anymore, but the code for the dynamic function is missing in the constructor when the constructor is empty, i.e.:

class Test {
  public function new() {}
  public dynamic function test() trace("test");
}

generates:

class Test {
  public function __construct() {}
  public function test() { ... }
  public $test = null;
}

if(!isset($this->test)) $this->test = array(new _hx_lambda(array(&$this), "Test1_0"), 'execute');
is missing in the constructor.

@mockey
Contributor
mockey commented Jan 6, 2016

....because ctx.constructor_block is false here:
https://github.com/HaxeFoundation/haxe/blob/development/genphp.ml#L1302
But the code for the dynamic function is here:
https://github.com/HaxeFoundation/haxe/blob/development/genphp.ml#L1311

@Simn
Member
Simn commented Jan 6, 2016

I'm confused, what does constructor_block have to do with dynamic functions?

@Simn
Member
Simn commented Jan 6, 2016

Oh I see, it generates the init there... argh.

@Simn Simn reopened this Jan 6, 2016
@Simn Simn added a commit to Simn/haxe that referenced this issue Feb 17, 2016
@Simn Simn [php] make sure to unset `constructor_block` (closes #4723) d873689
@Simn Simn modified the milestone: 3.3.0-rc1 Feb 23, 2016
@Simn Simn added the platform-php label Mar 23, 2016
@Simn Simn modified the milestone: 3.4, 3.3.0-rc1 Apr 2, 2016
@RealyUniqueName RealyUniqueName was assigned by Simn Dec 7, 2016
@RealyUniqueName
Member

This issue is not reproducible with the latest development version of Haxe.

@Simn
Member
Simn commented Dec 20, 2016

Hmm yes, this was supposed to be closed by f95d340 but for some reason didn't.

@mockey
Contributor
mockey commented Jan 10, 2017

This still seems to be broken for the old generator, also see: #4924

class Test {
  function new() {}

  dynamic function test() {
    trace("test");
  }

  static function main() {
    new Test().test();
  }
}

When the constructor is empty the init code is missing, e.g.:
if(!isset($this->test)) $this->test = ...
It only gets generated when the constructor is not empty.
Also -debug avoids the error, maybe that's why it's not caught in the unit test?

@Simn Simn reopened this Jan 10, 2017
@RealyUniqueName
Member

Yes, debug was preventing to reproduce this issue. Now it's really fixed.

@mockey
Contributor
mockey commented Jan 11, 2017

Confirmed.
Tiny flaw: there is a redundant ; now after the init line, it's like:

if(!isset($this->test)) $this->test = array(new _hx_lambda(array(&$this), "Test_0"), 'execute');
;
@RealyUniqueName
Member

Let's leave as-is for now :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment