Skip to content
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

dce should consider all members of a type consumed by an extern as used when the extern function is referenced #5889

Open
binki opened this issue Dec 19, 2016 · 10 comments
Milestone

Comments

@binki
Copy link
Contributor

binki commented Dec 19, 2016

I have written two distinct modules. One exports a function which takes an IMap<String, String> and iterates over it:

packageA/src/packageA/MapConsumer.hx:

package packageA;

import haxe.Constraints;

@:keep
@:expose
class MapConsumer {
    public static function consume(map:IMap<String, String>) {
	for (key in map.keys()) {
	    trace('${key}=>${map.get(key)}');
	}
    }
}

packageA/externs/packageA/MapConsumer.hx:

package packageA;

@:keep @:expose extern class MapConsumer
{
	static function consume(map:haxe.Constraints.IMap<String, String>) : Void;
}

My other module instantiates a StringMap and calls the function through the extern definition:

packageB/src/packageB/MapConsumerConsumer.hx:

package packageB;

import packageA.MapConsumer;

class MapConsumerConsumer {
    static function main() {
	MapConsumer.consume(['a' => '0', 'b' => '1',]);
    }
}

When compiling packageB with default settings and its classpath including packageA/externs, I get the following code generated for the JavaScript target:

// Generated by Haxe 3.4.0
(function ($hx_exports) { "use strict";
$hx_exports["packageA"] = $hx_exports["packageA"] || {};
var haxe_IMap = function() { };
var haxe_ds_StringMap = function() {
	this.h = { };
};
haxe_ds_StringMap.__interfaces__ = [haxe_IMap];
haxe_ds_StringMap.prototype = {
	setReserved: function(key,value) {
		if(this.rh == null) {
			this.rh = { };
		}
		this.rh["$" + key] = value;
	}
};
var packageB_MapConsumerConsumer = function() { };
packageB_MapConsumerConsumer.main = function() {
	var _g = new haxe_ds_StringMap();
	if(__map_reserved["a"] != null) {
		_g.setReserved("a","0");
	} else {
		_g.h["a"] = "0";
	}
	if(__map_reserved["b"] != null) {
		_g.setReserved("b","1");
	} else {
		_g.h["b"] = "1";
	}
	packageA.MapConsumer.consume(_g);
};
var __map_reserved = {}
packageB_MapConsumerConsumer.main();
})(typeof exports != "undefined" ? exports : typeof window != "undefined" ? window : typeof self != "undefined" ? self : this);

Trying to run the program (after manually assembling the emitted JavaScript as modules together) results in Failed: TypeError: values.keys is not a function being thrown by packageA.MapConsumer.consume() (in a JavaScript target).

As you can see, none of the IMap<K, V> are defined in the StringMap implementation due to dead code elimination. It would be nice if DCE could mark types that an extern definition consumes as needing to be kept if the extern is being referenced in kept code. In this case, with an extern consuming IMap<K, V>, the compiler has no knowledge of what methods the extern might call on the interface, so it should keep all of the implementations in the generated calling code.

As a workaround, I am probably going to have to write a dummy file which consumes IMap<K, V> to and mark it with @:keep to force the type to be more fully generated so that it can be passed to extern consumers. But this is difficult to maintain.

@binki
Copy link
Contributor Author

binki commented Dec 19, 2016

@binki
Copy link
Contributor Author

binki commented Dec 19, 2016

And same example with DCE off to prove that it would work if DCE were did not prune members of types referenced by used externs: http://try.haxe.org/#46428

@Simn
Copy link
Member

Simn commented Dec 19, 2016

I don't think we should do that by default because externs are usually not written for Haxe types.

@binki
Copy link
Contributor Author

binki commented Dec 19, 2016

@Simn If externs are being written for non-Haxe types, they won’t be using Haxe types like IMap<K, V> in their function signatures. So doing this by default (or at all) would not affect such externs. But if an externs definition for a system declared an interface and your local Haxe code implemented the interface but never consumed it itself, DCE would eat it even if your implementation is passed to an extern function with how it is currently. Externs document black-box code. The DCE stuff currently assumes it has access to the whole body of code, which isn’t true when externs are involved.

@ncannasse
Copy link
Member

I think that in that case you should exclude Map from your packageB and use packageA implementation instead, see what @elsassph is doing for JS code splitting.

@binki
Copy link
Contributor Author

binki commented Dec 20, 2016

Yeah, I think the best way would be to generate a module containing the whole Haxe standard library and having something like a -nostdlib option to Haxe and just compile everything against an extern variant of stdlib. I just saw some references to @elsassph’s repository today and have not had time to investigate it. At the moment I am getting more pressed for time and may have to just resort to hacks for what I’m doing.

It is also true that if one is writing code a module at a time, DCE is less and less useful. You would never write a small module with huge amounts of dead code in the first place because, with the whole “test coverage” thing, etc., dead code is likely buggy code. And the module’s public API would require you to mark most everything as “keep”. DCE is most useful when the Haxe compiler can see the whole program and all its dependencies together in a traditional setting, especially if you have multiple entry points, because removing unused members close to the entry point of the program may eliminate the need for huge portions of the stdlib itself. So, when trying to write modules, -dce no plus -nostdlib (haven’t figured out how to do this yet) might be the eventual acceptable approach.

However, with all that said, if packageA declares an interface and provides a function which accepts an argument with a type of that interface and packageB has its own implementation of the interface but only creates instances to pass to packageA, DCE will eat packageB’s implementation if the interface is `IMap<K, V>, see below my confusion. I’m still convinced that externs are to describe the border APIs of black boxes and that the moment you pass an object to a black box’s interface which accepts a particular type, the only right thing to do is mark that whole type as “keep”.

But, while playing around with the compiler, I found that the following code does not reproduce this problem as the implements IThing causes Thing and Thing.act to be emitted with -dce full under haxe-3.4rc1:

class Thing implements IThing {
    public function new() {
    }
    public function act() {
        trace('hi');
    }
}

class Test {
    static function main() {
    }
}

@:native("({consume: thing => thing.act(),})")
extern class Consumer {
    static function consume(thing:IThing):Void;
}

@:native("null")
extern interface IThing {
    function act():Void;
}

So it looks like I’m actually hitting something different and I found code which is not eliminated even though I’d expect it to be (Thing is never instantiated and without implements IThing it isn’t emitted).

I don’t understand enough of haxe’s implementation to guess why this happens. Somehow, if I implement IMap<K, V> from std, members always get pruned by dce. But if I create my own interface and implement it, the full implementation is always emitted even if I don’t reference my own interface at all—it just has to be in a loaded module.

Sorry for the rambling, but I’m confused on this. Why is IMap<K, V> special? I’m using haxe-3.4rc1/3.4.0 and the JavaScript target…

@elsassph
Copy link
Contributor

FWIW my experimentation (haxe-modular, branch automatic-splitting) is post processing a single JS output to emit multiple JS files which can be lazy-loaded and share classes. It's not one JS file by class but one main JS and several JS modules.

@binki
Copy link
Contributor Author

binki commented Dec 22, 2016

I’m trying something different, I think. I want a haxe invocation on a project to generate a single JavaScript module along with a directory tree of externs describing it. I also want to keep the stdlib out of this module and instead as its own module or set of modules so that if we have yet more modules they can share the same in-memory stdlib. I.e., I want npm-style incremental development. I posted some of these thoughts to #5831.

I still don’t understand why haxe emits different code when I implement my own interface versus IMap<K, V>. Maybe if I take a fresh look I can figure out something more. I was hoping that I could repro my claim in this issue with something other than IMap<K, V>, but I haven’t yet.

@binki
Copy link
Contributor Author

binki commented Dec 25, 2016

OK, I’m not sure what I was doing wrong earlier (actually, I suspect I was using -dce std without realizing it), but I think this is pretty convincing that DCE is pruning interface members when types implementing those interface members are being referenced by extern:

interface IThing {
    function act():Void;
}

@:native('({consume: thing => thing.act()})')
extern class NativeClass {
    public static function consume(thing:IThing):Void;
}

class Test {
    static function main() {
        NativeClass.consume(new Thing());
    }
}

class Thing implements IThing {
    public function new() {
    }

    public function act() {
        trace('hi');
    }
}

Produced by haxe -main Test -js Run.js -dce full:

// Generated by Haxe 3.4.0
(function () { "use strict";
var IThing = function() { };
var Test = function() { };
Test.main = function() {
	({consume: thing => thing.act()}).consume(new Thing());
};
var Thing = function() {
};
Thing.__interfaces__ = [IThing];
Test.main();
})();

And node Run.js yields a (trimmed) error:

TypeError: thing.act is not a function
    at Object.consume.thing [as consume] (c:\Users\ohnob\repos\haxe-extern\Run.js:6:28)
    at Function.Test.main (c:\Users\ohnob\repos\haxe-extern\Run.js:6:36)

Since new Thing() is being passed to an extern (black box) which declares it consumes IThing, then the implementation of new Thing() needs to be marked “needed” by DCE because Haxe cannot know what subset of IThing the extern code demands. I know it is pretty obvious in my example code what the external code requires because it’s all written right in the @:native(). But if consuming a real external library which expects to consume some interface, that can only work when DCE is on if Haxe actually treats externs like externs ;-).

Does this make sense?

@binki
Copy link
Contributor Author

binki commented Dec 25, 2016

P.S., I did verify that my code runs fine with -dce std, so it’s not like the node Run.js error comes from me making a mistake in my @:native or something ;-).

@Simn Simn modified the milestone: 4.0 Jan 9, 2017
@Simn Simn modified the milestones: Release 4.0, Design Apr 17, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants