Fix Issue 24159: Store lowering of CatAssignExp in a separate field (#15791)

This preserves the `CatAssignExp` in the AST until the glue layer where
an error is printed in case this expression is used with `-betterC`.
This is required to happen in the glue layer as the semantic analysis
doesn't correctly distinguish when code needs to be generated.

Signed-off-by: Teodor Dutu <teodor.dutu@gmail.com>

compiler/src/dmd/dsymbolsem.d

@@ -5901,7 +5901,7 @@ private bool isDRuntimeHook(Identifier id)
         id == Id._d_arraysetlengthTImpl || id == Id._d_arraysetlengthT ||
         id == Id._d_arraysetlengthTTrace ||
         id == Id._d_arrayappendT || id == Id._d_arrayappendTTrace ||
-        id == Id._d_arrayappendcTXImpl;
+        id == Id._d_arrayappendcTX;
 }
 
 void templateInstanceSemantic(TemplateInstance tempinst, Scope* sc, ArgumentList argumentList)

compiler/src/dmd/e2ir.d

@@ -2748,6 +2748,11 @@ elem* toElem(Expression e, ref IRState irs)
     {
         //printf("CatAssignExp.toElem('%s')\n", ce.toChars());
         elem *e;
+
+        switch (ce.op)
+        {
+            case EXP.concatenateDcharAssign:
+            {
                 Type tb1 = ce.e1.type.toBasetype();
                 Type tb2 = ce.e2.type.toBasetype();
                 assert(tb1.ty == Tarray);
@@ -2762,10 +2767,6 @@ elem* toElem(Expression e, ref IRState irs)
                 elem* re1 = addressElem(e1, ce.e1.type.pointerTo(), false);
                 elem* ev = el_same(&re1);
 
-        switch (ce.op)
-        {
-            case EXP.concatenateDcharAssign:
-            {
                 // Append dchar to char[] or wchar[]
                 assert(tb2.ty == Tdchar &&
                       (tb1n.ty == Tchar || tb1n.ty == Twchar));
@@ -2776,23 +2777,6 @@ elem* toElem(Expression e, ref IRState irs)
                         : RTLSYM.ARRAYAPPENDWD;
                 e = el_bin(OPcall, TYdarray, el_var(getRtlsym(rtl)), ep);
                 toTraceGC(irs, e, ce.loc);
-                elem_setLoc(e, ce.loc);
-                break;
-            }
-
-            case EXP.concatenateAssign:
-            {
-                assert(0, "This case should have been rewritten to `_d_arrayappendT` in the semantic phase");
-            }
-
-            case EXP.concatenateElemAssign:
-            {
-                assert(0, "This case should have been rewritten to `_d_arrayappendcTX` in the semantic phase");
-            }
-
-            default:
-                assert(0);
-        }
 
                 /* Generate: (re1, e, *ev)
                 */
@@ -2800,6 +2784,37 @@ elem* toElem(Expression e, ref IRState irs)
                 ev = el_una(OPind, e1.Ety, ev);
                 e = el_combine(e, ev);
 
+                break;
+            }
+
+            case EXP.concatenateAssign:
+            case EXP.concatenateElemAssign:
+            {
+                /* Do this check during code gen rather than semantic because appending is
+                * allowed during CTFE, and we cannot distinguish that in semantic.
+                */
+                if (!irs.params.useGC)
+                {
+                    irs.eSink.error(ce.loc,
+                        "appending to array in `%s` requires the GC which is not available with -betterC",
+                        ce.toChars());
+                    return el_long(TYint, 0);
+                }
+
+                if (auto lowering = ce.lowering)
+                    e = toElem(lowering, irs);
+                else if (ce.op == EXP.concatenateAssign)
+                    assert(0, "This case should have been rewritten to `_d_arrayappendT` in the semantic phase");
+                else
+                    assert(0, "This case should have been rewritten to `_d_arrayappendcTX` in the semantic phase");
+
+                break;
+            }
+
+            default:
+                assert(0);
+        }
+
         elem_setLoc(e, ce.loc);
         return e;
     }

compiler/src/dmd/expression.d

@@ -5633,7 +5633,9 @@ extern (C++) final class UshrAssignExp : BinAssignExp
  */
 extern (C++) class CatAssignExp : BinAssignExp
 {
-    extern (D) this(const ref Loc loc, Expression e1, Expression e2) @safe
+    Expression lowering;    // lowered druntime hook `_d_arrayappend{cTX,T}`
+
+    extern (D) this(const ref Loc loc, Expression e1, Expression e2)
     {
         super(loc, EXP.concatenateAssign, e1, e2);
     }

compiler/src/dmd/expression.h

@@ -1148,6 +1148,8 @@ public:
 class CatAssignExp : public BinAssignExp
 {
 public:
+    Expression *lowering;   // lowered druntime hook `_d_arrayappend{cTX,T}`
+
     void accept(Visitor *v) override { v->visit(this); }
 };
 

compiler/src/dmd/expressionsem.d

@@ -11199,8 +11199,7 @@ private extern (C++) final class ExpressionSemanticVisitor : Visitor
 
         result = res;
 
-        if ((exp.op == EXP.concatenateAssign || exp.op == EXP.concatenateElemAssign) &&
+        if ((exp.op == EXP.concatenateAssign || exp.op == EXP.concatenateElemAssign) && sc.needsCodegen())
-            sc.needsCodegen())
         {
             // if aa ordering is triggered, `res` will be a CommaExp
             // and `.e2` will be the rewritten original expression.
@@ -11244,7 +11243,9 @@ private extern (C++) final class ExpressionSemanticVisitor : Visitor
                 arguments.push(exp.e1);
                 arguments.push(exp.e2);
                 Expression ce = new CallExp(exp.loc, id, arguments);
-                *output = ce.expressionSemantic(sc);
+
+                exp.lowering = ce.expressionSemantic(sc);
+                *output = exp;
             }
             else if (exp.op == EXP.concatenateElemAssign)
             {
@@ -11264,15 +11265,12 @@ private extern (C++) final class ExpressionSemanticVisitor : Visitor
                 }
 
                 Identifier hook = global.params.tracegc ? Id._d_arrayappendcTXTrace : Id._d_arrayappendcTX;
-                if (!verifyHookExist(exp.loc, *sc, Id._d_arrayappendcTXImpl, "appending element to arrays", Id.object))
+                if (!verifyHookExist(exp.loc, *sc, hook, "appending element to arrays", Id.object))
                     return setError();
 
-                // Lower to object._d_arrayappendcTXImpl!(typeof(e1))._d_arrayappendcTX{,Trace}(e1, 1), e1[$-1]=e2
+                // Lower to object._d_arrayappendcTX{,Trace}(e1, 1), e1[$-1]=e2
                 Expression id = new IdentifierExp(exp.loc, Id.empty);
                 id = new DotIdExp(exp.loc, id, Id.object);
-                auto tiargs = new Objects();
-                tiargs.push(exp.e1.type);
-                id = new DotTemplateInstanceExp(exp.loc, id, Id._d_arrayappendcTXImpl, tiargs);
                 id = new DotIdExp(exp.loc, id, hook);
 
                 auto arguments = new Expressions();
@@ -11299,11 +11297,10 @@ private extern (C++) final class ExpressionSemanticVisitor : Visitor
                 {
                     /* Before the template hook, this check was performed in e2ir.d
                      * for expressions like `a ~= a[$-1]`. Here, $ will be modified
-                     * by calling `_d_arrayappendcT`, so we need to save `a[$-1]` in
+                     * by calling `_d_arrayappendcTX`, so we need to save `a[$-1]` in
                      * a temporary variable.
                      */
                     value2 = extractSideEffect(sc, "__appendtmp", eValue2, value2, true);
-                    exp.e2 = value2;
 
                     // `__appendtmp*` will be destroyed together with the array `exp.e1`.
                     auto vd = eValue2.isDeclarationExp().declaration.isVarDeclaration();
@@ -11319,13 +11316,12 @@ private extern (C++) final class ExpressionSemanticVisitor : Visitor
                 auto e0 = Expression.combine(ce, ae).expressionSemantic(sc);
                 e0 = Expression.combine(e0, value1);
                 e0 = Expression.combine(eValue1, e0);
-
                 e0 = Expression.combine(eValue2, e0);
 
-                *output = e0.expressionSemantic(sc);
+                exp.lowering = e0.expressionSemantic(sc);
+                *output = exp;
             }
         }
-
     }
 
     override void visit(AddExp exp)

compiler/src/dmd/frontend.h

@@ -7831,6 +7831,7 @@ public:
 class CatAssignExp : public BinAssignExp
 {
 public:
+    Expression* lowering;
     void accept(Visitor* v) override;
 };
 
@@ -8714,7 +8715,6 @@ struct Id final
     static Identifier* _d_arraysetlengthTTrace;
     static Identifier* _d_arrayappendT;
     static Identifier* _d_arrayappendTTrace;
-    static Identifier* _d_arrayappendcTXImpl;
     static Identifier* _d_arrayappendcTX;
     static Identifier* _d_arrayappendcTXTrace;
     static Identifier* _d_arraycatnTX;

compiler/src/dmd/id.d

@@ -366,7 +366,6 @@ immutable Msgtable[] msgtable =
     { "_d_arraysetlengthTTrace"},
     { "_d_arrayappendT" },
     { "_d_arrayappendTTrace" },
-    { "_d_arrayappendcTXImpl" },
     { "_d_arrayappendcTX" },
     { "_d_arrayappendcTXTrace" },
     { "_d_arraycatnTX" },

compiler/src/dmd/inline.d

@@ -773,6 +773,21 @@ public:
             result = ce;
         }
 
+        override void visit(CatAssignExp e)
+        {
+            auto cae = cast(CatAssignExp) e.copy();
+
+            if (auto lowering = cae.lowering)
+                cae.lowering = doInlineAs!Expression(cae.lowering, ids);
+            else
+            {
+                cae.e1 = doInlineAs!Expression(e.e1, ids);
+                cae.e2 = doInlineAs!Expression(e.e2, ids);
+            }
+
+            result = cae;
+        }
+
         override void visit(BinExp e)
         {
             auto be = cast(BinExp)e.copy();
@@ -1279,6 +1294,14 @@ public:
         inlineScan(e.e2);
     }
 
+    override void visit(CatAssignExp e)
+    {
+        if (auto lowering = e.lowering)
+            inlineScan(lowering);
+        else
+            visit(cast(BinExp) e);
+    }
+
     override void visit(BinExp e)
     {
         inlineScan(e.e1);

compiler/src/dmd/nogc.d

@@ -108,12 +108,6 @@ public:
                 return;
             f.printGCUsage(e.loc, "setting `length` may cause a GC allocation");
         }
-        else if (fd.ident == Id._d_arrayappendT || fd.ident == Id._d_arrayappendcTX)
-        {
-            if (setGC(e, "cannot use operator `~=` in `@nogc` %s `%s`"))
-                return;
-            f.printGCUsage(e.loc, "operator `~=` may cause a GC allocation");
-        }
     }
 
     override void visit(ArrayLiteralExp e)
@@ -187,20 +181,14 @@ public:
 
     override void visit(CatAssignExp e)
     {
-        /* CatAssignExp will exist in `__traits(compiles, ...)` and in the `.e1` branch of a `__ctfe ? :` CondExp.
-         * The other branch will be `_d_arrayappendcTX(e1, 1), e1[$-1]=e2` which will generate the warning about
-         * GC usage. See visit(CallExp).
-         */
         if (checkOnly)
         {
             err = true;
             return;
         }
-        if (f.setGC(e.loc, null))
+        if (setGC(e, "cannot use operator `~=` in `@nogc` %s `%s`"))
-        {
-            err = true;
             return;
-        }
+        f.printGCUsage(e.loc, "operator `~=` may cause a GC allocation");
     }
 
     override void visit(CatExp e)

compiler/src/dmd/optimize.d

@@ -928,6 +928,14 @@ Expression Expression_optimize(Expression e, int result, bool keepLvalue)
         }
     }
 
+    void visitCatAssign(CatAssignExp e)
+    {
+        if (auto lowering = e.lowering)
+            Expression_optimize(lowering, result, keepLvalue);
+        else
+            visitBinAssign(e);
+    }
+
     void visitBin(BinExp e)
     {
         //printf("BinExp::optimize(result = %d) %s\n", result, e.toChars());
@@ -1392,9 +1400,9 @@ Expression Expression_optimize(Expression e, int result, bool keepLvalue)
             case EXP.leftShiftAssign:
             case EXP.rightShiftAssign:
             case EXP.unsignedRightShiftAssign:
+            case EXP.concatenateDcharAssign: visitBinAssign(ex.isBinAssignExp()); break;
             case EXP.concatenateElemAssign:
-            case EXP.concatenateDcharAssign:
+            case EXP.concatenateAssign:      visitCatAssign(cast(CatAssignExp) ex); break;
-            case EXP.concatenateAssign: visitBinAssign(ex.isBinAssignExp()); break;
 
             case EXP.minusMinus:
             case EXP.plusPlus:

compiler/test/fail_compilation/test24159.d

@@ -0,0 +1,14 @@
+// https://issues.dlang.org/show_bug.cgi?id=24159
+// REQUIRED_ARGS: -betterC
+/*
+TEST_OUTPUT:
+---
+fail_compilation/test24159.d(13): Error: appending to array in `x ~= 3` requires the GC which is not available with -betterC
+---
+*/
+
+extern(C) void main()
+{
+    int[] x = null;
+    x ~= 3;
+}

druntime/src/core/internal/array/appending.d

@@ -14,12 +14,7 @@ private extern (C) byte[] _d_arrayappendcTX(const TypeInfo ti, ref return scope
 
 private enum isCopyingNothrow(T) = __traits(compiles, (ref T rhs) nothrow { T lhs = rhs; });
 
-/// Implementation of `_d_arrayappendcTX` and `_d_arrayappendcTXTrace`
+/**
-template _d_arrayappendcTXImpl(Tarr : T[], T)
-{
-    private enum errorMessage = "Cannot append to array if compiling without support for runtime type information!";
-
-    /**
  * Extend an array `px` by `n` elements.
  * Caller must initialize those elements.
  * Params:
@@ -32,8 +27,8 @@ template _d_arrayappendcTXImpl(Tarr : T[], T)
  *  purity, and throwabilty checks. To prevent breaking existing code, this function template
  *  is temporarily declared `@trusted pure` until the implementation can be brought up to modern D expectations.
  */
-    ref Tarr _d_arrayappendcTX(return ref scope Tarr px, size_t n) @trusted pure nothrow
+ref Tarr _d_arrayappendcTX(Tarr : T[], T)(return ref scope Tarr px, size_t n) @trusted
-    {
+{
     // needed for CTFE: https://github.com/dlang/druntime/pull/3870#issuecomment-1178800718
     version (DigitalMars) pragma(inline, false);
     version (D_TypeInfo)
@@ -49,21 +44,25 @@ template _d_arrayappendcTXImpl(Tarr : T[], T)
         return px;
     }
     else
-            assert(0, errorMessage);
+        assert(0, "Cannot append to array if compiling without support for runtime type information!");
-    }
+}
-
-    version (D_ProfileGC)
-    {
-        import core.internal.array.utils : _d_HookTraceImpl;
 
+version (D_ProfileGC)
+{
     /**
-         * TraceGC wrapper around $(REF _d_arrayappendcTX, rt,array,appending,_d_arrayappendcTXImpl).
+     * TraceGC wrapper around $(REF _d_arrayappendT, core,internal,array,appending).
-         * Bugs:
-         *  This function template was ported from a much older runtime hook that bypassed safety,
-         *  purity, and throwabilty checks. To prevent breaking existing code, this function template
-         *  is temporarily declared `@trusted pure` until the implementation can be brought up to modern D expectations.
      */
-        alias _d_arrayappendcTXTrace = _d_HookTraceImpl!(Tarr, _d_arrayappendcTX, errorMessage);
+    ref Tarr _d_arrayappendcTXTrace(Tarr : T[], T)(string file, int line, string funcname, return ref scope Tarr px, size_t n) @trusted
+    {
+        version (D_TypeInfo)
+        {
+            import core.internal.array.utils: TraceHook, gcStatsPure, accumulatePure;
+            mixin(TraceHook!(Tarr.stringof, "_d_arrayappendcTX"));
+
+            return _d_arrayappendcTX(px, n);
+        }
+        else
+            static assert(0, "Cannot append to array if compiling without support for runtime type information!");
     }
 }
 
@@ -78,7 +77,7 @@ ref Tarr _d_arrayappendT(Tarr : T[], T)(return ref scope Tarr x, scope Tarr y) @
     enum hasPostblit = __traits(hasPostblit, T);
     auto length = x.length;
 
-    _d_arrayappendcTXImpl!Tarr._d_arrayappendcTX(x, y.length);
+    _d_arrayappendcTX(x, y.length);
 
     // Only call `copyEmplace` if `T` has a copy ctor and no postblit.
     static if (hasElaborateCopyConstructor!T && !hasPostblit)
@@ -126,7 +125,7 @@ version (D_ProfileGC)
             return _d_arrayappendT(x, y);
         }
         else
-            assert(0, "Cannot append to array if compiling without support for runtime type information!");
+            static assert(0, "Cannot append to array if compiling without support for runtime type information!");
     }
 }
 

druntime/src/object.d

@@ -4667,11 +4667,12 @@ public import core.internal.array.appending : _d_arrayappendT;
 version (D_ProfileGC)
 {
     public import core.internal.array.appending : _d_arrayappendTTrace;
+    public import core.internal.array.appending : _d_arrayappendcTXTrace;
     public import core.internal.array.concatenation : _d_arraycatnTXTrace;
     public import core.lifetime : _d_newitemTTrace;
     public import core.internal.array.construction : _d_newarrayTTrace;
 }
-public import core.internal.array.appending : _d_arrayappendcTXImpl;
+public import core.internal.array.appending : _d_arrayappendcTX;
 public import core.internal.array.comparison : __cmp;
 public import core.internal.array.equality : __equals;
 public import core.internal.array.casting: __ArrayCast;