Object requires field iterator (ufront TemplateData)

[kevinresol]

package;

import haxe.ds.StringMap;
using StringTools;

class Main {

    public static function main()
    {
        var d:TemplateData = {};
    }

}

abstract TemplateData({}) from {} to {} {

    /**
    Create a template data object.

    @param obj The object to use.  If null a new TemplateData object with no values will be used.
    **/
    public inline function new( ?obj:{} )
        this = (obj!=null) ? obj : {};

    /**
    Convert into a `Dynamic<Dynamic>` anonymous object.
    Please note this is not an implicit `@:to` cast, because the resulting type would match too many false positives.
    To use this cast call `templateData.toObject()` explicitly.
    **/
    public inline function toObject():Dynamic<Dynamic>
        return this;

    /**
    Convert into a `Map<String,Dynamic>`.
    This is also available as an implicit `@:to` cast.
    **/
    @:to public function toMap():Map<String,Dynamic> {
        var ret = new Map<String,Dynamic>();
        for ( k in Reflect.fields(this) ) ret[k] = Reflect.field( this, k );
        return ret;
    }

    /**
    Convert into a `StringMap<Dynamic>`.
    This is also available as an implicit `@:to` cast.
    **/
    @:to public inline function toStringMap():StringMap<Dynamic> {
        return toMap();
    }

    /**
    Get a value from the template data.

    This is also used for array access: `templateData['name']` is the same as `templateData.get('name')`.

    @param key The name of the value to retrieve.
    @return The value, or null if it was not available.
    **/
    @:arrayAccess public inline function get( key:String ):Null<Dynamic> return Reflect.field( this, key );

    /**
    See if a specific field exists in the template data.
    **/
    public function exists( key:String ):Bool {
        return Reflect.hasField( this, key );
    }

    /**
    Set a value on the template data.

    Please note array setters are also available, but they use the private `array_set` method which returns the value, rather than the TemplateData object.

    @param key The name of the key to set.
    @param val The value to set.
    @return The same TemplateData so that method chaining is enabled.
    **/
    public function set( key:String, val:Dynamic ):TemplateData {
        Reflect.setField( this, key, val );
        return new TemplateData( this );
    }

    /** Array access setter. **/
    @:arrayAccess function array_set<T>( key:String, val:T ):T {
        Reflect.setField( this, key, val );
        return val;
    }

    /**
    Set many values from a `Map<String,Dynamic>`

    `templateData.set(key,map[key])` will be called for each pair in the map.

    @param map The map data to set.
    @return The same TemplateData so that method chaining is enabled.
    **/
    public function setMap<T>( map:Map<String,T> ):TemplateData {
        for ( k in map.keys() ) {
            set( k, map[k] );
        }
        return new TemplateData( this );
    }

    /**
    Set many values from an object.

    `templateData.set(fieldName,fieldValue)` will be called for each field or property on the object.

    The behaviour differ depending on if this is an anonymous object or a class instance:

    - Anonymous objects will find all fields using `Reflect.fields()` and fetch the values using `Reflect.field()`.
    - Class instance objects will find all fields using `Type.getInstanceFields()` and fetch the values using `Reflect.getProperty()`.
    - Other values will be ignored.

    Please note on PHP, objects that are class instances may fail to load fields that are functions.

    @param d The data object to set.
    @return The same TemplateData so that method chaining is enabled.
    **/
    public function setObject( d:{} ):TemplateData {
        switch Type.typeof(d) {
            case TObject:
                for ( k in Reflect.fields(d) ) set( k, Reflect.field(d,k) );
            case TClass(cls):
                #if php
                    // PHP can't access properties on class instances using Reflect.getProperty, it throws an error.
                    // These checks and fallbacks are not required on JS or neko. It might be good to submit a bug report.
                    for ( k in Type.getInstanceFields(cls) ) {
                        try set( k, Reflect.getProperty(d,k) )
                        catch ( e:Dynamic ) try set( k, Reflect.field(d,k) )
                        catch ( e:Dynamic ) {}
                    }
                #else
                    for ( k in Type.getInstanceFields(cls) ) set( k, Reflect.getProperty(d,k) );
                #end
            case _:
        }
        return new TemplateData( this );
    }

    /** from casts **/

    /**
    Automatically cast from a `Map<String,Dynamic>` into a TemplateData.
    **/
    @:from public static function fromMap<T>( d:Map<String,T> ):TemplateData {
        var m:TemplateData = new TemplateData( {} );
        m.setMap( d );
        return m;
    }

    /**
    Automatically cast from a `StringMap<Dynamic>` into a TemplateData.
    **/
    @:from public static inline function fromStringMap<T>( d:StringMap<T> ):TemplateData {
        return fromMap( d );
    }

    /**
    Automatically cast from a `Iterable<TemplateData>` into a combined `TemplateData.

    Values will be added in order, and later values with the same name as an earlier value will override the earlier value.

    If the iterable is empty, the resulting TemplateData will contain no properties.

    If an individual item is a StringMap, it will be added with `setMap`, otherwise it will be added with `setObject`.

    @param dataSets The collection of TemplateData objects to iterate over.
    @return The same TemplateData so that method chaining is enabled.
    **/
    @:from public static function fromMany( dataSets:Iterable<TemplateData> ):TemplateData {
        var combined:TemplateData = new TemplateData( {} );
        for ( d in dataSets ) {
            if ( d!=null ) {
                if ( Std.is(d,StringMap) ) {
                    var map:StringMap<Dynamic> = cast d;
                    combined.setMap( (map:StringMap<Dynamic>) );
                }
                else {
                    var obj:Dynamic = d;
                    combined.setObject( obj );
                }
            }
        }
        return combined;
    }

    /**
    Automatically cast from an object to a TemplateData.

    This results in a new object, the equivalent of calling `new TemplateData().setObject( d )`.

    This cast comes last in the code so it should be used only if none of the other casts were utilised.
    **/
    @:from public static inline function fromObject( d:{} ):TemplateData {
        return null ; //new TemplateData( {} ).setObject( d );
    }
}

This does not work on latest build (3d001e0) but had been working in 3.2.1.

[Simn]

I cannot bisect this properly because someone pushed non-compiling commits to the development branch, but the problems start with this commit with an assert failure: https://github.com/HaxeFoundation/haxe/commit/536668463dc5de3ed785f599daf74f8a7c642682

It fails with "Object requires field iterator" on this one: https://github.com/HaxeFoundation/haxe/commit/dc7f342e3e2cba0b14301b162ffc79436ed04dfe

[ncannasse]

I think that {} has some special typing cases, maybe simply ignoring it as part of our get_abstract_froms would fix it?

[Simn]

We should probably skip all that fancy with-typing when we have {}.

[kevinresol]

Any temporary workaround for this? This is preventing me from using the dev branch because I use ufront on a daily basis.

[Simn]

I don't know how to address this right now, whatever change I try to make breaks something else. The original issue is that we ignore {} as part of the top-down inference, which causes the from {} to be ignored. The typer then picks up the @:from Iterable and tries to type against that.

Minimal example:

class Main {
    public static function main() {
        var a:A = {};
    }
}

abstract A({}) from {} {
    @:from static function fromB(b:{a:Int}):A {
        return null;
    }
}

[Simn]

I have committed a temp fix for this so it should at least work now. I don't like this though because it's an exception to the exception of the rule which reminds me of my Latin classes.

[kevinresol]

Maybe I can add a @:from static function fromStruct(b:{}) return b as a temp workaround too. (If that will work)

[Simn]

I don't think that would make a difference. We just have to address this on our end in some way that makes sense.

Anyway, can you try if you can use current Haxe dev version now so I can start looking into the duplicate metadata stuff?

[kevinresol]

I am having another bug now (was working before)

src/controller/HomeController.hx:17: characters 8-58 : Warning : @:route metadata was used on verificationController, which was not a controller.
src/controller/HomeController.hx:17: characters 8-58 : Warning :   VerificationController did not unify with ufront.web.Controller
src/controller/HomeController.hx:10: characters 8-46 : @:route metadata was used on uploadController, which is not a function.

[Simn]

Hmm, can you try with -dce no so we can be sure it's not that? I don't know how ufront determines if something is a controller or not, but VerificationController sounds like it's a controller.

[Simn]

Oh I just realized that this is a compile-time error from some macro, so DCE is not going to matter. What happens if you do var c:Controller = new VerificationController(); somewhere? I don't really see why VerificationController would fail to unify with Controller if it extends it.

[kevinresol]

-dce no doesn't help. Trying to reproduce a minimal example. But that worked :expressionless:

Using Context.unify on these two Type TInst(controller.VerificationController,[]),TInst(ufront.web.Controller,[]) and it returned false

[kevinresol]

var c:Controller = new VerificationController();

Type not found : VerificationController

Edit: NO.. it worked, I just forgot to import

[Simn]

Oh my, this might be a tricky one. It could be related to #4825 but that's not so easy to verify.

[kevinresol]

ping @Simn (just make sure you can read my edit above)

[kevinresol]

p.s. not using compilation server since switched to latest haxe build

[Simn]

I have committed something which might be related, please try again with latest dev.

[kevinresol]

No difference, using 07f211c

Have a side issue though, I built lastest haxe with brew install haxe --HEAD After that when I run haxelib list i get:

/usr/local/lib/haxe/std/neko/_std/sys/io/FileInput.hx:60: characters 26-29 : Invalid package : std should be <empty>
std/Type.hx:32: characters 1-30 : Invalid package : std should be <empty>
std/Type.hx:33: characters 1-28 : Invalid package : std should be <empty>
std/Type.hx:64: characters 15-65 : Invalid package : std should be <empty>
std/Type.hx:94: characters 15-68 : Invalid package : std should be <empty>
std/Type.hx:159: characters 15-86 : Invalid package : std should be <empty>

[Simn]

No idea right now, I'll probably have to look at it myself somehow.

[kevinresol]

It worked if I put VerificationController in the same module as HomeController (where the error is thrown)

[ncannasse]

Alternatively, you could try to run the following command to check the compiler cache state: haxe --connect <port> --display memory and check if there are some reported "leaks" in dependencies, which might cause problems with compilation server

[kevinresol]

called haxe --connect <port> --display memory twice on a clean haxe --wait run the first one looks fine and the second one has a LOT of leaks

Log: http://pastebin.com/PS8fkXA4

[Simn]

Oh god...

[ncannasse]

Ok, now we need a very small example that does reproduce at least ONE leak. A leak is defined by a module referencing one other without being listed in its collected dependencies. I wonder how it is possible to haxe.PosInfos to actualy Leak things, I guess it might be macro-related, not sure.

[ncannasse]

@kevinresol it seems you already have several compilations done in this leak report, since you have already context 67001b17f091082436248dbd295398e9 referencing previous context acde67fa4bd4d55bf306090e35a942f3 . Could you report leaks after one single compilation ? (another option could be if the macros modify the context defines, I'm not sure what would happen then...)

[kevinresol]

As mentioned, I started a clean haxe --wait and run the build two times. The first one has no leaks, see the first few lines of the log up to "Cache dump complete", and the second build starts from there

[ncannasse]

@kevinresol please run a build, then simply do the --display memory , without building anything

[kevinresol]

Not sure if I am doing it correctly:

1. Terminal A: haxe --wait 6112
2. Terminal B: haxe build.hxml --connect 6112
3. Terminal B: haxe --connect 6112 --display memory

Log of Terminal A: http://pastebin.com/Wbc7HFRF

[ncannasse]

Uhm, it seems your macro context is leaking your js context, which is strange since there should not be references from the macro context to the main context I think.

[kevinresol]

Is it user code problem or haxe problem?

[kevinresol]

I suddenly recall something, not sure but I think it is the same thing. I once tried to use the new import.hx feature and I barely remember that somehow the compiler complained that I am referencing the js package while in macro, and I never make the compiler happy even I wrap all js packages inside #if !macro

[Simn]

This is definitely a Haxe problem but I don't even know where to start looking right now.

[kevinresol]

The "leak" thing is produced from a minimal ufront project, if that helps. (But ufront itself is definitely not minimal)

[Simn]

Yes we came across it too with haxe.org. Unfortunately a minimal ufront example is still very far from a minimal example, so this is quite rough to figure out.

[ncannasse]

Maybe try a minimal minject example ?

[ncannasse]

This looks definitely macro based, so tracing which macros are called might help

[Simn]

I think this is a separate issue.

I just tried to revert your change https://github.com/HaxeFoundation/haxe/commit/eca310eac210fc3379ef560ab3fd22c20fc6344b and the "is not a controller" problem went away. This means the issue here is almost certainly related to #4825.

[ncannasse]

Uhm, it might mean the macros involved are doing some not correct things, such as typing expressions in @:build phase... Might be interesting to get an print comparison of the typing order with and without https://github.com/HaxeFoundation/haxe/commit/eca310eac210fc3379ef560ab3fd22c20fc6344b, to pinpoint which/when the macros requires some typing to be done.

[Simn]

doing some not correct things, such as typing expressions in @:build phase

Every other build-macro does that...

[ncannasse]

That shouldn't be allowed or seriously restricted. There are big chances that doing so will create some pretty complex bugs in typing ordering. Building of a class is by definition an operation that can't be reentrant.

https://github.com/HaxeFoundation/haxe/commit/eca310eac210fc3379ef560ab3fd22c20fc6344b prevent a lot of such errors from occurring, but OTOH you might force some typing while follow'ing some types while some classes have not been yet properly built.

I wonder how we should address that, but forcing all pending classes to be built as soon as we do a Context.getType/getModule does not sound like a good idea.

[Simn]

I still don't understand what horrible bugs this could cause. Worst case is that the dependency chain leads back to the type we're currently building, in which case a natural "Class WhatWeCurrentlyBuild has no field whatever" error would occur.

[ncannasse]

The problem I have is the following:

When you reach the class to build, you have a pending list of classes waiting to be built.

Entering in PTypeExpr phase will flush this list, until there's no PBuildClass delay.

Now, if you have a class that extends the class you're building, it will have its super.cl_init() return false (because you're currently building it), so it will delay_late it. But this will only put it back into the PBuildClass flush list. So we're entering an infinite loop.

[ncannasse]

The general problem is that you're not supposed to type an expression until all classes are built. We could of course skip the subclass from being built, but then how to we store this to make sure it's being correctly built after the superclass build macros as ended?

[Simn]

I see, I didn't realize that typing any expression would cause this kind of flushing.

This suggests to me that our pass system is flawed. We shouldn't have to build every class (structure) that is currently known before typing an expression. Only if that expression leads to the class structure being required (e.g. by accessing a field on it) should a build be triggered. If then we determine that the class is already being built (i.e. it's on the "stack"), we have a legitimate dependency error.

On a related note, why do we have to super.cl_build() so early in the first place? We shouldn't need its structure immediately because we can delay all override/implement checking until the very end of compilation. It looks like we only have to build the class when we build the sub-classes structure so we can deal with relevant metadata and such.

[Simn]

Given the complexity of this I suggest that for the time being we revert your change to avoid regressions, then work towards getting this right in the next Haxe release. I get what you're trying to do, but from my point of view the change merely shifts the problem to other places.

[ncannasse]

I agree in theory, but that would require a lot of checks, and forgetting a simple cl_build() might trigger a lot of hard to pinpoint errors, which occur only in some cases.

We need the super.cl_build early since it creates the dependency chain and we might inherit some build metadata.

I think we should find a solution before 3.3, I don't like to have something that is known to create infinite loop in the compiler be kept unaddressed.

[ncannasse]

Let me try something

[Simn]

Yes forgetting about cl_build() is indeed a problem. Ideally we would have something like cl_structure : unit -> tclass_structure to take care of that, but obviously that would be a massive internal change.

[ncannasse]

BTW why some field reset in cancel_build were commented out?

[Simn]

Because I split up the relation-setting from the metadata handling. Class relations are constant in the sense that it's still going to be the same tclass we're related to even if its structure changes.