Fix 23103 - static initialization of associative arrays is not implemented #15468

dkorpel · 2023-07-28T12:17:43Z

@RazvanN7 This is the idea I discussed this morning:

Add lowering field to AssocArrayLiteralExp
Rewrite [3:4] as object.aaAsHash([3:4]) and run ctfeInterpret on it when it's a static var initializer
In todt, call Expression_toDt on the lowering, which is binary compatible

It's WIP because it only handles the easy case so far:

int[int] x = [3: 4];

While an AA could be only part of an initializer expression:

int[int][2] x = [[3: 4], [5: 6]];

Which could be implemented as well with a visitor, but I want a review on the approach first.

Update: visitor now implemented

dlang-bot · 2023-07-28T12:17:48Z

Thanks for your pull request and interest in making D better, @dkorpel! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.
Please verify that your PR follows this checklist:

My PR is fully covered with tests (you can see the coverage diff by visiting the details link of the codecov check)
My PR is as minimal as possible (smaller, focused PRs are easier to review than big ones)
I have provided a detailed rationale explaining my changes
New or modified functions have Ddoc comments (with Params: and Returns:)

Please see CONTRIBUTING.md for more information.

If you have addressed all reviews or aren't sure how to proceed, don't hesitate to ping us with a simple comment.

Bugzilla references

Auto-close	Bugzilla	Severity	Description
✓	23103	normal	static initialization of associative arrays is not implemented

Testing this PR locally

If you don't have a local development environment setup, you can use Digger to test this PR:

dub run digger -- build "master + dmd#15468"

schveiguy · 2023-07-28T13:11:13Z

Oh nice! I will take a look at how this is being done, probably the aaAsHash function can be streamlined -- the newaa thing doesn't have to actually work as a normal type, it just has to initialize the hash table.

One of the bigger things -- the asAA function adds the TypeInfo of the element at runtime, but this needs to be done at compile time for the AA to be valid. I can't remember if there's a good reason I deferred this until runtime...

And it doesn't look like you are calling the asAA function (which does this).

dkorpel · 2023-07-28T13:24:49Z

I didn't know asAA would be needed to set TypeInfo. I suppose the line hash.aa.entryTI = typeid(hash.Entry); could be added to aaAsHash. Do you know what AA operations require reading typeinfo from the structure, so I can test whether it works properly?

RazvanN7 · 2023-07-28T14:00:54Z

@dkorpel yes, this is nice and works. I got confused thinking that the AA requires extra representation for to_dt.

schveiguy · 2023-07-28T14:01:40Z

Looking for usages of entryTI:

It is used to interface with rt.lifetime for allocation, so when an entry is destroyed by the GC, things happen properly.
For some reason it is checked to see if it exists, in order to call a value destructor (not sure why this happens)

Seems we could remove the second usage, so it just becomes a parameter to the first usage.

In answer to the question, to test, you could build an AA statically with values that have destructors, then add an element, remove it, and see if the GC calls the destructor later (it obviously won't call dtors on statically constructed elements)

maxhaton · 2023-07-28T16:09:04Z

newaa -> druntime is the right move. Will take a look and hack on this on Monday when I'm back from holiday. If the basic idea is sound it shouldn't be super difficult to get the nested tables and so on working.

It's also a good idea to migrate away from a blob of hooks to libraries that happen to be in druntime - currently the amount of crap we import on every compilation is just huge, compartmentalization is a good antidote.

schveiguy · 2023-07-28T21:26:57Z

newaa -> druntime is the right move

I would say, the newaa part that initializes the AA should be moved to druntime. We don't need lookups, etc. We aren't going to use this thing other than to do static initializations.

Note that before this is included, we need a unittest (one which I intended to write) that validates the layout is identical for both real AA and the aaAsHash thing. Basically a unittest inside rt.aaA, which build a bunch of AAs at runtime and compile time, and validate they are identical.

maxhaton · 2023-07-28T21:50:00Z

newaa -> druntime is the right move

I would say, the newaa part that initializes the AA should be moved to druntime. We don't need lookups, etc. We aren't going to use this thing other than to do static initializations.

Note that before this is included, we need a unittest (one which I intended to write) that validates the layout is identical for both real AA and the aaAsHash thing. Basically a unittest inside rt.aaA, which build a bunch of AAs at runtime and compile time, and validate they are identical.

I don't see why not use it everywhere ideally? The existing implementation is a mess, quite unmalleable etc so why not use some more modern practices. It's not uncommon for a syntax such as this to desugar to a library type - we don't have to expose that to the user directly (although maybe we should). In short it would pay off some technical debt for once.

maxhaton · 2023-07-28T21:50:28Z

That and having two implementations does beg the question of how to truly test they are the same.

RazvanN7 · 2023-08-02T11:37:50Z

compiler/src/dmd/semantic2.d

+        id = new DotIdExp(exp.loc, id, Id.object);
+        id = new DotIdExp(exp.loc, id, Id._aaAsStruct);
+        auto arguments = new Expressions();
+        arguments.push(exp.syntaxCopy());


This second syntax copy does not look necessary, exp is a fresh copy of initExp and it gets reassigned anyway.

Good catch. The code was hastily copy pasted from other lowerings, so I've also given better variable names now.

dkorpel · 2023-08-03T11:34:58Z

compiler/src/dmd/semantic2.d

@@ -275,6 +275,7 @@ private extern(C++) final class Semantic2Visitor : Visitor
            // https://issues.dlang.org/show_bug.cgi?id=20417
            // Don't run CTFE for the temporary variables inside typeof or __traits(compiles)
            vd._init = vd._init.initializerSemantic(sc, vd.type, sc.intypeof == 1 || sc.flags & SCOPE.compile ? INITnointerpret : INITinterpret);
+            tryLowerStaticAA(vd, sc);


So it turns out classes get initializerSemantic run twice, so this does the lowering a second time, which is a bit wasteful. Perhaps the VarDeclaration / Initializer should get a semanticDone flag.

VarDeclaration has a semanticRun field. Doesn't that help?

Yes, I'm now checking for semanticDone >= PASS.semantic2done to skip this

druntime/src/core/internal/newaa.d

schveiguy

I really think we need a unittest to ensure binary compatibility. I'll see if I can come up with one.

schveiguy · 2023-08-09T15:52:44Z

I think we need a unittest inside rt.aaA:

// ensure the struct layout is the same
unittest
{
    import newaa = core.internal.newaa;

    // ensure the implementation structs are the same size
    static assert(newaa.Impl.sizeof == Impl.sizeof);

    // ensure all the offsets are the same
    static foreach(i; Impl.tupleof.length)
    {
        // for bucket array and Flags, we "compatible" types, not exactly the same types.
        static if(__traits(identifier, Impl.tupleof[i]) == "buckets")
            // not sure if this is right, basically I want to compare the Bucket structs.
            static assert(is(typeof(typeof(Impl.tupleof[i]).tupleof) == typeof(typeof(newaa.Impl.tupleof[i]).tupleof)));
        else static if(__traits(identifier, Impl.tupleof[i]) == "flags")
            static assert(Impl.tupleof[i].sizeof == newaa.Impl.tupleof[i].sizeof)
        else
            static assert(is(typeof(Impl.tupleof[i]) == typeof(newaa.Impl.tupleof[i])));
        static assert(Impl.tupleof[i].offsetof == newaa.Impl.tupleof[i].offsetof);
    }
}

I think this sanity check is enough for now. Probably it would be good to make some random AAs both at compile time and runtime, and ensure they have the same Bucket ordering, but maybe not necessary for this change.

As somewhat of a compromise, we could put some (or all) of the aaA Impl implementation into this file instead, and then just use it from core.rt. Like the Bucket type could be the same exact type.

You may have to make the core.internal.newaa.Impl public for this to work.

schveiguy · 2023-08-09T18:16:50Z

At some point in the future, we may have a fully-library-based API for AAs. You might be able to use a custom druntime at that point to add in your own special allocators. You could technically do that today if you were ok dealing with TypeInfo.

As it stands, I don't see any allocation schemes besides the GC fitting the AA api anyway.

schveiguy · 2023-08-11T14:40:39Z

This looks great! I don't want to push the button myself since I don't really understand how the compiler code works.

maxhaton · 2023-08-11T17:13:40Z

compiler/src/dmd/expression.d

@@ -3250,6 +3250,7 @@ extern (C++) final class AssocArrayLiteralExp : Expression

    Expressions* keys;
    Expressions* values;
+    Expression lowering; /// Lower to core.internal.newaa for static initializaton


Comment before decl please.

DDoc knows the comment belongs to lowering

It reads nicer preceding the declaration

maxhaton · 2023-08-11T17:15:08Z

compiler/src/dmd/semantic2.d

+ *   vd = Variable to lower
+ *   sc = Scope
+ */
+void tryLowerStaticAA(VarDeclaration vd, Scope* sc)


This is called tryLower, how do I know (documentation, type signature) if it failed or not?

The caller doesn't need to know whether it succeeded

If so why does the name contain try?

Because the VarDeclaration may not contain an AA

I kind of agree with max. try has a specific meaning in D.

What about lowerAnyStaticAA or something like this?

Do I understand correctly that the visitor will recurse through the AST looking for static AAs?

maxhaton · 2023-08-11T17:16:27Z

compiler/src/dmd/todt.d

@@ -488,8 +488,13 @@ extern (C++) void Expression_toDt(Expression e, ref DtBuilder dtb)
     */
    void visitAssocArrayLiteral(AssocArrayLiteralExp e)
    {
-        e.error("static initializations of associative arrays is not allowed.");
-        errorSupplemental(e.loc, "associative arrays must be initialized at runtime: https://dlang.org/spec/hash-map.html#runtime_initialization");
+        if (e.lowering)


Minor not: this branch being the other way around reads a bit nicer

maxhaton · 2023-08-11T17:17:24Z

druntime/src/core/internal/newaa.d

+   the associative array type in druntime, just enough to create an AA from an
+   existing range of key/value pairs.
+
+   Copyright: Copyright Digital Mars 2000 - 2015, Steven Schveighoffer 2022.


Digital Mars copyright should be dlang foundation I think, and the years potentially wrong?

This is copied verbatim from here: https://github.com/dlang/dmd/blob/98833b0b82b5f51c769e460c6956d6477480d6c6/druntime/src/rt/aaA.d

maxhaton · 2023-08-11T17:17:55Z

druntime/src/object.d

@@ -104,6 +104,14 @@ else version (AArch64)
    else version = WithArgTypes;
 }

+// Lower Associative Array to a newaa struct for static initialization.


Doc comment please.

This isn't really meant to be a public function.

In fact, I'm actually thinking, we should rename this with a _d_ in front to let people know this is a compiler hook and not for general use.

maxhaton · 2023-08-11T17:19:17Z

druntime/src/rt/aaA.d

+    // ensure compatible types and offsets
+    static foreach (i; 0 .. Impl.tupleof.length)
+    {
+        // for bucket array and Flags, we "compatible" types, not exactly the same types.


What counts as a compatible type.

i.e. the same layout, different types. The Bucket struct should be identical in both implementations.

Note that this test isn't adequate to ensure that, as you could e.g. reorder the fields and this would still pass. But this is better than nothing.

maxhaton · 2023-08-11T17:20:22Z

Should probably go under a preview flag to start with but could be enabled pretty swiftly. I am worried about having two AA implementations though

schveiguy · 2023-08-11T17:29:41Z

Should probably go under a preview flag to start with

This is a straight-up bug fix. I don't think it needs a preview flag. The spec specifically identifies that it's an intended feature, just not implemented.

I am worried about having two AA implementations though

This is the same implementation. I can think of some possible updates once this is merged, i.e. we can probably factor out some of the common elements, that should help make sure this stays in sync.

The big sticking point is that the rt package has no source available to an end-user, yet the compiler needs to build AAs with a template using CTFE.

dkorpel · 2023-08-11T19:20:11Z

Should probably go under a preview flag to start with

We want less preview flags, not more

…ented

dkorpel added the WIP Work In Progress - not ready for review or pulling label Jul 28, 2023

dlang-bot added the Bug Fix label Jul 28, 2023

dkorpel force-pushed the newaa branch 2 times, most recently from d871c0a to a107f99 Compare August 1, 2023 11:25

dkorpel changed the title ~~[POC] Fix 23103 - static initialization of associative arrays is not implemented~~ Fix 23103 - static initialization of associative arrays is not implemented Aug 1, 2023

dkorpel force-pushed the newaa branch 9 times, most recently from 5c2a43d to 22aa673 Compare August 1, 2023 12:36

dkorpel removed the WIP Work In Progress - not ready for review or pulling label Aug 1, 2023

dkorpel marked this pull request as ready for review August 1, 2023 15:49

dkorpel requested a review from ibuclaw as a code owner August 1, 2023 15:49

RazvanN7 reviewed Aug 2, 2023

View reviewed changes

dkorpel force-pushed the newaa branch from 22aa673 to e043335 Compare August 3, 2023 11:30

dkorpel commented Aug 3, 2023

View reviewed changes

dkorpel force-pushed the newaa branch from e043335 to ecb7375 Compare August 3, 2023 11:44

schveiguy reviewed Aug 9, 2023

View reviewed changes

druntime/src/core/internal/newaa.d Outdated Show resolved Hide resolved

schveiguy approved these changes Aug 9, 2023

View reviewed changes

dkorpel force-pushed the newaa branch from da9a82d to 72d72df Compare August 10, 2023 11:41

maxhaton reviewed Aug 11, 2023

View reviewed changes

dkorpel added 8 commits August 15, 2023 11:17

Fix 23103 - static initialization of associative arrays is not implem…

6c34a3c

…ented

Remove Phobos references in newaa.d

33bc62e

Expand static AA support and tests

449de8d

Use minimized newaa implementation

8e3eb6f

Test AA with immutable key/value

eb61453

Add semantic2done check before class initsem

d6ba782

Add test for layout compatibility

abddad8

Address review comments

4513f66

dkorpel force-pushed the newaa branch from 7f2b9aa to 4513f66 Compare August 15, 2023 09:17

dkorpel merged commit 243aaef into dlang:master Aug 31, 2023
40 checks passed

dkorpel deleted the newaa branch September 1, 2023 08:34

dkorpel mentioned this pull request Sep 1, 2023

Remove note about static initialization of AAs dlang/dlang.org#3691

Merged

ibuclaw mentioned this pull request Oct 14, 2023

Fix ICE: failed to detect static initialization of associative array #15683

Merged

ibuclaw added the New Language Feature label Oct 14, 2023

schveiguy mentioned this pull request Oct 29, 2023

Fix static AA initialization #15744

Merged

Fix 23103 - static initialization of associative arrays is not implemented #15468

Fix 23103 - static initialization of associative arrays is not implemented #15468

Conversation

dkorpel commented Jul 28, 2023 • edited Loading

dlang-bot commented Jul 28, 2023

Bugzilla references

Testing this PR locally

schveiguy commented Jul 28, 2023

dkorpel commented Jul 28, 2023 • edited Loading

RazvanN7 commented Jul 28, 2023

schveiguy commented Jul 28, 2023 • edited Loading

maxhaton commented Jul 28, 2023

schveiguy commented Jul 28, 2023

maxhaton commented Jul 28, 2023

maxhaton commented Jul 28, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

schveiguy left a comment

Choose a reason for hiding this comment

schveiguy commented Aug 9, 2023 • edited Loading

schveiguy commented Aug 9, 2023

schveiguy commented Aug 11, 2023

maxhaton Aug 11, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maxhaton commented Aug 11, 2023

schveiguy commented Aug 11, 2023 • edited Loading

dkorpel commented Aug 11, 2023

dkorpel commented Jul 28, 2023 •

edited

Loading

dkorpel commented Jul 28, 2023 •

edited

Loading

schveiguy commented Jul 28, 2023 •

edited

Loading

schveiguy commented Aug 9, 2023 •

edited

Loading

maxhaton Aug 11, 2023 •

edited

Loading

schveiguy commented Aug 11, 2023 •

edited

Loading