Improve fast dfa so borrow checker supports ternary and if statements

Author: rikkimax

compiler/src/dmd/dfa/entry.d

@@ -100,7 +100,7 @@ void fastDFA(FuncDeclaration fd, Scope* sc)
     }
 
     // Use these if statements for debugging specific things.
-    //if (fd.ident.toString != "borrowOk5") return;
+    //if (fd.ident.toString != "borrowInTernaryConditionInfect") return;
     //if (!(fd.ident.toString == "checkViaObjNullDeref" || fd.ident.toString == "typeNextIterate")) return;
     //if (fd.loc.linnum != 54) return;
     //if (fd.getModule.ident.toString != "doc") return;
@@ -234,10 +234,10 @@ void fastDFA(FuncDeclaration fd, Scope* sc)
 
             dfaCommon.allocator.allVariables((DFAVar* var) {
                 prefix("var");
-                ob.printf(" %p base1=%p, base2=%p, dereferenceVar=%p, oldestLifeTimeAllowedDepth=%d<%d, writeCount=%d, unmodel=%d, isScope=%d, isByRef=%d, mayEscapeInitialValue=%d",
-                var, var.base1, var.base2, var.dereferenceVar, var.oldestLifeTimeAllowedDepth,
-                var.youngestLifeTimeAllowedDepth, var.writeCount, var.unmodellable,
-                var.isScope, var.isByRef, var.mayEscapeInitialValue);
+                ob.printf(" %p base1=%p, base2=%p, dereferenceVar=%p, storageFor=%p, oldestLifeTimeAllowedDepth=%d<%d, writeCount=%d, unmodel=%d, isScope=%d, isByRef=%d, mayEscapeInitialValue=%d",
+                var, var.base1, var.base2, var.dereferenceVar, var.storageFor,
+                var.oldestLifeTimeAllowedDepth, var.youngestLifeTimeAllowedDepth, var.writeCount,
+                var.unmodellable, var.isScope, var.isByRef, var.mayEscapeInitialValue);
 
                 if (var.var !is null)
                 {

compiler/src/dmd/dfa/fast/analysis.d

@@ -1189,7 +1189,15 @@ struct DFAAnalyzer
                     DFAScope* sideEffectScope = dfaCommon.getSideEffectScope();
                     DFAScopeVar* scv = sideEffectScope.getScopeVar(root.storageFor);
 
-                    if (argListItem.paramType is null || argListItem.paramType.isTypeMutable)
+                    version(none)
+                    {
+                        printf("walking root=%p, cell=%p\n", root, lr.findConsequence(root.storageFor));
+                    }
+
+                    // Check to see if the object is the storage for a variable that we can model.
+                    // If so we probably already handled it with seePointer, so if we were to do it again it would be a duplicate.
+                    if (lr.findConsequence(root.storageFor) is null
+                        && (argListItem.paramType is null || argListItem.paramType.isTypeMutable))
                         seeWrite(root.storageFor, scv.lr, loc, silenceWriteError);
                 }
             });
@@ -1512,7 +1520,6 @@ struct DFAAnalyzer
             DFALatticeRef lr, int alteredState, ref Loc loc,
             DFALatticeRef indexLR = DFALatticeRef.init)
     {
-
         DFAVar* assignToCtx = assignTo.getContextVar;
         DFAVar* lrCtx;
         DFAConsequence* lrCctx = lr.getContext(lrCtx);
@@ -1521,6 +1528,11 @@ struct DFAAnalyzer
         const unmodellable = lrCtx !is null && !lrCtx.isModellable;
         DFALatticeRef ret;
 
+        version(none)
+        {
+            printf("assigning to var=%p, construct=%d, isBlit=%d, alteredState=%d, noLR=%d, lrIsTruthy=%d, unmodellable=%d\n", assignToCtx, construct, isBlit, alteredState, noLR, lrIsTruthy, unmodellable);
+        }
+
         this.onRead(assignTo, loc, true);
         // Explicitly allow returns of uninitialized variables.
         this.onRead(lr, loc, assignToCtx !is null && assignToCtx is dfaCommon.getReturnVariable);
@@ -2573,6 +2585,7 @@ struct DFAAnalyzer
                 printf("found storage %p, hadAnIndirection=%d, hadAnOuterDeref=%d, takenAddressOf=%d, hadFields=%d, isOffsetOfStorage=%d, unknown=%d\n",
                     var, hadAnIndirection, hadAnOuterDeref, takenAddressOf,
                     hadFields, isOffsetOfStorage, unknown);
+                printf("   isByRef=%d\n", var.isByRef);
             }
 
             // Storage consequence may not have a object available for it.
@@ -3190,6 +3203,13 @@ private:
         {
             assignTo.visitIndirectSources((DFAVar* var, bool hadAnIndirection, bool hadAnInnerDeref, bool hadAnOuterDeref,
                     bool takenAddressOf, bool hadFields, bool isOffsetOfStorage, ref bool unknown) {
+                version (none)
+                {
+                    printf("Indirect source for %p, hadAnIndirection=%d, hadAnInnerDeref=%d, hadAnOuterDeref=%d, takenAddressOf=%d, hadFields=%d, isOffsetOfStorage=%d\n",
+                        var, hadAnIndirection, hadAnInnerDeref, hadAnOuterDeref,
+                        takenAddressOf, hadFields, isOffsetOfStorage);
+                }
+
                 if (hadAnIndirection || hadAnInnerDeref)
                     return;
 

compiler/src/dmd/dfa/fast/expression.d

@@ -696,10 +696,6 @@ struct ExpressionWalker
                                     }
                                 }
 
-                                // does this var escape another? Can't model that.
-                                if (dfaVar.isByRef)
-                                    markUnmodellable(ei.exp);
-
                                 DFALatticeRef lr = this.walk(ei.exp);
 
                                 if (!(ei.exp.isConstructExp || ei.exp.isBlitExp))
@@ -905,19 +901,27 @@ struct ExpressionWalker
                 // See Slice
                 auto ie = expr.isIndexExp;
 
-                dfaCommon.printStateln("index rhs");
-                DFALatticeRef index = this.walk(ie.e2);
+                dfaCommon.printStructureln("Index[key] expression");
+
                 dfaCommon.printStateln("index lhs");
                 DFALatticeRef lhs = this.walk(ie.e1);
+                lhs.printState("lhs");
+
+                dfaCommon.printStateln("index rhs");
+                DFALatticeRef index = this.walk(ie.e2);
+                index.printState("rhs");
 
                 DFAVar* lhsCtx;
                 DFAConsequence* lhsCctx = lhs.getContext(lhsCtx);
+                DFAVar* indexVar = dfaCommon.findIndexVar(lhsCtx);
 
                 DFAObject* lhsObject;
 
                 Type lhsType = ie.e1.type;
                 bool resultHasEffect;
 
+                DFALatticeRef ret;
+
                 if (lhsCctx !is null)
                 {
                     // Apply effects that this operation will have on to the lhs
@@ -941,31 +945,39 @@ struct ExpressionWalker
                     }
                 }
 
-                if (lhsCtx !is null && lhsCtx.isNullable)
+                if (lhsType.isTypeSArray !is null)
                 {
-                    // Dereference the lhs if its a pointer,
-                    //  this really should be the case, but it prevents unnecessary work for static arrays.
-                    lhs = seeDereference(ie.loc, lhs);
+                    // T[X] lhs;
+                    // lhs[index]
+
+                    ret = this.seeLogicalAnd(lhs, index);
+                    DFAConsequence* newCctx = ret.setContext(indexVar);
+
+                    if (lhsObject !is null && lhsObject.onTheStack)
+                        newCctx.obj = dfaCommon.makeInCellObject(lhsObject);
                 }
+                else
+                {
+                    // T[] lhs;
+                    // lhs[index]
 
-                DFAVar* indexVar = dfaCommon.findIndexVar(lhsCtx);
-                DFALatticeRef combined = this.seeLogicalAnd(lhs, index);
+                    if (lhsCtx !is null && lhsCtx.isNullable)
+                    {
+                        // Dereference the lhs if its a pointer,
+                        //  this really should be the case, but it prevents unnecessary work for static arrays.
+                        lhs = seeDereference(ie.loc, lhs);
+                    }
 
-                DFAConsequence* newCctx = combined.addConsequence(indexVar);
-                combined.setContext(newCctx);
+                    // (*lhs)[index]
 
-                if (lhsObject !is null)
-                    newCctx.obj = dfaCommon.makeInCellObject(lhsObject);
+                    ret = this.seeLogicalAnd(lhs, index);
+                    DFAConsequence* newCctx = ret.setContext(indexVar);
 
-                if (resultHasEffect && indexVar !is null)
-                {
-                    if (indexVar.isTruthy)
-                        newCctx.truthiness = Truthiness.True;
-                    if (indexVar.isNullable)
-                        newCctx.nullable = Nullable.NonNull;
+                    if (lhsObject !is null)
+                        newCctx.obj = dfaCommon.makeObject(lhsObject);
                 }
 
-                return combined;
+                return ret;
             }
 
         case EXP.slice:
@@ -1488,18 +1500,17 @@ struct ExpressionWalker
                 {
                     stmtWalker.startScope;
                     dfaCommon.currentDFAScope.sideEffectFree = true;
+                    dfaCommon.currentDFAScope.inConditional = true;
 
                     dfaCommon.printStateln("Question condition:");
                     conditionLR = this.walkCondition(qe.econd, predicateNegation);
                     conditionVar = conditionLR.getGateConsequenceVariable;
 
-                    stmtWalker.endScope;
+                    dfaCommon.currentDFAScope.sideEffectFree = false;
                 }
 
                 {
                     dfaCommon.printStateln("Question true branch:");
-                    stmtWalker.startScope;
-                    dfaCommon.currentDFAScope.inConditional = true;
 
                     DFAConsequence* c = conditionLR.getContext;
                     if (c !is null)

compiler/src/dmd/dfa/fast/report.d

@@ -575,6 +575,13 @@ private:
         void inferParameter(DFAVar* from, bool hadAnIndirection,
                 bool haveConstraintOnLifeTime, bool inCell)
         {
+            version (none)
+            {
+                printf("wanting to infer param rel from=%p, hadAnIndirection=%d, haveConstraintOnLifeTime=%d, inCell=%d, intoVar=%p, paramId=%d\n",
+                        from, hadAnIndirection, haveConstraintOnLifeTime,
+                        inCell, intoVar, intoVar.param.parameterId);
+            }
+
             EscapedRelationship currentRel = from.param.inferred.willEscape(
                     intoVar.param.parameterId);
 
@@ -614,7 +621,7 @@ private:
 
             version (none)
             {
-                printf("potential escape 1 obj %p, hadAnIndirection=%d, haveConstraintOnLifeTime=%d, inCell=%d\n",
+                printf("potential escape 1 obj=%p, hadAnIndirection=%d, haveConstraintOnLifeTime=%d, inCell=%d\n",
                     obj, hadAnIndirection, haveConstraintOnLifeTime, inCell);
             }
 

compiler/src/dmd/dfa/fast/statement.d

@@ -647,9 +647,14 @@ final:
             DFAVar* conditionVar;
 
             {
+                this.startScope;
+                dfaCommon.currentDFAScope.inConditional = true;
+                dfaCommon.currentDFAScope.sideEffectFree = true;
+
                 conditionLR = expWalker.walkCondition(ifs.condition, predicateNegation);
                 conditionVar = conditionLR.getGateConsequenceVariable;
 
+                dfaCommon.currentDFAScope.sideEffectFree = false;
                 seeRead(conditionLR, ifs.condition.loc);
             }
 
@@ -678,9 +683,6 @@ final:
 
             {
                 dfaCommon.printStructureln("If true branch:");
-                this.startScope;
-                dfaCommon.currentDFAScope.inConditional = true;
-
                 expWalker.seeSilentAssert(trueCondition, true, true);
                 this.applyGateOnBranch(conditionVar, predicateNegation, true);
             }

compiler/src/dmd/func.d

@@ -253,8 +253,7 @@ extern(D) struct ParameterDFAInfo
             else if (id == -1)
                 return cast(EscapedRelationship)((this.escapesInto >> 4) & 0x3);
 
-            this.escapesInto >>= 6 + (id * 2);
-            return cast(EscapedRelationship)(this.escapesInto & 0x3);
+            return cast(EscapedRelationship)((this.escapesInto >> (6 + (id * 2))) & 0x3);
         }
 
         /// Set this parameter's relationship strength to the following parameter id.

compiler/test/compilable/fastdfa.d

@@ -1415,4 +1415,17 @@ void classOk()
     int** b = c.get(); // ok: heap owner, no lifetime constraint
 }
 
+void borrowInConditionNoInfectElse()
+{
+    int* x;
+    if (borrowFn2(&x) != null)
+    {
+        // true branch - borrow active
+    }
+    else
+    {
+        x = null; // ok - borrow not active in false branch
+    }
+}
+
 /****************** End borrow checker (ok) ******************/

compiler/test/fail_compilation/fastdfa.d

@@ -73,12 +73,30 @@ fail_compilation/fastdfa.d(1455):        For variable `b`
 fail_compilation/fastdfa.d(1474): Error: A borrow cannot outlive the variable it borrows from
 fail_compilation/fastdfa.d(1473):        Possible source `c`
 fail_compilation/fastdfa.d(1471):        The borrow is stored in variable `b`
-fail_compilation/fastdfa.d(1487): Error: Cannot mutate the owner of an active borrow
-fail_compilation/fastdfa.d(1482):        For variable `s`
-fail_compilation/fastdfa.d(1483):        Borrowed here
-fail_compilation/fastdfa.d(1488): Error: Cannot pass the owner of an active borrow to a function that may mutate it
-fail_compilation/fastdfa.d(1488):        Parameter `obj` must be const or immutable
-fail_compilation/fastdfa.d(1483):        Borrowed here
+fail_compilation/fastdfa.d(1488): Error: Cannot mutate the owner of an active borrow
+fail_compilation/fastdfa.d(1483):        For variable `s`
+fail_compilation/fastdfa.d(1484):        Borrowed here
+fail_compilation/fastdfa.d(1489): Error: Cannot pass the owner of an active borrow to a function that may mutate it
+fail_compilation/fastdfa.d(1489):        Parameter `obj` must be const or immutable
+fail_compilation/fastdfa.d(1484):        Borrowed here
+fail_compilation/fastdfa.d(1497): Error: Cannot mutate the owner of an active borrow
+fail_compilation/fastdfa.d(1494):        For variable `x`
+fail_compilation/fastdfa.d(1495):        Borrowed here
+fail_compilation/fastdfa.d(1498): Error: Cannot mutate the owner of an active borrow
+fail_compilation/fastdfa.d(1494):        For variable `x`
+fail_compilation/fastdfa.d(1495):        Borrowed here
+fail_compilation/fastdfa.d(1499): Error: Cannot mutate the owner of an active borrow
+fail_compilation/fastdfa.d(1494):        For variable `x`
+fail_compilation/fastdfa.d(1496):        Borrowed here
+fail_compilation/fastdfa.d(1509): Error: Cannot mutate the owner of an active borrow
+fail_compilation/fastdfa.d(1504):        For variable `x`
+fail_compilation/fastdfa.d(1506):        Borrowed here
+fail_compilation/fastdfa.d(1517): Error: Cannot mutate the owner of an active borrow
+fail_compilation/fastdfa.d(1513):        For variable `x`
+fail_compilation/fastdfa.d(1516):        Borrowed here
+fail_compilation/fastdfa.d(1526): Error: Cannot mutate the owner of an active borrow
+fail_compilation/fastdfa.d(1523):        For variable `x`
+fail_compilation/fastdfa.d(1524):        Borrowed here
 ---
 */
 
@@ -516,8 +534,8 @@ void methodOutliveErr()
 void methodPassErr()
 {
     BorrowStruct s;
-    int** b = s.get(); // error: passing the borrowed owner to a mutating function
-    methodTake(&s.p);
+    int** b = s.get();
+    methodTake(&s.p); // error: passing the borrowed owner to a mutating function
 }
 
 void borrowOutliveErr1()
@@ -561,7 +579,8 @@ void classOutliveErr()
     }
 }
 
-void borrowMutateAssignCall() {
+void borrowMutateAssignCall()
+{
     void call(ref const BorrowStruct, scope int**) {}
     void borrow(scope int**) {}
 
@@ -574,4 +593,46 @@ void borrowMutateAssignCall() {
     destroy(s); // error
 }
 
+void ternaryByRefNoInfect(bool condition)
+{
+    int* x;
+    int** q = condition ? borrowFn2(&x) : &x;
+    int** r = condition ?
+        borrowFn2(&x) : // error borrow could be in here, and param is not const
+        borrowFn2(&x);  // error
+    x = null;
+}
+
+void borrowInTernaryConditionNoInfect()
+{
+    int* x;
+    int* y;
+    int** result = (borrowFn2(&x) != null) ?
+        borrowFn2(&x) : // ok, the previous borrow wasn't stored
+        &y;
+    x = null; // error could be a borrow
+}
+
+void borrowInTernaryConditionInfect() {
+    int* x;
+    int* y;
+    int** temp;
+    int** result = ((temp = borrowFn2(&x)) !is null) ?
+        borrowFn2(&x) : // error
+        &y;
+}
+
+void borrowInConditionInfect()
+{
+    int* x = new int;
+    if (int** temp = borrowFn2(&x))
+    {
+        x = null; // error
+    }
+    else
+    {
+        x = null; // ok - borrow not active in false branch
+    }
+}
+
 /****************** End borrow checker (errors) ******************/