Analyzer regression with SPOD/record macros

[jonasmalacofilho]

The analyzer seems to ignore some side-effects on code generated by SPOD/record macros, particularly when there are @:relations involved.

f1.bar = Bar.manager.select(true, { orderBy:-id, limit:1 });

doesn't set f1.bar (or the @:relation(bar_id)), but

var tmp = Bar.manager.select(true, { orderBy:-id, limit:1 });
f1.bar = tmp;

does.

This worked on v3.3-rc1, but doesn't on v3.4 or current head. I've further bisected the issue to the big rewrite on 055801388839d1935b96f94fa0e3e1566130b272.

---

A complete example:

Main.hx:

import sys.db.*;
import sys.db.Types;

class Bar extends Object {
    public var id:SId;
}

class Foo extends Object {
    public var id:SId;
    @:relation(bar_id) public var bar:Bar;
}

class Main {
    static function main()
    {
        Manager.cnx = Sqlite.open("test.db3");
        Manager.initialize();
        for (m in ([Bar.manager, Foo.manager]:Array<Manager<Dynamic>>)) {
            if (!TableCreate.exists(m))
                TableCreate.create(m);
        }

        var b1 = new Bar();
        b1.insert();

        var f1 = new Foo();
        f1.bar = Bar.manager.select(true, { orderBy:-id, limit:1 });
        f1.insert();  // FAILS, f1.bar is missing

        // but this works:
        // var tmp = Bar.manager.select(true, { orderBy:-id, limit:1 });
        // f1.bar = tmp;
        // f1.insert();

        trace(f1.bar.id);
    }
}

build.hxml:

-cmd haxe -version

--next

-neko test.n
-main Main
-lib record-macros
-cmd neko test.n
-D dump=pretty
-D analyzer
-dce=full

[nadako]

I'll try reducing it further.

[nadako]

Hmm, it actually generates this for me:

var f1 = new Foo();
var f11 = f1.set_bar;
f11(Bar.manager.unsafeObjects("SELECT * FROM Bar WHERE TRUE ORDER BY id DESC LIMIT 1", true).first());
f1.insert();

Though I can't see set_bar even generated which must be because of record-macros adding some Dynamic/untyped, I'll check it out.

[waneck]

that closure call looks bad though

[nadako]

BTW uncommenting the tmp code doesn't cause set_bar to be generated.

[nadako]

And here's the AST dump, so yeah, it's too Dynamic somewhere:

[Var f11(13576):Bar -> Bar]
    [Field:Bar -> Bar]
        [Local f1(12055):Foo]
        [FDynamic:Bar -> Bar] set_bar

[waneck]

I think the closure call is the culprit. It could be that var f11 = f1.set_bar; f11(...) is different from f1.set_bar(...) on neko, because the function does not bind this correctly

[nadako]

I though that it's a problem only on js... Anyway, I also don't understand why the property is generated as (dynamic,dynamic) instead of (get,set).

[nadako]

And how it was working before, hmm...

[waneck]

SPOD does some black magic on neko (on Manager.initialize()) that initializes the properties

[nadako]

Ew, but why?

[waneck]

Ask @ncannasse

[nadako]

This makes it an instance of #4787, I think. Better fix record-macros, really...

[waneck]

It would be pretty simple really - just take off the special case that is done on neko. But I think that someVar.someFunction(...) should never generate that kind of code anyway

[nadako]

well the issue here is that haxe tempvars it because it thinks that Bar.manager.unsafeObjects(...).first() could possibly change f1.set_bar.

[waneck]

Then it should probably make it a FClosure, not FDynamic access

[Simn]

That sounds a bit like https://github.com/HaxeFoundation/haxe/issues/5082#issuecomment-210392412.

[ncannasse]

(dynamic,dynamic) is quite old, keep in mind this was a pre-macro era (prehistoric times)

[waneck]

I'll fix it, but I still don't see a reason to not generate any field.func(...) decomposition as FClosure

[waneck]

At least on neko/js

[bablukid]

I don't want to blame anyone, but I dont understand how such a regression can happen in a stable haxe release. My whole app relies heavily on spod/record-macros, Guys at Motion-Twin have like 30 websites and thousand code lines using SPOD. I know record-macros is a separate library now, but it is "famous enough" to deserve a test suite to run against all new haxe release.

I'm not smart enough to fix this issue, but I can write these tests, should I add them in https://github.com/HaxeFoundation/haxe/tree/development/tests/sys or in record-macros repo ?

[Simn]

I don't want to have any record-macros stuff in this repository.

As for the regression, there's only so much untyped/Dynamic I can account for. If something isn't noticed during an RC phase (because not enough people test it), it's gonna end up in a so-called stable release. That's unfortunate, but I don't see how this could be avoided in general.

[bablukid]

Ok @Simn , I'll propose a PR in the record-macros repo.

IMHO if the repository is under HF github account, I understand that HF maintains the library in sync with new haxe releases. If HF doesn't want to maintain record-macros ( or any other HF library like format or hxnodejs ), you should give the repo to someone else. Therefore the community will be aware that those libraries need to be "crowd tested" against every RC ( or tested by a non HF maintainer ).

Once its clearly defined if HF takes responsability for these libs, those problems could be avoided by running strong test suites with Travis, and not by "release an RC and see what happens".

BTW The last 3.4.1 and 3.4.2 releases came very quickly after 3.4.0, prehaps it's also a signal that the testing suite is not strong and large enough to detects bugs before releasing.

Sorry to bother you, I love haxe and I'm just trying to help. I'm building my company on apps made with haxe, that's why I need confidence with my tools and why I'm talking about this.

[Simn]

I'm always happy to see more tests, and if someone wants to maintain the record-macros library, I'll be more than happy to invite him to the HaxeFoundation team to do so.

[bablukid]

I'd be glad to test and write tests for record-macros and dbadmin, but i'm not skilled enough to fix macro stuff

[ncannasse]

That seems like an important enough regression to make it a 3.4.3

[Simn]

I don't even know what to fix here.

[nadako]

Shouldn't this just be fixed in record-macros (having proper typing instead of dynamic mess)?

[Simn]

In my opinion it should be, and I thought it already was. This seems to be an instance of #4787 as you said, and that is pretty much unfixable unless you are willing to sacrifice evaluation order guarantees, which I am not.