HPCC-20974 Optimize the code that resolves dataset selectors

The previous code reported the issues when the no_select expression was traversed
but that required expressions to be walked multiple times in different scopes.
Moving the check to the outer expressions allows more expressions to be traversed
only once.

Signed-off-by: Gavin Halliday <gavin.halliday@lexisnexis.com>

ecl/hql/hqlthql.cpp

@@ -734,6 +734,8 @@ void HqltHql::toECL(IHqlExpression *expr, StringBuffer &s, bool paren, bool inTy
             s.append(']');
 #endif
         }
+        if (containsImplicitNormalize(expr))
+            s.append('.');
         if (containsAssertKeyed(expr))
             s.append('K');
         if (containsAliasLocally(expr))

ecl/hql/hqltrans.cpp

@@ -3926,7 +3926,6 @@ IHqlExpression * ScopedTransformer::createTransformed(IHqlExpression * expr)
     case no_extractresult:
     case no_createdictionary:
         {
-            unsigned __int64 savedCount = getNestedUsageCount();
             IHqlExpression * dataset = expr->queryChild(0);
             pushScope(expr);
             IHqlExpression * transformedDs = transform(dataset);
@@ -3936,11 +3935,6 @@ IHqlExpression * ScopedTransformer::createTransformed(IHqlExpression * expr)
                 children.append(*transform(expr->queryChild(idx)));
             clearDataset(nested);
             popScope();
-
-            //There were no references to outer datasets in the contained expressions => it will always be transformed
-            //the same way as long as there are no instances of globalDataset.childDataset
-            if (!containsImplicitNormalize(expr) && (savedCount == getNestedUsageCount()))
-                noteTransformedOnce(expr);
             break;
         }
     case no_projectrow:
@@ -4402,6 +4396,17 @@ bool ScopedTransformer::isDatasetRelatedToScope(IHqlExpression * search)
     return false;
 }
 
+bool ScopedTransformer::isWithinRootScope() const
+{
+    ForEachItemIn(idx, scopeStack)
+    {
+        ScopeInfo & cur = scopeStack.item(idx);
+        if (cur.dataset || cur.left)
+            return false;
+    }
+    return true;
+}
+
 bool ScopedTransformer::checkInScope(IHqlExpression * selector, bool allowCreate)
 {
     switch (selector->getOperator())
@@ -4445,16 +4450,10 @@ bool ScopedTransformer::checkInScope(IHqlExpression * selector, bool allowCreate
     {
         ScopeInfo & cur = scopeStack.item(idx);
         if (cur.dataset && cur.dataset->queryNormalizedSelector(false) == normalized)
-        {
-            cur.usageCount++;
             return true;
-        }
 
         if (isInImplictScope(cur.dataset, normalized))
-        {
-            cur.usageCount++;
             return true;
-        }
     }
     return false;
 }
@@ -4535,15 +4534,6 @@ unsigned ScopedTransformer::tableNesting()
     return numTables;
 }
 
-unsigned __int64 ScopedTransformer::getNestedUsageCount()
-{
-    unsigned __int64 total = 0;
-    ForEachItemIn(i, scopeStack)
-        total += scopeStack.item(i).usageCount;
-    return total;
-}
-
-
 bool ScopedTransformer::insideActivity()
 {
     return (scopeStack.ordinality() != 0) || (savedStack.ordinality() != 0);

ecl/hql/hqltrans.ipp

@@ -1035,7 +1035,6 @@ public:
 
 public:
     IHqlExpression * context;
-    unsigned __int64 usageCount = 0;
     HqlExprAttr     dataset;
     HqlExprAttr     transformedDataset;
     HqlExprAttr     left;
@@ -1069,10 +1068,10 @@ protected:
     bool isDatasetActive(IHqlExpression * expr);                // is it in an active dataset?
     bool isDatasetARow(IHqlExpression * expr);                  // can it be used as a row?
     bool isDatasetRelatedToScope(IHqlExpression * dataset);
+    bool isWithinRootScope() const;
     bool isNewDataset()                                             { return innerScope && innerScope->isEmpty(); }
     bool insideActivity();
     unsigned tableNesting();
-    unsigned __int64 getNestedUsageCount();
 
     IHqlExpression * getScopeState();
 

ecl/hqlcpp/hqlttcpp.cpp

@@ -9922,7 +9922,7 @@ IHqlExpression * HqlLinkedChildRowTransformer::createTransformedBody(IHqlExpress
 
 HqlScopeTaggerInfo::HqlScopeTaggerInfo(IHqlExpression * _expr) : MergingTransformInfo(_expr)
 {
-    if (!onlyTransformOnce() && isIndependentOfScope(_expr))
+    if (!onlyTransformOnce() && (!containsImplicitNormalize(_expr) || _expr->isIndependentOfScope()))
     {
         //If the node doesn't have any active selectors then it isn't going to be context dependent
         setOnlyTransformOnce(true);
@@ -9944,11 +9944,24 @@ ANewTransformInfo * HqlScopeTagger::createTransformInfo(IHqlExpression * expr)
  *
  *   SELF.x := ds1
  *      This either means the entire ds1, or the current row in ds1 = depending on whether dataset is active or not
- *      (e.g, due to a TABLE() statement.)  This code reports a warning in this case - active(ds1) should be used for
+ *      (e.g, due to a TABLE() statement.)  This code reports an error in this case - active(ds1) should be used for
  *      the second case.
  *
- * In order to achieve this the entire expression tree needs to be transformed differently within each potentital dataset
+ * The following contexts are situations where the datasets are potentially ambiguous:
+ *    o left hand side of a no_select (case detailed above)
+ *    o parameters
+ *    o right hand sizes of assignments
+ *    o sizeof()
+ *
+ * For the first the ambiguity is resolved.  For all the others, it is treated as a new dataset, and a warning/error
+ * is reported if the dataset is in scope.  (?How would you suppress this?)
+ * reference to an active dataset should be done with ROW(dataset)
+ *
+ * In order to achieve this the entire expression tree needs to be transformed differently within each potential dataset
  * scope combination.
+ * This means that an expression that does not contain a normalized select (a.ds.x) must be transformed the same way each
+ * time.
+ *
 
 Details of the no_select representation:
 
@@ -9970,6 +9983,49 @@ Note:
 
   */
 
+
+//Recursively search for an expression which refers to the first selector that is unresolved.
+static IHqlExpression * findOriginalReference(IHqlExpression * * location, IHqlExpression * expr, IHqlExpression * transformed, IHqlExpression * search)
+{
+    for(;;)
+    {
+        if (transformed == search)
+            return expr;
+
+        //If this is a no_select expression that is not resolving from a row then we have a match
+        node_operator transformOp = transformed->getOperator();
+        if ((transformOp == no_select) && !isNewSelector(transformed))
+            return expr;
+
+        //If tree has been transformed to a different structure then do not continue
+        if (transformOp != expr->getOperator())
+            return nullptr;
+
+        //Keep track of the best location that we have seen so far
+        IHqlExpression * symbol = queryLocation(expr);
+        if (symbol && location && (symbol->getStartLine() != 0))
+            *location = symbol;
+
+        //Find the first child expression that is also dependent on that selector
+        bool matched = false;
+        ForEachChild(i, transformed)
+        {
+            IHqlExpression * cur = transformed->queryChild(i);
+            if (cur->usesSelector(search))
+            {
+                transformed = cur;
+                expr = expr->queryChild(i);
+                matched = true;
+                break;
+            }
+        }
+
+        if (!matched || !expr)
+            return nullptr;
+    }
+}
+
+
 static HqlTransformerInfo hqlScopeTaggerInfo("HqlScopeTagger");
 HqlScopeTagger::HqlScopeTagger(IErrorReceiver & _errors, ErrorSeverityMapper & _errorMapper)
 : ScopedDependentTransformer(hqlScopeTaggerInfo), errors(_errors), errorMapper(_errorMapper)
@@ -10332,6 +10388,7 @@ IHqlExpression * HqlScopeTagger::createTransformed(IHqlExpression * expr)
             }
             break;
         case annotate_symbol:
+        case annotate_location:
             {
                 ErrorSeverityMapper::SymbolScope saved(errorMapper, expr);
                 OwnedHqlExpr transformedBody = transform(body);
@@ -10339,10 +10396,6 @@ IHqlExpression * HqlScopeTagger::createTransformed(IHqlExpression * expr)
                     return LINK(expr);
                 return expr->cloneAnnotation(transformedBody);
             }
-        case annotate_location:
-            {
-                break;
-            }
         }
         OwnedHqlExpr transformedBody = transform(body);
         if (body == transformedBody)
@@ -10464,15 +10517,38 @@ IHqlExpression * HqlScopeTagger::createTransformed(IHqlExpression * expr)
             }
             return expr->clone(children);
         }
+    case no_colon:
+    {
+        OwnedHqlExpr transformed = Parent::createTransformed(expr);
+        IHqlExpression * child = transformed->queryChild(0);
+        if (!child->isIndependentOfScope())
+            reportRootSelectorError(expr->queryChild(0), child);
+        return transformed.getClear();
+    }
+    case no_apply:
+    case no_output:
+    case no_outputscalar:
+    case no_setresult:
+    case no_buildindex:
+        {
+            OwnedHqlExpr transformed = Parent::createTransformed(expr);
+            if (isWithinRootScope() && !transformed->isIndependentOfScope())
+                reportRootSelectorError(expr, transformed);
+            return transformed.getClear();
+        }
     }
 
     return Parent::createTransformed(expr);
 }
 
-
 void HqlScopeTagger::reportError(WarnErrorCategory category, const char * msg)
 {
     IHqlExpression * location = errorMapper.queryActiveSymbol();
+    reportError(category, msg, location);
+}
+
+void HqlScopeTagger::reportError(WarnErrorCategory category, const char * msg, IHqlExpression * location)
+{
     //Make this an error when we are confident...
     int startLine= location ? location->getStartLine() : 0;
     int startColumn = location ? location->getStartColumn() : 0;
@@ -10482,6 +10558,38 @@ void HqlScopeTagger::reportError(WarnErrorCategory category, const char * msg)
     errors.report(err);        // will throw immediately if it is an error.
 }
 
+void HqlScopeTagger::reportRootSelectorError(IHqlExpression * expr, IHqlExpression * transformed)
+{
+    HqlExprCopyArray inScope;
+    transformed->gatherTablesUsed(nullptr, &inScope);
+    assertex(inScope.ordinality());
+
+    //Recursively search for an expression which refers to the first selector that is unresolved.
+    IHqlExpression * selector = &inScope.item(0);
+    IHqlExpression * location = nullptr;
+    IHqlExpression * original = findOriginalReference(&location, expr, transformed, selector);
+    if (original)
+    {
+        StringBuffer exprText;
+        getECL(original->queryBody(), exprText);
+        elideString(exprText, 100); // very unlikely to be long, but limit just in case
+
+        VStringBuffer msg("expression '%s' is used within expression, but is not in scope", exprText.str());
+        if (!location)
+            location = errorMapper.queryActiveSymbol();
+        reportError(CategoryError, msg, location);
+    }
+    else
+    {
+        StringBuffer exprText;
+        getECL(expr, exprText);
+        elideString(exprText, 30);
+
+        VStringBuffer msg("Global expression '%s' uses fields from a dataset that is not in scope", exprText.str());
+        reportError(CategoryError, msg);
+    }
+}
+
 
 //---------------------------------------------------------------------------------------------------------------------
 

ecl/hqlcpp/hqlttcpp.ipp

@@ -1100,7 +1100,9 @@ protected:
     IHqlExpression * transformWithin(IHqlExpression * dataset, IHqlExpression * scope);
 
     void reportError(WarnErrorCategory category, const char * msg);
+    void reportError(WarnErrorCategory category, const char * msg, IHqlExpression * location);
     void reportSelectorError(IHqlExpression * selector, IHqlExpression * expr);
+    void reportRootSelectorError(IHqlExpression * expr, IHqlExpression * transformed);
 
 protected:
     IErrorReceiver & errors;