New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Reflect.field on public static inline return null #6630

Open
aliokan opened this Issue Oct 2, 2017 · 23 comments

Comments

Projects
None yet
7 participants
@aliokan
Contributor

aliokan commented Oct 2, 2017

With the nightly build e477477 (2017-10-01 21:32);
If I want resolve an public static inline with Reflect.field, I get null.
Without inline is working.

class Test {

  public static inline var TEST="hehe";
  
  public static function main() {
    trace("Haxe is great!");
    var classReference : Class<Dynamic> = Type.resolveClass( "Test" );
    trace("Test.TEST: " + Test.TEST);
    trace(Reflect.field(Test, "TEST"));
  }
}

haxe -main Test.hx -php test && php test/index.php

Related to HaxeFoundation/haxe#6583
hexMachina failing test : https://travis-ci.org/DoclerLabs/hexCore/jobs/282173307#L1135

cc @RealyUniqueName

@aliokan

This comment has been minimized.

Show comment
Hide comment
@aliokan

aliokan Oct 2, 2017

Contributor

Working correctly on haxe 3.4.2

Contributor

aliokan commented Oct 2, 2017

Working correctly on haxe 3.4.2

@RealyUniqueName

This comment has been minimized.

Show comment
Hide comment
@RealyUniqueName

RealyUniqueName Oct 2, 2017

Member

Nothing changed in genphp regarding static vars. Maybe inlined vars are now gone before compiled code get to generator.

Member

RealyUniqueName commented Oct 2, 2017

Nothing changed in genphp regarding static vars. Maybe inlined vars are now gone before compiled code get to generator.

@aliokan

This comment has been minimized.

Show comment
Hide comment
@aliokan

aliokan Oct 2, 2017

Contributor

Maybe Reflect::field(Boot::getClass(Test::class), "TEST") do not work with php const :
With nightly and inline

const TEST = "hehe";

Without inline

static public $TEST = "hehe";

And with 3.4.2, with and without inline

static $TEST = "hehe";

the rest of generated code looks similar.

Contributor

aliokan commented Oct 2, 2017

Maybe Reflect::field(Boot::getClass(Test::class), "TEST") do not work with php const :
With nightly and inline

const TEST = "hehe";

Without inline

static public $TEST = "hehe";

And with 3.4.2, with and without inline

static $TEST = "hehe";

the rest of generated code looks similar.

@RealyUniqueName

This comment has been minimized.

Show comment
Hide comment
@RealyUniqueName

RealyUniqueName Oct 2, 2017

Member

It looks like static inline var is now cut out by dce. Disable dce or add @:keep and it will be generated.
Also since it's supposed to be inlined, i think it's not valid to rely on existance of this field at runtime.
It's generated as class const to improve interop with 3rd-party php code.

Member

RealyUniqueName commented Oct 2, 2017

It looks like static inline var is now cut out by dce. Disable dce or add @:keep and it will be generated.
Also since it's supposed to be inlined, i think it's not valid to rely on existance of this field at runtime.
It's generated as class const to improve interop with 3rd-party php code.

@aliokan

This comment has been minimized.

Show comment
Hide comment
@aliokan

aliokan Oct 3, 2017

Contributor

I don't get the point about DCE, because the const is in the generated PHP. By the way, the problem still with @:keep or haxe -main Test.hx -php test -dce no && php test/index.php.

Test.hx:6: Haxe is great!
Test.hx:8: Test.TEST: hehe
Test.hx:9: null

The behavior is no the same between Haxe 3.4.2 and Haxe 4.

Contributor

aliokan commented Oct 3, 2017

I don't get the point about DCE, because the const is in the generated PHP. By the way, the problem still with @:keep or haxe -main Test.hx -php test -dce no && php test/index.php.

Test.hx:6: Haxe is great!
Test.hx:8: Test.TEST: hehe
Test.hx:9: null

The behavior is no the same between Haxe 3.4.2 and Haxe 4.

@RealyUniqueName

This comment has been minimized.

Show comment
Hide comment
@RealyUniqueName

RealyUniqueName Oct 3, 2017

Member

The point with DCE is that genphp can't generate something, which does not exist when compiled code comes to generator.
I checked 3.4.2 and static inline var did not survive DCE. Here is an example for js, but it's the same for php: https://try.haxe.org/#EEf1e
I doubt it can be considered a bug. cc @Simn

As for Reflect.field() i now think you're right. This is a bug.

Member

RealyUniqueName commented Oct 3, 2017

The point with DCE is that genphp can't generate something, which does not exist when compiled code comes to generator.
I checked 3.4.2 and static inline var did not survive DCE. Here is an example for js, but it's the same for php: https://try.haxe.org/#EEf1e
I doubt it can be considered a bug. cc @Simn

As for Reflect.field() i now think you're right. This is a bug.

@aliokan

This comment has been minimized.

Show comment
Hide comment
@aliokan

aliokan Oct 3, 2017

Contributor

If you use DCE it makes sence, but here is not the case, and const is generated, but not accessible by Reflect.field.
Bellow the PHP generated from the example :

<?php
/**
 * Generated by Haxe 4.0.0 (git build development @ e477477)
 */

use \php\Boot;
use \haxe\Log;
use \php\_Boot\HxAnon;

class Test {
	/**
	 * @var string
	 */
	const TEST = "hehe";


	/**
	 * @return void
	 */
	static public function main () {
		#Test.hx:6: characters 5-10
		(Log::$trace)("Haxe is great!", new HxAnon([
			"fileName" => "Test.hx",
			"lineNumber" => 6,
			"className" => "Test",
			"methodName" => "main",
		]));
		#Test.hx:7: characters 5-71
		$classReference = \Type::resolveClass("Test");
		#Test.hx:8: characters 5-10
		(Log::$trace)("Test.TEST: " . "hehe", new HxAnon([
			"fileName" => "Test.hx",
			"lineNumber" => 8,
			"className" => "Test",
			"methodName" => "main",
		]));
		#Test.hx:9: characters 5-10
		(Log::$trace)(\Reflect::field(Boot::getClass(Test::class), "TEST"), new HxAnon([
			"fileName" => "Test.hx",
			"lineNumber" => 9,
			"className" => "Test",
			"methodName" => "main",
		]));
	}


	/**
	 * @return void
	 */
	public function __construct () {
	}
}

Boot::registerClass(Test::class, 'Test');
Contributor

aliokan commented Oct 3, 2017

If you use DCE it makes sence, but here is not the case, and const is generated, but not accessible by Reflect.field.
Bellow the PHP generated from the example :

<?php
/**
 * Generated by Haxe 4.0.0 (git build development @ e477477)
 */

use \php\Boot;
use \haxe\Log;
use \php\_Boot\HxAnon;

class Test {
	/**
	 * @var string
	 */
	const TEST = "hehe";


	/**
	 * @return void
	 */
	static public function main () {
		#Test.hx:6: characters 5-10
		(Log::$trace)("Haxe is great!", new HxAnon([
			"fileName" => "Test.hx",
			"lineNumber" => 6,
			"className" => "Test",
			"methodName" => "main",
		]));
		#Test.hx:7: characters 5-71
		$classReference = \Type::resolveClass("Test");
		#Test.hx:8: characters 5-10
		(Log::$trace)("Test.TEST: " . "hehe", new HxAnon([
			"fileName" => "Test.hx",
			"lineNumber" => 8,
			"className" => "Test",
			"methodName" => "main",
		]));
		#Test.hx:9: characters 5-10
		(Log::$trace)(\Reflect::field(Boot::getClass(Test::class), "TEST"), new HxAnon([
			"fileName" => "Test.hx",
			"lineNumber" => 9,
			"className" => "Test",
			"methodName" => "main",
		]));
	}


	/**
	 * @return void
	 */
	public function __construct () {
	}
}

Boot::registerClass(Test::class, 'Test');
@andyli

This comment has been minimized.

Show comment
Hide comment
@andyli

andyli Oct 3, 2017

Member

The test failed for the cpp target.

Command: /home/travis/build/HaxeFoundation/haxe/tests/unit/bin/cpp/TestMain-debug []
TestMain.hx:38: Generated at: 2017-10-03 06:47:00
TestMain.hx:40: START
Issue6630.hx:9: null should be hello
Issue6630.hx:10: null should be hello
Test.hx:222: DONE [7226 tests]
Test.hx:223: SUCCESS: false
Member

andyli commented Oct 3, 2017

The test failed for the cpp target.

Command: /home/travis/build/HaxeFoundation/haxe/tests/unit/bin/cpp/TestMain-debug []
TestMain.hx:38: Generated at: 2017-10-03 06:47:00
TestMain.hx:40: START
Issue6630.hx:9: null should be hello
Issue6630.hx:10: null should be hello
Test.hx:222: DONE [7226 tests]
Test.hx:223: SUCCESS: false

@andyli andyli reopened this Oct 3, 2017

@RealyUniqueName

This comment has been minimized.

Show comment
Hide comment
@RealyUniqueName

RealyUniqueName Oct 3, 2017

Member

Maybe open separate issue for cpp?

Member

RealyUniqueName commented Oct 3, 2017

Maybe open separate issue for cpp?

@andyli

This comment has been minimized.

Show comment
Hide comment
@andyli

andyli Oct 3, 2017

Member

Feel free to reassign to hugh or open a separate issue. I merely reopen this because the failed test case is from here. :)

Member

andyli commented Oct 3, 2017

Feel free to reassign to hugh or open a separate issue. I merely reopen this because the failed test case is from here. :)

@andyli andyli changed the title from [php] Reflect.field on public static inline return null to Reflect.field on public static inline return null Oct 5, 2017

andyli added a commit to andyli/haxe that referenced this issue Oct 5, 2017

@hughsando

This comment has been minimized.

Show comment
Hide comment
@hughsando

hughsando Oct 5, 2017

Member

There could be some code-size issues making inline-variables reflective.
If we are going have the option of "final", then I would suggest that no reflection is the correct optimization & behaviour for "inline", since "final" would mean reflective/read-only/use in "case", so there would be some value in having both.

Member

hughsando commented Oct 5, 2017

There could be some code-size issues making inline-variables reflective.
If we are going have the option of "final", then I would suggest that no reflection is the correct optimization & behaviour for "inline", since "final" would mean reflective/read-only/use in "case", so there would be some value in having both.

andyli added a commit that referenced this issue Oct 5, 2017

@Simn

This comment has been minimized.

Show comment
Hide comment
@Simn

Simn Oct 10, 2017

Member

I agree anything inline should not be reflective. These two concepts just don't mix.

Member

Simn commented Oct 10, 2017

I agree anything inline should not be reflective. These two concepts just don't mix.

@RealyUniqueName

This comment has been minimized.

Show comment
Hide comment
@RealyUniqueName

RealyUniqueName Oct 10, 2017

Member

It's still nice to be able to generate such fields. E.g. if i use haxe generated php code in "native" php code.

Member

RealyUniqueName commented Oct 10, 2017

It's still nice to be able to generate such fields. E.g. if i use haxe generated php code in "native" php code.

@RealyUniqueName

This comment has been minimized.

Show comment
Hide comment
@RealyUniqueName

RealyUniqueName Oct 10, 2017

Member

Clarifying docs for Reflect should be enough.

Member

RealyUniqueName commented Oct 10, 2017

Clarifying docs for Reflect should be enough.

@andyli

This comment has been minimized.

Show comment
Hide comment
@andyli

andyli Oct 10, 2017

Member
Member

andyli commented Oct 10, 2017

@Simn

This comment has been minimized.

Show comment
Hide comment
@Simn

Simn Oct 10, 2017

Member

I think "being reflective" is one level above "being generated". But that probably depends on the actual target.

Member

Simn commented Oct 10, 2017

I think "being reflective" is one level above "being generated". But that probably depends on the actual target.

@back2dos

This comment has been minimized.

Show comment
Hide comment
@back2dos

back2dos Oct 10, 2017

Member

Can we remove Reflect in Haxe 4? :P

Member

back2dos commented Oct 10, 2017

Can we remove Reflect in Haxe 4? :P

@ncannasse

This comment has been minimized.

Show comment
Hide comment
@ncannasse

ncannasse Oct 11, 2017

Member
Member

ncannasse commented Oct 11, 2017

@ncannasse

This comment has been minimized.

Show comment
Hide comment
@ncannasse

ncannasse Oct 11, 2017

Member
Member

ncannasse commented Oct 11, 2017

@Simn

This comment has been minimized.

Show comment
Hide comment
@Simn

Simn Oct 11, 2017

Member

What about @:extern? The semantics of that aren't really clear to me because we basically use it as @:forceInline.

Member

Simn commented Oct 11, 2017

What about @:extern? The semantics of that aren't really clear to me because we basically use it as @:forceInline.

@ncannasse

This comment has been minimized.

Show comment
Hide comment
@ncannasse

ncannasse Oct 11, 2017

Member

@:extern means no generation , like if the class was extern (but it's per field/class)

Member

ncannasse commented Oct 11, 2017

@:extern means no generation , like if the class was extern (but it's per field/class)

@ncannasse

This comment has been minimized.

Show comment
Hide comment
@ncannasse

ncannasse Oct 11, 2017

Member

and it implies forceInline because else it would fail at runtime

Member

ncannasse commented Oct 11, 2017

and it implies forceInline because else it would fail at runtime

@aliokan

This comment has been minimized.

Show comment
Hide comment
@aliokan

aliokan Mar 19, 2018

Contributor

I still get the issue with 4.0.0-preview.3 on cpp.

class Main {
  
  public static inline var TEST="hehe";

  public static function main() {
    trace("Haxe is great!");
    var classReference : Class<Dynamic> = Type.resolveClass( "Main" );
    trace("Main.TEST: " + Main.TEST);
    trace(Reflect.field(Main, "TEST"));
  }
}
LUX153:Desktop laurent$ haxe -main Main.hx -cpp test && test/Main
haxelib run hxcpp Build.xml haxe -Dhaxe3="1" -Dhaxe_ver="4.000" -Dhxcpp_api_level="332" -Dsource-header="Generated by Haxe 4.0.0 (git build development @ 3018ab1)" -Dstatic="1" -I"" -I"/Users/laurent/haxe/versions/4.0.0-preview.3/std/cpp/_std/" -I"/Users/laurent/haxe/versions/4.0.0-preview.3/std/"
Main.hx:6: Haxe is great!
Main.hx:8: Main.TEST: hehe
Main.hx:9: null
Contributor

aliokan commented Mar 19, 2018

I still get the issue with 4.0.0-preview.3 on cpp.

class Main {
  
  public static inline var TEST="hehe";

  public static function main() {
    trace("Haxe is great!");
    var classReference : Class<Dynamic> = Type.resolveClass( "Main" );
    trace("Main.TEST: " + Main.TEST);
    trace(Reflect.field(Main, "TEST"));
  }
}
LUX153:Desktop laurent$ haxe -main Main.hx -cpp test && test/Main
haxelib run hxcpp Build.xml haxe -Dhaxe3="1" -Dhaxe_ver="4.000" -Dhxcpp_api_level="332" -Dsource-header="Generated by Haxe 4.0.0 (git build development @ 3018ab1)" -Dstatic="1" -I"" -I"/Users/laurent/haxe/versions/4.0.0-preview.3/std/cpp/_std/" -I"/Users/laurent/haxe/versions/4.0.0-preview.3/std/"
Main.hx:6: Haxe is great!
Main.hx:8: Main.TEST: hehe
Main.hx:9: null

@Simn Simn added this to the Design milestone Apr 17, 2018

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