Add const checking for elements of tuple formals (#16041)

Add const checking for elements of tuple formals (#16041)

This PR adjusts the compiler to emit errors when a const element of
a tuple formal is modified. Prior to this PR the compiler would
compile programs such as the following without incident:

```chapel

record r { var x: int = 0; }

proc modifyDefaultIntentFormal(tup) {
  tup[1] = new r(128);
}

proc test() {
  const a = (0, new r(0));
  modifyDefaultIntentFormal(a);
  writeln(a);
}
test();
```

The call to `modifyDefaultIntentFormal` incorrectly modifies the
second element of the tuple formal `tup`. Because `tup` has the
default argument intent, its second element should be passed by
`const ref` (because it is a record). Any modification of `tup[1]`
within the body of `modifyDefaultIntentFormal` should emit an error.

---

- Add a function `checkTupleFormalUses` to `lateConstCheck`. This
  method checks to make sure the constness of `const ref` tuple
  elements is not violated. If a field should be const but is set to
  `QUAL_REF` by `cullOverReferences`, then emit an error. The
  location that is reported is an imprecise guess - future work
  should make this reporting as precise as possible and provide more
  context for the use.
- Use a set of `FnSymbol*` in the `for_formals_actuals` loop in
  `lateConstCheck` so that `const ref` elements of tuple formals are
  checked once per function instead of once per call.
- Add a function `checkTupleFormalToActual` to `lateConstCheck`. This
  function checks ref/ref-if-modified elements of tuple formals. If
  the element is ref (or inferred to be ref) and the actual element
  is const, then emit an error. If the element is ref-if-modified,
  then provide an imprecise hint about where it is used. Future work
  should make this hint more precise.
- Infer the concrete intent of blank tuple formals that contain
  single/sync/atomic elements to be `INTENT_REF_MAYBE_CONST`. The
  blank intent for single/sync/atomic elements is `ref`. It does not
  make sense to infer the intent of the entire tuple as `INTENT_REF`
  if it contains such an element, so mimic what we do for blank
  intent tuples containing arrays.
- Fix a bug in `CullRefCtx::collectTuplesAndRefMaybeConstArgs` that
  caused the compiler to collect a _reference_ to a tuple for
  analysis instead of the tuple itself.
- Adjust `CullRefCtx::checkTupleCastCall` to only collect the LHS of
  a tuple cast call as a dependency if it contains `ref` elements.
  If the LHS is a value tuple, the compiler need not be concerned
  with how it is used.
- In `CullRefCtx::checkAccessorLikeCall`, do not add the LHS as a
  dependency if it is known to be constant. Any misuse of a const LHS
  should have already been reported in a previous pass.
- In `CullRefCtx::checkRefMaybeConstFormal`, escape early if the
  function being called has flag `FLAG_REF_TO_CONST_WHEN_CONST_THIS`
  and the actual is a `const` method receiver. Otherwise, we add a
  false dependency that the compiler may consider a "setting" use.
- Add debugging tools to `lateConstCheck`.
- Clean up `lateConstCheck` by moving calls to skip into a function
  `isCallToSkip`. Additional "light" refactoring might help with
  integrating new code into the rest of the pass.
- Adjust the IO proc <~> for writing channels so that the element to
  write is `const`.
- Adjust the tests `types/tuple/dinan/pining_for_lisp.chpl` and
  `pining_for_lisp_ver2.chpl` so that their `car` and `cdr` functions
  return `const ref` instead of `ref`.
- Add two tests to `types/tuple/const` that verify that the constness
  of tuple elements is respected for blank/const tuples of almost
  every element type. Future work should adjust the tests for tuples
  of sync/single when those are permitted in tuples. Future work
  should adjust the tests for tuples containing `dmap`, if possible.

---

Future work:

- Adjust `cullOverReferences` to store field index in addition to
  `ASTNode*` in the `reasonNotConst` map, and then use this
  information to print out more accurate error messages in
  `checkTupleFormalUses` and `checkTupleFormalToActual`.
- Better integrate error reporting for tuples with existing error
  reporting in `lateConstCheck`.
- Do const checking for tuple elements of tuples.
- Add tests checking constness for tuples containing `sync` and
  `single` when appropriate.

---

Testing:

  - [x] ALL on linux64 when CHPL_COMM=none
  - [x] ALL on linux64 when CHPL_COMM=gasnet

---

Reviewed by @mppf. Thanks!

Author: dlongnecke-cray

compiler/include/resolveIntents.h

@@ -26,6 +26,7 @@
 #include "expr.h"
 
 IntentTag blankIntentForType(Type* t);
+IntentTag constIntentForType(Type* t);
 IntentTag concreteIntent(IntentTag existingIntent, Type* t);
 IntentTag concreteIntentForArg(ArgSymbol* arg);
 void resolveArgIntent(ArgSymbol* arg);

compiler/passes/resolveIntents.cpp

@@ -25,7 +25,7 @@
 
 bool intentsResolved = false;
 
-static IntentTag constIntentForType(Type* t) {
+IntentTag constIntentForType(Type* t) {
   if (is_bool_type(t) ||
       is_int_type(t) ||
       is_uint_type(t) ||
@@ -85,6 +85,31 @@ bool isTupleContainingRefMaybeConst(Type* t)
   return false;
 }
 
+static bool isTupleContainingTypeWithFlag(Type* t, Flag f) {
+  AggregateType* at = toAggregateType(t->getValType());
+
+  if (at != NULL && at->symbol->hasFlag(FLAG_TUPLE)) {
+    for_fields(field, at) {
+      Type* ft = field->type->getValType();
+      if (ft->symbol->hasFlag(f))
+        return true;
+    }
+  }
+
+  return false;
+}
+
+static bool isTupleContainingSyncType(Type* t) {
+  return isTupleContainingTypeWithFlag(t, FLAG_SYNC);
+}
+
+static bool isTupleContainingSingleType(Type* t) {
+  return isTupleContainingTypeWithFlag(t, FLAG_SINGLE);
+}
+
+static bool isTupleContainingAtomicType(Type* t) {
+  return isTupleContainingTypeWithFlag(t, FLAG_ATOMIC_TYPE);
+}
 
 IntentTag blankIntentForType(Type* t) {
   IntentTag retval = INTENT_BLANK;
@@ -93,8 +118,14 @@ IntentTag blankIntentForType(Type* t) {
       t->symbol->hasFlag(FLAG_DEFAULT_INTENT_IS_REF)) {
     retval = INTENT_REF;
 
+    // Blank intent for sync/single/atomic types is ref, but we can't mark
+    // the entire tuple as INTENT_REF because that has a special meaning.
+    // So go ahead and mark the tuple as INTENT_REF_MAYBE_CONST instead.
   } else if (t->symbol->hasFlag(FLAG_DEFAULT_INTENT_IS_REF_MAYBE_CONST)
-            || isTupleContainingRefMaybeConst(t)) {
+            || isTupleContainingRefMaybeConst(t)
+            || isTupleContainingSyncType(t)
+            || isTupleContainingSingleType(t)
+            || isTupleContainingAtomicType(t)) {
     retval = INTENT_REF_MAYBE_CONST;
 
   } else if (isManagedPtrType(t)) {

compiler/resolution/cullOverReferences.cpp

@@ -67,9 +67,12 @@ static const int breakOnId2 = 0;
 static const int breakOnId3 = 0;
 
 #define DEBUG_SYMBOL(sym) \
-  if (sym->id == breakOnId1 || sym->id == breakOnId2 || sym->id == breakOnId3) { \
-    gdbShouldBreakHere(); \
-  }
+  do { \
+    if (sym->id == breakOnId1 || sym->id == breakOnId2 || \
+        sym->id == breakOnId3) { \
+      gdbShouldBreakHere(); \
+    } \
+  } while (0)
 
 static const int trace_all = 0;
 static const int trace_usr = 0;
@@ -641,6 +644,8 @@ void CullRefCtx::propagateConstnessToMethodReceivers(void) {
         if (fn != call->resolvedFunction())
           continue;
 
+        DEBUG_SYMBOL(call);
+
         SymExpr* thisActual   = toSymExpr(call->get(1));
         Symbol*  actualSymbol = thisActual->symbol();
 
@@ -700,12 +705,12 @@ void CullRefCtx::collectTuplesAndRefMaybeConstArgs(void) {
     if (arg->defPoint->parentSymbol->hasFlag(FLAG_TUPLE_CAST_FN))
       continue;
 
-    // If the argument is a tuple with reference fields, explore it...
+    // If the formal is a tuple with reference fields, explore it...
     AggregateType* argAt = toAggregateType(arg->getValType());
     if (argAt && argAt->symbol->hasFlag(FLAG_TUPLE) &&
         containsReferenceFields(argAt)) {
 
-      AggregateType* tupleType  = toAggregateType(arg->type);
+      AggregateType* tupleType  = argAt; 
       int            fieldIndex = 1;
 
       // Collect reference or tuple fields for later.
@@ -876,6 +881,7 @@ bool CullRefCtx::checkLoopForDependencies(GraphNode node, LoopInfo &info,
 // the SymExpr in a ref field? If so, add the tuple as a dependency.
 bool CullRefCtx::checkBuildTupleCall(SymExpr* se, CallExpr* call,
                                      GraphNode node, bool &revisit) {
+  Symbol* sym = se->symbol();
 
   if (FnSymbol* calledFn = call->resolvedFunction()) {
     if (!calledFn->hasFlag(FLAG_BUILD_TUPLE))
@@ -912,6 +918,8 @@ bool CullRefCtx::checkBuildTupleCall(SymExpr* se, CallExpr* call,
 
       // If it is a ref field, collect it for later.
       if (tupleField->isRef()) {
+
+        DEBUG_SYMBOL(sym);
         DEBUG_SYMBOL(lhsSymbol);
 
         GraphNode srcNode = makeNode(lhsSymbol, j);
@@ -967,23 +975,39 @@ bool CullRefCtx::checkContextCallExpr(CallExpr* call, GraphNode node,
 // consider it const if the result of the tuple cast is const.
 bool CullRefCtx::checkTupleCastCall(SymExpr* se, CallExpr* call,
                                     GraphNode node, bool &revisit) {
+  Symbol* sym = se->symbol();
 
   if (FnSymbol* calledFn = call->resolvedFunction()) {
     if (!calledFn->hasFlag(FLAG_TUPLE_CAST_FN))
       return false;
 
     CallExpr* move = toCallExpr(call->parentExpr);
 
-    if (move && move->isPrimitive(PRIM_MOVE)) {
+    if (move != NULL && move->isPrimitive(PRIM_MOVE)) {
       SymExpr*  lhs       = toSymExpr(move->get(1));
       Symbol*   lhsSymbol = lhs->symbol();
-      GraphNode srcNode   = makeNode(lhsSymbol, node.fieldIndex);
 
-      collectedSymbols.push_back(srcNode);
-      addDependency(revisitGraph, srcNode, node);
-      revisit = true;
+      AggregateType* lhsAt = toAggregateType(lhsSymbol->type);
 
-      return true;
+      // AFAIK, only internal (tuple -> tuple) casts that adjust tuple ref
+      // levels will be labeled with the flag FLAG_TUPLE_CAST_FN.
+      INT_ASSERT(lhsAt != NULL);
+      INT_ASSERT(lhsAt->symbol->hasFlag(FLAG_TUPLE));
+
+      // Only add the LHS as a dependency if it contains reference fields.
+      // If not, then it's a cast to a value tuple and we don't care
+      // about it.
+      if (containsReferenceFields(lhsAt)) {
+        DEBUG_SYMBOL(sym);
+        DEBUG_SYMBOL(lhsSymbol);
+
+        GraphNode srcNode   = makeNode(lhsSymbol, node.fieldIndex);
+        collectedSymbols.push_back(srcNode);
+        addDependency(revisitGraph, srcNode, node);
+        revisit = true;
+
+        return true;
+      }
     }
   }
 
@@ -1013,12 +1037,24 @@ bool CullRefCtx::checkAccessorLikeCall(SymExpr* se, CallExpr* call,
       Symbol*  lhsSymbol = lhs->symbol();
 
       if (lhsSymbol->isRef() && lhsSymbol != sym) {
-        GraphNode srcNode = makeNode(lhsSymbol, node.fieldIndex);
-        collectedSymbols.push_back(srcNode);
-        addDependency(revisitGraph, srcNode, node);
-        revisit = true;
 
-        return true;
+        // Don't worry about tracking the destination if it's constant or
+        // marked with the flag FLAG_REF_TO_CONST.
+        // TODO: Also check against `isConstValWillNotChange`?
+        if (lhsSymbol->hasFlag(FLAG_REF_TO_CONST) ||
+            lhsSymbol->isConstant() ||
+            QualifiedType::qualifierIsConst(lhsSymbol->qual)) {
+          return false;
+        } else {
+          GraphNode srcNode = makeNode(lhsSymbol, node.fieldIndex);
+          collectedSymbols.push_back(srcNode);
+          addDependency(revisitGraph, srcNode, node);
+          revisit = true;
+
+          DEBUG_SYMBOL(call);
+
+          return true;
+        }
       }
     }
   }
@@ -1031,6 +1067,7 @@ bool CullRefCtx::checkAccessorLikeCall(SymExpr* se, CallExpr* call,
 // function.
 bool CullRefCtx::checkRefMaybeConstFormal(SymExpr* se, CallExpr* call,
                                           GraphNode node, bool &revisit) {
+  Symbol* sym = node.variable;
 
   if (FnSymbol* calledFn = call->resolvedFunction()) {
     if (calledFn->hasFlag(FLAG_BUILD_TUPLE))
@@ -1042,6 +1079,25 @@ bool CullRefCtx::checkRefMaybeConstFormal(SymExpr* se, CallExpr* call,
     if (formal->intent != INTENT_REF_MAYBE_CONST)
       return false;
 
+    // @dlongnecke-cray:
+    //
+    // If formal is receiver marked INTENT_REF_MAYBE_CONST (e.g. array or
+    // tuple containing array), and the function to be called is marked
+    // FLAG_REF_TO_CONST_WHEN_CONST_THIS, go ahead and leave if the actual
+    // is constant. Otherwise we can create a situation where the actual
+    // receiver depends on the formal when being marked const. If the
+    // function returns a "non-const" access of the formal (e.g. via a
+    // this call), the compiler views that as an escaping reference and
+    // will transitively mark the actual receiver as non-const (when we
+    // know that it _is_ const). Things get even weirder when the function
+    // being called is an iterator.
+    if (calledFn->hasFlag(FLAG_REF_TO_CONST_WHEN_CONST_THIS) &&
+        formal->hasFlag(FLAG_ARG_THIS)) {
+      if (se->qualType().isConst()) {
+        return false;
+      }
+    }
+
     // No need to add formal to `collectedSymbols` because we already did
     // so in `collectRefMaybeConstArgs`.
 
@@ -1052,6 +1108,9 @@ bool CullRefCtx::checkRefMaybeConstFormal(SymExpr* se, CallExpr* call,
     addDependency(revisitGraph, srcNode, node);
     revisit = true;
 
+    DEBUG_SYMBOL(formal);
+    DEBUG_SYMBOL(sym);
+
     return true;
   }