Inconsistent structual typing of inferred anon-types.

[deltaluca]

class Test {
    static function main() {
        var x1 = {x:1, y:2};
        var x2 = ({x:1, y:2}:{x:Int,y:Int});
        var x3:{x:Int,y:Int} = {x:1, y:2};
        var y1:{} = x1;
        var y2:{} = x2;
        var y3:{} = x3;
        var z1:{x:Int} = x1; // <-- error only on this line
        var z2:{x:Int} = x2;
        var z3:{x:Int} = x3;
    }
}

{y:Int,x:Int} should be {x:Int}
{y:Int,x:Int} has extra field y

[Simn]

Knowing our anon type rules this is probably totally expected due to x1 having Const status. I'll let @ncannasse confirm that though.

[deltaluca]

Const status?

[Simn]

I just tried explaining it here: #3198 (comment)

[ncannasse]

The idea is that constant structures are not allowed to be reduced. This allows for instance to check for the following:

function foo(o:{?x:Int,?y:Int}) {
}
var pt = { x: 0, yy : 1 }; // typo
foo(pt); // goes unnoticed

Also, it will gives error if you modify the signature of foo, for instance by removing a field.

[deltaluca]

It seems fundamentally wrong to me that this is not represented in the type system though. Adding an explicit type that is the exact same as inferred type should not be changing semantics...

[deltaluca]

I seriously question the logic in helping track an occasional possible typo with removing structual typing by default with crazy compiler internal semantics. Surely it would be better to keep things consistent and instead provide a warning of unused fields that would still catch this typo and keep structual typing

[ncannasse]

We don't like much warnings in general and try to avoid them most of the time.
But since that case often comes up yes we could turn that into a warning to be a bit more precise since it's not an error per-se.

[Simn]

OT: When you refer to yourself as "we" I always have to imagine you dressed up as the Borgia pope, speaking these words in the voice of Jeremy Irons. Unfortunately that makes it pretty much impossible to disagree with anything you say.

As for the actual issue, detecting unused fields for Const anon types makes a lot of sense to me. I think it should still be an error though, just like e.g. unused patterns are an error. I very much doubt anyone would want to deliberately add unused fields in order to introduce side-effects, and if they do they deserve to be punished.

We can still keep the Const status to help with this detection, but should treat it just like Closed.

[ncannasse]

The problem with current error behavior is that it doesn't teach people that "we" actually support structural subtyping. By making it a warning, "we"'re being much more clear that it's allowed, but that "we" are also smart enough to tell the user something in wrong.

"We" then agree to make an exception to the divine rule that warnings should be errors, as dictated by the Emperor Nicolas the First.

[Simn]

Ok, but I still don't really know what to do with this. :P

[ncannasse]

The idea is to just turn the error into a warning. However given the check happens in unify I wonder if that's actually feasible in a simple way...

[ncannasse]

PS : we can't just catch+turn the exception into warning because some other parts of the unify might have not been done

[Simn]

How about we remove this restriction and I train the analyzer to detect fields which are never accessed or unified? That should take care of all typo cases.

[nadako]

that sounds just right

[ncannasse]

@Simn I'm pretty sure this would be breaking a lot of code, especially when having Reflection/Dynamic

[Simn]

Unifying an object with Dynamic counts as all fields being used.

[ncannasse]

@Simn if you can get the same warnings as we currently get errors (no less, no more) then it's good for me

[Simn]

I just re-read everything and still don't think that turning this into a warning is the right thing to do. Warnings only make sense when there's an obvious fix for them and I have trouble spotting obvious fixes here. Let's look at a really basic example:

class Main {
    static function main() {
        var x1 = {x:1, y:2};
        var z1:{x:Int} = x1;
        trace(x1.y);
    }
}

Let's assume I get a warning from that because of the extra y field. Then what? I don't want to remove the field because it's actually used. I also cannot adjust the type of z1 in the general case because it could be some API function somewhere.

The only thing I can do is type-hint x1 to {x:Int, y:Int}. But that raises some other questions, like why that makes a difference in the first place. Usually type-inference takes care of that, so why is that not the case here?

I keep coming back to the idea that extra fields should only be reported when we're directly declaring a structure against a type that has less fields.

[ncannasse]

I'm fine with your last sentence, it should clear up misunderstandings about structural subtyping while still eliminating most of typo errors (when optional fields are involved in particular). I'm just a bit concerned that this will go unnoticed:

function foo( o : { x : Int, ?foo : Int } ) {
}
var o = { x : 0, fooo : 10 }; // typo
foo(o); // pass! 

[Simn]

Yes I know, which is why I think detecting unused fields is ultimately the way to go.

[Simn]

Doesn't this mean we can get rid of the Const flag entirely? I know it does something special in unify_min which I don't understand. There's also a hack for using_field which I believe becomes irrelevant once unification is allowed properly.

[Simn]

@ncannasse: I'm working on this part of the compiler right now, can you confirm that we can get rid of the Const flag?

[ncannasse]

We can get rid of it as long as we have a check at a later compilation phase which warn for extra fields when unifying a TObjectDecl with a TAnon

[ncannasse]

... or if we use with_type to warn about it when typing EObjectDecl, that should be enough

[Simn]

... or if we use with_type to warn about it when typing EObjectDecl, that should be enough

That's the plan!

[nadako]

Looks like this should be reopened

[jasononeil]

Here's a try-haxe with the same code producing the same error: https://try.haxe.org/#62857

The same problem occurs on 3.4.4, 4.0.0-preview.2 and the current nightly (git build development @ 8951632)

[haxiomic]

I would be nice to be able to suppress the warning in the context of externs (or by using metadata)

In js externs sometimes you have a structure that has some explicit fields but allows extra fields (that will be used at runtime)

[Gama11]

Should that then be Dynamic<{explitiFields}> or something like that?

[haxiomic]

That would compile but you’d lose type check and autocomplete on your other fields I think