Array.join() and -dce full issue

[sh-dave]

https://try.haxe.org/#0Dc93

class Foo {
    public function new() {        
    }

    public function toString()
        return Std.string(Std.random(100) + 100);
}

class Test {
    static function main() {
        // trace(Std.string(new Foo()));        
        var foos: Array<Foo> = [new Foo(), new Foo(), new Foo()];
        trace(foos.join('/'));        
    }
}

Results in

[object Object]/[object Object]/[object Object]

If i uncomment the first trace trace(Std.string(new Foo())); i get the expected output (of some random numbers).

170
182/170/178

Is this expected behavior with DCE, I certainly was surprised by it?

[RealyUniqueName]

This happens because your toString() is not used directly in your code. Std.string(new Foo()) tells compiler that you will stringify Foo and toString() should be kept. Probably compiler should keep toString() methods of the types used in "joined" arrays. Meanwhile you can add @:keep meta to workaround this issue: https://try.haxe.org/#0AdC1

[nadako]

Probably compiler should keep toString() methods of the types used in "joined" arrays.

Since toString is a kind of "known magic method name", maybe we should have some generic way to mark which methods makes use of toString at run-time with some metadata.

[sh-dave]

That's what i actually expected would happen already, as it's mentioned in the docs. Array.join used Std.string, and that will call toString() for class instances.

[ncannasse]

@nadako : we have that already for Std.string, it was supposed to work for trace() too but maybe some other change disabled it (related to console.log for JS?)

On Thu, Feb 8, 2018 at 2:00 PM, David Klein notifications@github.com wrote:

That's what i actually expected would happen already, as it's mentioned in the docs. Array.join used Std.string, and that will call toString() for class instances.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/HaxeFoundation/haxe/issues/6841#issuecomment-364104531, or mute the thread https://github.com/notifications/unsubscribe-auth/AA-bwGsR7jQReRtTEIUMIvRxfRT5oULbks5tSu_lgaJpZM4R-QyD .

[abeaumont]

I guess you're referring to this code:

https://github.com/HaxeFoundation/haxe/blob/a27f9c5a13aa7b4c164dfe01f82721b5f450219e/src/optimization/dce.ml#L506-L527

This handles both Std.string and haxe.Log.trace code, but it doesn't seem to cover the reported case (that is, calling the Array.join method.

I've tried to solve it adding a new case for Array.join, such as the following:

    | TCall ({eexpr = TField({eexpr = TLocal ({v_type = TInst({cl_path = ([], "Array")}, params)})}, FInstance({cl_path = ([], "Array")}, _, {cf_name="join"}))} as ef, args) ->
        List.iter (fun t -> to_string dce t) params;
        expr dce ef;
        List.iter (expr dce) args;

This seems to work but the generated output is much more verbose: it produces the same code as with -dce std, so I'm probably doing something wrong, please let me know if you find my mistake.

This should (eventually) fix the Array.join problem, but there may be other similar cases, so it may be a better idea to use some generic metadata instead.

[nadako]

What I was thinking is something like:

extern class Array<T> {
  @:usesToString(T)
  function join():String;
}

class Std {
  function string(@:usesToString v:Dynamic):String;
}

This meta would be handled by the DCE, propagating through type parameters to keep toString` on the types that are known to be affected. Not sure though how hard would it be to implement within current DCE architecture.

[abeaumont]

It looks good and I think it should be doable with current DCE architecture, instead of looking at some instance or static methods by name, check them for this new metadata. If we want to go this way I can try to implement it. What do you think @nadako @ncannasse?

[nadako]

Well I would try it :)

[abeaumont]

Looking deeper at this, it doesn't seem that metadata can be attached to method parameters... adding support for metadata in method parameters only for this case may be a bit overkill, I'd like to know what do you think about it. In the meantime, I have fixed the Array.join problem without the touching metadatas at https://github.com/HaxeFoundation/haxe/pull/6868.

[RealyUniqueName]

I like the idea of metadata support for function arguments. That will allow additional macro magic.

[ncannasse]

The original issue is fixed but I think we should check that other cases are covered, such as:

"" + foos foos.toString()

[ncannasse]

Also we need something that supports Map while we're at it:

class Main {

    static function main() {
        var m = [55 => new A(5), 66 => new A(6)];
        trace(m);
    }
}

class A {

    var x : Int;

    public function new(x:Int) {
        this.x = x;
    }

    public function toString() {
        return "A#"+x;
    }

}

[abeaumont]

@ncannasse that's correct, the point is whether is better to go check and fix all the cases in the dce or add support to mark those cases with metadata, which would probably need metadata support in method parameters, as commented above.

[abeaumont]

I've implemented the metadata support at #6894. It varies slightly from the initial proposal from @nadako, it reuses the existing but unused @:toString metadata and doesn't require metadata support in parameters. The original example with this syntax would be:

extern class Array<T> {
  @:toString(T)
  function join():String;
}

class Std {
  @:toString(v) // or just @:toString that defaults to all the parameters
  function string(v:Dynamic):String;
}

I think the current implementation supports everything that was previously supported (unless I missed something), but it's still not complete. It does support code like trace([1 => new A(5), 2 => new A(6)].toString()) but not just trace([1 => new A(5), 2 => new A(6)]) (which wasn't supported before either). This happens because only direct dependencies are being handled at the moment and transitive relations are not considered, that is, when trace is called on a Map, it knows that it has to preserve Map.to_string, but it doesn't go recursively checking which methods Map.to_string itself should preserve.