Enum map does not play nice with ValueType

[lptr]

This code works as expected:

import Type;

class Test2 {
    static function main(){
        var map:Map<ValueType,String> = new Map();
        map.set(TInt, "TInt");
        map.set(TClass(String), "TClass(String)");
        map.set(TClass(Color), "TClass(Color)");
        map.set(TClass(Length), "TClass(Length)");
        map.set(TFloat, "TFloat");

        trace("TInt: " + map.get(TInt));
        trace("TFloat: " + map.get(TFloat));
        trace("TClass(String): " + map.get(TClass(String)));
        trace("TClass(Color): " + map.get(TClass(Color)));
        trace("TClass(Length): " + map.get(TClass(Length)));
    }
}

class Color {
    public function clone() {
        return this;
    }
}

class Length {
    public function clone() {
        return this;
    }
}

However, if I move map.set(TClass(Color), "TClass(Color)"); one line up, the code does not find TClass(Color) anymore:

import Type;

class Test2 {
    static function main(){
        var map:Map<ValueType,String> = new Map();
        map.set(TInt, "TInt");
        map.set(TClass(Color), "TClass(Color)");  // <-- moved one line higher
        map.set(TClass(String), "TClass(String)");
        map.set(TClass(Length), "TClass(Length)");
        map.set(TFloat, "TFloat");

        trace("TInt: " + map.get(TInt));
        trace("TFloat: " + map.get(TFloat));
        trace("TClass(String): " + map.get(TClass(String)));
        trace("TClass(Color): " + map.get(TClass(Color)));
        trace("TClass(Length): " + map.get(TClass(Length)));
    }
}

class Color {
    public function clone() {
        return this;
    }
}

class Length {
    public function clone() {
        return this;
    }
}

This prints this on the console:

[Log] TInt: TInt (run, line 37)
[Log] TFloat: TFloat (run, line 38)
[Log] TClass(String): TClass(String) (run, line 39)
[Log] TClass(Color): null (run, line 45)
[Log] TClass(Length): TClass(Length) (run, line 51)

Essentially, not finding TClass(Color) anymore.

[lptr]

I'm using Haxe 3.0.1 and compile to JavaScript.

[Simn]

I suppose our compare function isn't really valid when it comes to instances or Class<T>. We end up using Reflect.compare on the enum constructor arguments and that's unspecified for anything but numeric types and String.

I don't know what we can do about this in the context of a tree implementation.

[lptr]

I don't know the implementation details, but wouldn't it be possible to have a working compare for enums?

[Simn]

The problem is that enums can have arbitrary arguments and things can get really messy from there. We don't define how to instances compare, which naturally extends to enums which have instances (or things like Class<T>) as arguments.

[ncannasse]

Seems like something is wrong with JS way of comparing Class (which are created with functions), which makes it unsuitable for such sorting. You should consider using the class name instead.

Color = function() {}
function () {}
Length = function() {}
function () {}
Color > Length;
false
Color < Length;
false
Color >= Length;
true
Color <= Length;
true
Color == Length;
false

[ncannasse]

Another way would be to be able to generate at compile time compare method for enums (and other values) which would deal with Class in a specific way (adding object id as haxe.ds.ObjectMap does)

[frabbit]

ideal use case for compiler generated type classes ;)

[sledorze]

@frabbit you name it! :)

[lptr]

I'm not sure I understand the problem. So enums have zero or more parameters. We are fine with zero-parameter enums, because they are inherently orderable.

If the parameters themselves are orderable too, then a lexicographic ordering should be simple for enum values with parameters, too. If they are not orderable, the compiler could give an error.

Would this /solve the problem?

[ncannasse]

@lptr the problem comes from Class not being comparable in JS runtime. That's a quite narrow problem, we use propertly orderable sort, but Class comparison operators are simply broken in JS. It will work with any other value.

[lptr]

Thanks for the clarification. Are they going to be fixed?

[ncannasse]

@lptr fixing it would slow down the whole system since it would require to check if every value is a function and then use a specific comparison function. A possibility would be to either generate at compile-time a type-specific comparison function, or to be able to switch the comparison function if the types contain functions in JS. I'm not very happy with either of the choices, maybe @Simn will have other ideas.

In the meanwhile you should be able to use the following class in replacement:

class FixedEnumValueMap<K:EnumValue, V> extends haxe.ds.EnumValueMap<K,V> {
    static var UID = 0;
    #if js
    inline function getId(v:Dynamic) : Int {
        return untyped v.__id__ || (v.__id__ == ++UID);
    }
    override function compareArg(v1:Dynamic,v2:Dynamic ) : Int {
        if( Reflect.isFunction(v1) || Reflect.isFunction(v2) )
              return v1 == null ? -1 : (v2 == null ? 1 : getId(v1) - getId(v2));
        return super.compareArg(v1,v2);
    }
    #end
}

[lptr]

Thanks. This indeed looks slower. What would be a way to extract quickly something that I can use as a key instead of ValueType?

[ncannasse]

@lptr what kind of values do you want to reference by type? any value? a specific set? (eg only objects instances)

[lptr]

Any value indeed. What I use now is a Map<ValueType, Dynamic>, and put stuff in there ranging from Ints to classes.

[ncannasse]

@lptr I guess you will have to map every ValueType to a custom ValueType2 that replaces Class value by classes unique ids as shown in my getId example. Then use ValueType2 as key. Sorry for not providing a better solution but as you can understand it's quite a JS-specific issue that were not aware of before.

[Simn]

I don't really know what to do with this issue. I kind of regret adding EnumValueMap because dealing with arbitrary recursive types as keys is a nightmare.

Given that we have no map which accepts Class or Enum as key, it's fair to say that this limitation extends to enums. It's sad that this means we cannot use ValueType as a key though.

[waneck]

It seems to me that saying that dealing with this correctly would slow down all js comparisons is an overstatement. We could easily modify Reflect.compare and only use Reflect.compare if the types are Dynamic; if the types are Class<>, we use a custom compare function as well. Also, we could try overriding valueOf from Class types and see if it works then.

[Simn]

This is going into the JS internals, so I'll assign this to Nicolas for now. For what it's worth, Caue's idea makes sense to me.

[ncannasse]

@waneck that would break native/fast/unsafe comparison when comparing two Dynamics. There's no Class type in JS that we can change, and we have forbidden ourselves from overriding native JS behaviors to prevent issues with 3rd party libraries.

It seems better to me to say that Class in JS is not comparable. We might want to add some documentation about comparisons so I'm assigning to @Simn until it's closed.

[waneck]

Actually there's a real problem with objects and comparisons on js:

var x = {};
var y = {};
x == y; // false
x < y; //false
x > y; //false
x >= y; //true!?
x <= y; //true!?

So I guess this is something we can/should deal with when comparing Dynamics

[Simn]

We now disallow using anything involving Class<T> as a key type to Map on the JS target.

[romamik]

Actually there's a real problem with objects and comparisons on js:

var x = {};
var y = {};
x == y; // false
x < y; //false
x > y; //false
x >= y; //true!?
x <= y; //true!?

So I guess this is something we can/should deal with when comparing Dynamics

Yes, EnumValueMap does not work correctly if key has object parameter. This test fails on js, but runs fine on cpp. I was going to report this as new issue but found this issue. I think it should be reopened.

class FooParam {
	public function new() {
	}
}

enum EFoo {
	Foo(a: FooParam);
}

class Main {

	static function assert(v: Bool, m: String) {
		if (!v) {
			throw m;
		}
	}

	static function main() {

		var map: Map<EFoo, Int> = [];
		var keys: Array<EFoo> = [for (i in 0...100) EFoo.Foo(new FooParam())];
		for (i in 0...keys.length) {
			map[keys[i]] = i;
		}
		for (i in 0...keys.length) {
			assert(map[keys[i]] == i, 'map[keys[i]] == i, i=$i');
		}
	}
}

[RealyUniqueName]

Looking at EnumValueMap I don't understand why does it extend BalancedTree and use Reflect.compare. We have Type.enumEq, which works perfectly fine for any enum values. If anything EnumValueMap should have target-specific implementations like ObjectMap instead of a generic BalancedTree.

[back2dos]

We want EnumValueMap to satisfy map[Some(42)] == map[Some(42)], which is why ObjectMap won't do the trick. And so we're left with using a tree, to not have everything run at linear cost and more.

[RealyUniqueName]

Type.enumEq(Some(42), Some(42)) emits true. And I didn't mean to use the same implementations ObjectMap use. I meant implementations should be platform specific. Yes, probably at some map size it would be slower. But it would still be correct and it would work for any EnumValue.

[zommerfelds]

I'm not sure if this issue is unique to JS. I was trying to build a Map<Class<T>, Foo> and tried to use ValueType as an alternative to Class<T>. It could be that two different ValueType.TClass(type) instances were not being compared to equal and I inserted two elements with the same type.

This was on HashLink. If you are interested I could investigate more.

[zommerfelds]

Sorry, it turns out that the issue is I was using Anonymous Structures as the Map key (as a pair class), but this doesn't work as two equal copies of a structure don't compare to be the same key in ObjectMap. Ignore my comment above.

[RealyUniqueName]

I'd like to resolve this issue by merging #9670 Any objections?

[yz-xlame]

Have we made any progress on this matter