1254 string literal 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, usingDef);
+        return usingDef;
     }
 
     public Model getModel() {
@@ -196,6 +198,7 @@ public Object visitIncludeDefinition(cqlParser.IncludeDefinitionContext ctx) {
         }
 
         libraryBuilder.addInclude(library);
+        libraryBuilder.pushIdentifierForHiding(library.getLocalIdentifier(), library);
 
         return library;
     }
@@ -232,6 +235,7 @@ public ParameterDef visitParameterDefinition(cqlParser.ParameterDefinitionContex
         }
 
         libraryBuilder.addParameter(param);
+        libraryBuilder.pushIdentifierForHiding(param.getName(), param);
 
         return param;
     }
@@ -274,6 +278,7 @@ public CodeSystemDef visitCodesystemDefinition(cqlParser.CodesystemDefinitionCon
         }
 
         libraryBuilder.addCodeSystem(cs);
+        libraryBuilder.pushIdentifierForHiding(cs.getName(), 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(), 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(), 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 hollowExpressionDef = of.createExpressionDef()
+                .withName(identifier)
+                .withContext(getCurrentContext());
+        libraryBuilder.pushIdentifierForHiding(identifier, hollowExpressionDef);
         if (def == null || isImplicitContextExpressionDef(def)) {
             if (def != null && isImplicitContextExpressionDef(def)) {
                 libraryBuilder.removeExpression(def);
@@ -562,7 +575,9 @@ public ExpressionDef visitExpressionDefinition(cqlParser.ExpressionDefinitionCon
 
     @Override
     public Literal visitStringLiteral(cqlParser.StringLiteralContext ctx) {
-        return libraryBuilder.createLiteral(parseString(ctx.STRING()));
+        final Literal stringLiteral = libraryBuilder.createLiteral(parseString(ctx.STRING()));
+        libraryBuilder.pushIdentifierForHiding(stringLiteral.getValue(), stringLiteral);
+        return stringLiteral;
     }
 
     @Override
@@ -3049,9 +3064,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 +3077,10 @@ public Object visitQuery(cqlParser.QueryContext ctx) {
 
             queryContext.addPrimaryQuerySources(sources);
 
+            for (AliasedQuerySource source : sources) {
+                libraryBuilder.pushIdentifierForHiding(source.getAlias(), source);
+            }
+
             // 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 +3091,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(), letClause);
+                    }
+                }
 
                 List<RelationshipClause> qicx = new ArrayList<>();
                 if (ctx.queryInclusionClause() != null) {
@@ -3180,10 +3205,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 +4053,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(), fun);
+        final List<OperandDef> operand = op.getFunctionDef().getOperand();
+        for (OperandDef operandDef : operand) {
+            libraryBuilder.pushIdentifierForHiding(operandDef.getName(), operandDef);
+        }
 
         if (ctx.functionBody() != null) {
             libraryBuilder.beginFunctionDef(fun);
@@ -4033,6 +4074,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/HidingIdentifierContext.java

@@ -0,0 +1,49 @@
+package org.cqframework.cql.cql2elm;
+
+import org.cqframework.cql.elm.tracking.Trackable;
+
+import java.util.Objects;
+import java.util.StringJoiner;
+
+public class HidingIdentifierContext {
+    private final String identifier;
+    private final Class<? extends Trackable> elementSubclass;
+
+    public HidingIdentifierContext(String theIdentifier, Class<? extends Trackable> theElementSubclass) {
+        identifier = theIdentifier;
+        elementSubclass = theElementSubclass;
+    }
+
+    public String getIdentifier() {
+        return identifier;
+    }
+
+    public Class<? extends Trackable> getTrackableSubclass() {
+        return elementSubclass;
+    }
+
+    @Override
+    public boolean equals(Object theO) {
+        if (this == theO) {
+            return true;
+        }
+        if (theO == null || getClass() != theO.getClass()) {
+            return false;
+        }
+        HidingIdentifierContext that = (HidingIdentifierContext) theO;
+        return Objects.equals(identifier, that.identifier) && Objects.equals(elementSubclass, that.elementSubclass);
+    }
+
+    @Override
+    public int hashCode() {
+        return Objects.hash(identifier, elementSubclass);
+    }
+
+    @Override
+    public String toString() {
+        return new StringJoiner(", ", HidingIdentifierContext.class.getSimpleName() + "[", "]")
+                .add("identifier='" + identifier + "'")
+                .add("elementSubclass=" + elementSubclass)
+                .toString();
+    }
+}

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

@@ -4,17 +4,18 @@
 import org.cqframework.cql.cql2elm.model.*;
 import org.cqframework.cql.cql2elm.model.invocation.*;
 import org.cqframework.cql.elm.tracking.TrackBack;
+import org.cqframework.cql.elm.tracking.Trackable;
 import org.hl7.cql.model.*;
 import org.hl7.cql_annotations.r1.CqlToElmError;
 import org.hl7.cql_annotations.r1.CqlToElmInfo;
 import org.hl7.cql_annotations.r1.ErrorSeverity;
 import org.hl7.cql_annotations.r1.ErrorType;
 import org.hl7.elm.r1.*;
+
 import javax.xml.namespace.QName;
 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 +102,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<HidingIdentifierContext> hidingIdentifiersContexts = new ArrayDeque<>();
     private int literalContext = 0;
     private int typeSpecifierContext = 0;
     private NamespaceInfo namespaceInfo = null;
@@ -1427,8 +1429,8 @@ private Expression convertListExpression(Expression expression, Conversion conve
         return query;
     }
 
-    private void reportWarning(String message, Expression expression) {
-        TrackBack trackback = expression.getTrackbacks() != null && expression.getTrackbacks().size() > 0 ? expression.getTrackbacks().get(0) : null;
+    private void reportWarning(String message, Trackable expression) {
+        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 +2346,48 @@ public Expression resolveIdentifier(String identifier, boolean mustResolve) {
         return null;
     }
 
+    private static String lookupElementWarning(Object element) {
+        // TODO:  this list is not exhaustive and may need to be updated
+        if (element instanceof ExpressionDef) {
+            return "An expression";
+        }
+        else if (element instanceof ParameterDef) {
+            return "A parameter";
+        }
+        else if (element instanceof ValueSetDef) {
+            return "A valueset";
+        }
+        else if (element instanceof CodeSystemDef) {
+            return "A codesystem";
+        }
+        else if (element instanceof CodeDef) {
+            return "A code";
+        }
+        else if (element instanceof ConceptDef) {
+            return "A concept";
+        }
+        else if (element instanceof IncludeDef) {
+            return "An include";
+        }
+        else if (element instanceof AliasedQuerySource) {
+            return "An alias";
+        }
+        else if (element instanceof LetClause) {
+            return "A let";
+        }
+        else if (element instanceof OperandDef) {
+            return "An operand";
+        }
+        else if (element instanceof UsingDef) {
+            return "A using";
+        }
+        else if (element instanceof Literal) {
+            return "A literal";
+        }
+        //default message if no match is made:
+        return "An [unknown structure]";
+    }
+
     /**
      * 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 +2979,67 @@ 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.
+     * <p/>
+     * Also, special case function overloads so that only a single overloaded function name is taken into account.
+     *
+     * @param identifier The identifier belonging to the parameter, expression, function, alias, etc, to be evaluated.
+     * @param trackable  The construct trackable, for example {@link ExpressionRef}.
+     */
+    void pushIdentifierForHiding(String identifier, Trackable trackable) {
+        final boolean hasRelevantMatch = hidingIdentifiersContexts.stream()
+                .filter(innerContext -> innerContext.getIdentifier().equalsIgnoreCase(identifier))
+                .peek(matchedContext -> {
+                    // If we are passing an overloaded function definition, do not issue a warning
+                    if (trackable instanceof FunctionDef &&
+                            FunctionDef.class == matchedContext.getTrackableSubclass()) {
+                        return;
+                    }
+
+                    reportWarning(resolveWarningMessage(matchedContext.getIdentifier(), identifier, trackable), trackable);
+                }).findAny()
+                .isPresent();
+
+        // We will only add function definitions once
+        if (! (trackable instanceof FunctionDef) || ! hasRelevantMatch) {
+            // Sometimes the underlying Trackable doesn't resolve in the calling code
+            final Class<? extends Trackable> trackableOrNull = trackable != null ? trackable.getClass() : null;
+            if (! (trackable instanceof Literal)) {
+                hidingIdentifiersContexts.push(new HidingIdentifierContext(identifier, trackableOrNull));
+            }
+        }
+    }
+
+    private String resolveWarningMessage(String matchedIdentifier, String identifierParam, Trackable trackable) {
+        final boolean isExactMatch = matchedIdentifier.equals(identifierParam);
+        final String elementString = lookupElementWarning(trackable);
+
+        if (trackable instanceof Literal) {
+            return String.format("You used a string literal: [%s] here that matches an identifier in scope: [%s]. Did you mean to use the identifier instead? %n", identifierParam, matchedIdentifier);
+        }
+
+        return isExactMatch
+                ? String.format("%s identifier [%s] is hiding another identifier of the same name. %n", elementString, identifierParam)
+                : String.format("Are you sure you mean to use %s identifier [%s], instead of [%s]? %n", elementString.toLowerCase(), identifierParam, matchedIdentifier);
+    }
+
+    /**
+     * 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() {
+        hidingIdentifiersContexts.pop();
+    }
+
     private class Scope {
         private final Stack<Expression> targets = new Stack<>();
         private final Stack<QueryContext> queries = new Stack<>();