Full implementation of detection of identifier hiding

Src/java/cql-to-elm/src/main/java/org/cqframework/cql/cql2elm/Cql2ElmVisitor.java

@@ -26,7 +26,7 @@
 
 
 public class Cql2ElmVisitor extends CqlPreprocessorElmCommonVisitor {
-    static final Logger logger = LoggerFactory.getLogger(Cql2ElmVisitor.class);
+    private static final Logger logger = LoggerFactory.getLogger(Cql2ElmVisitor.class);
     private final SystemMethodResolver systemMethodResolver;
 
     public void setLibraryInfo(LibraryInfo libraryInfo) {
@@ -134,7 +134,9 @@ public UsingDef visitUsingDefinition(cqlParser.UsingDefinitionContext ctx) {
         }
 
         // The model was already calculated by CqlPreprocessorVisitor
-        return libraryBuilder.resolveUsingRef(localIdentifier);
+        final UsingDef usingDef = libraryBuilder.resolveUsingRef(localIdentifier);
+        libraryBuilder.pushIdentifierForHiding(localIdentifier, false, usingDef, null);
+        return usingDef;
     }
 
     public Model getModel() {
@@ -196,6 +198,7 @@ public Object visitIncludeDefinition(cqlParser.IncludeDefinitionContext ctx) {
         }
 
         libraryBuilder.addInclude(library);
+        libraryBuilder.pushIdentifierForHiding(library.getLocalIdentifier(), false, library, null);
 
         return library;
     }
@@ -232,6 +235,7 @@ public ParameterDef visitParameterDefinition(cqlParser.ParameterDefinitionContex
         }
 
         libraryBuilder.addParameter(param);
+        libraryBuilder.pushIdentifierForHiding(param.getName(), false, param, libraryBuilder.resolveParameterRef(param));
 
         return param;
     }
@@ -274,6 +278,7 @@ public CodeSystemDef visitCodesystemDefinition(cqlParser.CodesystemDefinitionCon
         }
 
         libraryBuilder.addCodeSystem(cs);
+        libraryBuilder.pushIdentifierForHiding(cs.getName(), false, cs, libraryBuilder.getCodeSystemRef(cs));
         return cs;
     }
 
@@ -345,6 +350,7 @@ public ValueSetDef visitValuesetDefinition(cqlParser.ValuesetDefinitionContext c
             vs.setResultType(new ListType(libraryBuilder.resolveTypeName("System", "Code")));
         }
         libraryBuilder.addValueSet(vs);
+        libraryBuilder.pushIdentifierForHiding(vs.getName(), false, vs, libraryBuilder.getValueSetRef(vs));
 
         return vs;
     }
@@ -366,6 +372,7 @@ public CodeDef visitCodeDefinition(cqlParser.CodeDefinitionContext ctx) {
 
         cd.setResultType(libraryBuilder.resolveTypeName("Code"));
         libraryBuilder.addCode(cd);
+        libraryBuilder.pushIdentifierForHiding(cd.getName(), false, cd, libraryBuilder.getCodeRef(cd));
 
         return cd;
     }
@@ -514,6 +521,12 @@ private void removeImplicitContextExpressionDef(ExpressionDef def) {
     public ExpressionDef internalVisitExpressionDefinition(cqlParser.ExpressionDefinitionContext ctx) {
         String identifier = parseString(ctx.identifier());
         ExpressionDef def = libraryBuilder.resolveExpressionRef(identifier);
+
+        // lightweight ExpressionDef to be used to output a hiding warning message
+        final ExpressionDef hallowExpressionDef = of.createExpressionDef()
+                .withName(identifier)
+                .withContext(getCurrentContext());
+        libraryBuilder.pushIdentifierForHiding(identifier, false, hallowExpressionDef, null);
         if (def == null || isImplicitContextExpressionDef(def)) {
             if (def != null && isImplicitContextExpressionDef(def)) {
                 libraryBuilder.removeExpression(def);
@@ -3049,9 +3062,9 @@ public Object visitSourceClause(cqlParser.SourceClauseContext ctx) {
     public Object visitQuery(cqlParser.QueryContext ctx) {
         QueryContext queryContext = new QueryContext();
         libraryBuilder.pushQueryContext(queryContext);
+        List<AliasedQuerySource> sources = null;
         try {
 
-            List<AliasedQuerySource> sources;
             queryContext.enterSourceClause();
             try {
                 sources = (List<AliasedQuerySource>)visit(ctx.sourceClause());
@@ -3062,6 +3075,10 @@ public Object visitQuery(cqlParser.QueryContext ctx) {
 
             queryContext.addPrimaryQuerySources(sources);
 
+            for (AliasedQuerySource source : sources) {
+                libraryBuilder.pushIdentifierForHiding(source.getAlias(), false, source, source.getExpression());
+            }
+
             // If we are evaluating a population-level query whose source ranges over any patient-context expressions,
             // then references to patient context expressions within the iteration clauses of the query can be accessed
             // at the patient, rather than the population, context.
@@ -3072,9 +3089,15 @@ public Object visitQuery(cqlParser.QueryContext ctx) {
                 expressionContextPushed = true;
             }
             */
+            List<LetClause> dfcx = null;
             try {
+                dfcx = ctx.letClause() != null ? (List<LetClause>) visit(ctx.letClause()) : null;
 
-                List<LetClause> dfcx = ctx.letClause() != null ? (List<LetClause>) visit(ctx.letClause()) : null;
+                if (dfcx != null) {
+                    for (LetClause letClause : dfcx) {
+                        libraryBuilder.pushIdentifierForHiding(letClause.getIdentifier(), false, letClause, libraryBuilder.getQueryLetRef(letClause));
+                    }
+                }
 
                 List<RelationshipClause> qicx = new ArrayList<>();
                 if (ctx.queryInclusionClause() != null) {
@@ -3180,10 +3203,21 @@ else if (ret != null) {
                 if (expressionContextPushed) {
                     libraryBuilder.popExpressionContext();
                 }
+                if (dfcx != null) {
+                    for (LetClause letClause : dfcx) {
+                        libraryBuilder.popIdentifierForHiding();
+                    }
+                }
+
             }
 
         } finally {
             libraryBuilder.popQueryContext();
+            if (sources != null) {
+                for (AliasedQuerySource source : sources) {
+                    libraryBuilder.popIdentifierForHiding();
+                }
+            }
         }
     }
 
@@ -4017,6 +4051,11 @@ public FunctionDef compileFunctionDefinition(cqlParser.FunctionDefinitionContext
         if (op == null) {
             throw new IllegalArgumentException(String.format("Internal error: Could not resolve operator map entry for function header %s", fh.getMangledName()));
         }
+        libraryBuilder.pushIdentifierForHiding(fun.getName(), true, fun, fun.getExpression());
+        final List<OperandDef> operand = op.getFunctionDef().getOperand();
+        for (OperandDef operandDef : operand) {
+            libraryBuilder.pushIdentifierForHiding(operandDef.getName(), false, operandDef, libraryBuilder.resolveOperandRef(operandDef));
+        }
 
         if (ctx.functionBody() != null) {
             libraryBuilder.beginFunctionDef(fun);
@@ -4033,6 +4072,9 @@ public FunctionDef compileFunctionDefinition(cqlParser.FunctionDefinitionContext
                     libraryBuilder.popExpressionContext();
                 }
             } finally {
+                for (OperandDef operandDef : operand) {
+                    libraryBuilder.popIdentifierForHiding();
+                }
                 libraryBuilder.endFunctionDef();
             }
 

Src/java/cql-to-elm/src/main/java/org/cqframework/cql/cql2elm/LibraryBuilder.java

@@ -14,7 +14,6 @@
 import java.math.BigDecimal;
 import java.util.*;
 import java.util.List;
-import java.util.stream.Collectors;
 
 /**
  * Created by Bryn on 12/29/2016.
@@ -101,6 +100,7 @@ public List<CqlCompilerException> getExceptions() {
     private final Stack<String> expressionContext = new Stack<>();
     private final ExpressionDefinitionContextStack expressionDefinitions = new ExpressionDefinitionContextStack();
     private final Stack<FunctionDef> functionDefs = new Stack<>();
+    private final Deque<String> identifiersToCheckForHiding = new ArrayDeque<>();
     private int literalContext = 0;
     private int typeSpecifierContext = 0;
     private NamespaceInfo namespaceInfo = null;
@@ -1428,7 +1428,7 @@ private Expression convertListExpression(Expression expression, Conversion conve
     }
 
     private void reportWarning(String message, Expression expression) {
-        TrackBack trackback = expression.getTrackbacks() != null && expression.getTrackbacks().size() > 0 ? expression.getTrackbacks().get(0) : null;
+        TrackBack trackback = expression != null && expression.getTrackbacks() != null && !expression.getTrackbacks().isEmpty() ? expression.getTrackbacks().get(0) : null;
         CqlSemanticException warning = new CqlSemanticException(message, CqlCompilerException.ErrorSeverity.Warning, trackback);
         recordParsingException(warning);
     }
@@ -2344,6 +2344,114 @@ public Expression resolveIdentifier(String identifier, boolean mustResolve) {
         return null;
     }
 
+    public QueryLetRef getQueryLetRef(LetClause let) {
+        QueryLetRef result = of.createQueryLetRef().withName(let.getIdentifier());
+        result.setResultType(let.getResultType());
+        return result;
+    }
+
+    public ValueSetRef getValueSetRef(ValueSetDef valueSetDef) {
+        checkLiteralContext();
+        ValueSetRef valuesetRef = of.createValueSetRef().withName(valueSetDef.getName());
+        valuesetRef.setResultType(valueSetDef.getResultType());
+        if (valuesetRef.getResultType() == null) {
+            // ERROR:
+            throw new IllegalArgumentException(String.format("Could not validate reference to valueset %s because its definition contains errors.",
+                    valuesetRef.getName()));
+        }
+        if (isCompatibleWith("1.5")) {
+            valuesetRef.setPreserve(true);
+        }
+
+        return valuesetRef;
+    }
+
+    public Expression getCodeRef(CodeDef codeDef) {
+        checkLiteralContext();
+        CodeRef codeRef = of.createCodeRef().withName((codeDef).getName());
+        codeRef.setResultType(codeDef.getResultType());
+        if (codeRef.getResultType() == null) {
+            // ERROR:
+            throw new IllegalArgumentException(String.format("Could not validate reference to code %s because its definition contains errors.",
+                    codeRef.getName()));
+        }
+        return codeRef;
+    }
+
+    public Expression getCodeSystemRef(CodeSystemDef codeSystemDef) {
+        checkLiteralContext();
+        CodeSystemRef codesystemRef = of.createCodeSystemRef().withName(codeSystemDef.getName());
+        codesystemRef.setResultType(codeSystemDef.getResultType());
+        if (codesystemRef.getResultType() == null) {
+            // ERROR:
+            throw new IllegalArgumentException(String.format("Could not validate reference to codesystem %s because its definition contains errors.",
+                    codesystemRef.getName()));
+        }
+        return null;
+    }
+
+    public ParameterRef resolveParameterRef(ParameterDef theParameterDef) {
+        final ParameterRef parameterRef = of.createParameterRef().withName(theParameterDef.getName());
+        parameterRef.setResultType(parameterRef.getResultType());
+        return parameterRef;
+    }
+
+    public OperandRef resolveOperandRef(OperandDef operandDef) {
+        return (OperandRef)of.createOperandRef()
+                .withName(operandDef.getName())
+                .withResultType(operandDef.getResultType());
+    }
+
+    private static String formatMatchedMessage(MatchType matchType) {
+        switch (matchType) {
+            case EXACT:
+                return " with exact case matching.";
+            case CASE_IGNORED:
+                return " with case insensitive matching.";
+            default:
+                return " with invalid MatchType.";
+        }
+    }
+
+    private static String lookupElementWarning(Object element) {
+        // TODO:  this list is not exhaustive and may need to be updated
+        if (element instanceof ExpressionDef) {
+            return "[%s] resolved as an expression definition";
+        }
+        else if (element instanceof ParameterDef) {
+            return "[%s] resolved as a parameter";
+        }
+        else if (element instanceof ValueSetDef) {
+            return "[%s] resolved as a value set";
+        }
+        else if (element instanceof CodeSystemDef) {
+            return "[%s] resolved as a code system";
+        }
+        else if (element instanceof CodeDef) {
+            return "[%s] resolved as a code";
+        }
+        else if (element instanceof ConceptDef) {
+            return "[%s] resolved as a concept";
+        }
+        else if (element instanceof IncludeDef) {
+            return "[%s] resolved as a library";
+        }
+        else if (element instanceof AliasedQuerySource) {
+            return "[%s] resolved as an alias of a query";
+        }
+        else if (element instanceof LetClause) {
+            return "[%s] resolved as a let of a query";
+        }
+        else if (element instanceof OperandDef) {
+            return "[%s] resolved as an operand to a function";
+        }
+        else if (element instanceof UsingDef) {
+            return "[%s] resolved as a using definition";
+        }
+        //default message if no match is made:
+        return "[%s] resolved more than once: " + ((element != null) ? element.getClass() : "[null]");
+    }
+
     /**
      * An implicit context is one where the context has the same name as a parameter. Implicit contexts are used to
      * allow FHIRPath expressions to resolve on the implicit context of the expression
@@ -2935,6 +3043,60 @@ private DataType getExpressionDefResultType(ExpressionDef expressionDef) {
         throw new IllegalArgumentException(String.format("Invalid context reference from %s context to %s context.", currentExpressionContext(), expressionDef.getContext()));
     }
 
+    /**
+     * Add an identifier to the deque to indicate that we are considering it for consideration for identifier hiding and
+     * adding a compiler warning if this is the case.
+     * <p/>
+     * For example, if an alias within an expression body has the same name as a parameter, execution would have
+     * added the parameter identifier and the next execution would consider an alias with the same name, thus resulting
+     * in a warning.
+     * <p/>
+     * Exact case matching as well as case-insensitive matching are considered.  If known, the type of the structure
+     * in question will be considered in crafting the warning message, as per the {@link Element} parameter.
+     *
+     * @param identifier The identifier belonging to the parameter, expression, function, alias, etc, to be evaluated.
+     * @param onlyOnce Special case to deal with overloaded functions, which are out scope for hiding.
+     * @param element The consturct element, for {@link ExpressionRef}.
+     * @param nullableExpression Use strictly to comply with the signature for {@link #reportWarning(String, Expression)}.
+     *                           If the caller could not obtain an Element, it simply passes null and this is safe to do.
+     */
+    void pushIdentifierForHiding(String identifier, boolean onlyOnce, Element element, Expression nullableExpression) {
+        final MatchType matchType = identifiersToCheckForHiding.stream()
+                .map(innerIdentifier -> {
+                    if (innerIdentifier.equals(identifier)) {
+                        return MatchType.EXACT;
+                    }
+
+                    if (innerIdentifier.equalsIgnoreCase(identifier)) {
+                        return MatchType.CASE_IGNORED;
+                    }
+
+                    return MatchType.NONE;
+                })
+                .filter(innerMatchType -> MatchType.NONE != innerMatchType)
+                .findFirst()
+                .orElse(MatchType.NONE);
+
+        if (MatchType.NONE != matchType && ! onlyOnce) {
+            final String message = String.format("Identifier hiding detected: Identifier for identifiers: %s%s",
+                    String.format(lookupElementWarning(element), identifier),
+                    formatMatchedMessage(matchType)+ "\n");
+            reportWarning(message, nullableExpression);
+        }
+
+        if (! onlyOnce || MatchType.NONE == matchType) {
+            identifiersToCheckForHiding.push(identifier);
+        }
+    }
+
+    /**
+     * Pop the last resolved identifier off the deque.  This is needed in case of a context in which an identifier
+     * falls out of scope, for an example, an alias within an expression or function body
+     */
+    void popIdentifierForHiding() {
+        identifiersToCheckForHiding.pop();
+    }
+
     private class Scope {
         private final Stack<Expression> targets = new Stack<>();
         private final Stack<QueryContext> queries = new Stack<>();

Src/java/cql-to-elm/src/main/java/org/cqframework/cql/cql2elm/model/MatchType.java

@@ -0,0 +1,17 @@
+package org.cqframework.cql.cql2elm.model;
+
+public enum MatchType {
+    EXACT, CASE_IGNORED, NONE;
+
+    public static MatchType resolveMatchType(String val, String checkVal) {
+        if (val.equals(checkVal)) {
+            return EXACT;
+        }
+
+        if (val.equalsIgnoreCase(checkVal)) {
+            return CASE_IGNORED;
+        }
+
+        return NONE;
+    }
+}