back2dos / tinkerbell

MIT License
83 stars 8 forks source link

toComplex infinite recursion with recursive constraints? #55

Open MetaChrome opened 10 years ago

MetaChrome commented 10 years ago

It would appear that toComplex falls into infinite recursion with the following patterns:

s:/_flash/shared_haxe/tinkerbell_h3/src/tink/macro/tools/TypeTools.hx:224: characters 63-91 : Called from s:/_flash/shared_haxe/tinkerbell_h3/src/tink/macro/tools/TypeTools.hx:203: characters 19-37 : Called from s:/_flash/shared_haxe/tinkerbell_h3/src/tink/macro/tools/TypeTools.hx:207: characters 48-71 : Called from s:/_flash/shared_haxe/tinkerbell_h3/src/tink/macro/tools/TypeTools.hx:228: characters 16-40 : Called from s:/_flash/shared_haxe/tinkerbell_h3/src/tink/macro/tools/TypeTools.hx:203: characters 19-37 : Called from s:/_flash/shared_haxe/tinkerbell_h3/src/tink/macro/tools/TypeTools.hx:224: characters 63-91 : Called from s:/_flash/shared_haxe/tinkerbell_h3/src/tink/macro/tools/TypeTools.hx:203: characters 19-37 : Called from

I am identifying atm what is happening but I think I can imagine what example would cause this. I think this can be fixed by passing a cache array of complex types around these functions.

MetaChrome commented 10 years ago

Yah it appears that this occurs on recursive constraints. Namely:

<T:Poolable_i<T>>. 

(they btw, are officially supported)

MetaChrome commented 10 years ago

I attempted to resolve the issue with the following which ends up in a stackoverflow when attempting to use the complex type in building:

static function paramsToComplex(params:Array<Type>, ?local_params:StringMap<TypePath>):Array<TypeParam> {
    var ret = [];
    for (p in params) 
        ret.push(TPType(toComplex(p, true, local_params)));
    return ret;
}
static function baseToComplex(t:BaseType, params:Array<Type>, ?local_params:StringMap<TypePath>) 
    return asComplexType(t.module + '.' + t.name, paramsToComplex(params, local_params));

static public function toComplex(type:Type, ?pretty = false, ?local_params:StringMap<TypePath>):ComplexType {
    return 
        if (pretty) {
            if (local_params==null) {
                local_params=new StringMap<TypePath>();
            }
            switch (type) {
                case TEnum(t, params):
                    var t = t.get();
                    if(t.isPrivate)
                        return toComplex(type, false);
                    baseToComplex(t, params, local_params);
                case TInst(t, params):
                    var t = t.get();
                    if(t.isPrivate)
                        return toComplex(type, false);
                    switch (t.kind) {
                        #if haxe3
                        case KTypeParameter(constraints):
                            if(local_params.exists(t.name)) {
                                TPath(local_params.get(t.name));
                            } else {
                                var p:TypePath = asTypePath(t.name);
                                local_params.set(t.name,p);
                                p.params=paramsToComplex(constraints,local_params);
                                TPath(p);
                            }
                        #else
                        case KTypeParameter: asComplexType(t.name);
                        #end
                        default: baseToComplex(t, params, local_params);
                    }

Edit: Nvm, I think the above might actually be correct. I am using the above complex type as defined in a class declaration and replacing it's parameters with implementing types. I think I need to just replace kind with an applicable type or something along those lines. A complex type with the parameters implemented obviously doesn't need constraints.

Irregardless would be nice to have confirmation that the above is correct.

MetaChrome commented 10 years ago

Well, as specified this is still an open issue with the proposed fix as above. I am not sure if the proposed recursive structure specified is acceptable to the haxe compiler but one would presume so.

However, with regards to the stackoverflow I encountered, that was due to the fact that declared parameters no longer have a sub field in the TypePath and was unrelated to this.

MetaChrome commented 10 years ago

Please fix. The current implementation is incorrect. The specified change is valid.