Implement identifier hiding detection for String literals. Includes s…

…ome minor refactoring for the overall identifier hiding solution.

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

@@ -135,7 +135,7 @@ public UsingDef visitUsingDefinition(cqlParser.UsingDefinitionContext ctx) {
 
         // The model was already calculated by CqlPreprocessorVisitor
         final UsingDef usingDef = libraryBuilder.resolveUsingRef(localIdentifier);
-        libraryBuilder.pushIdentifierForHiding(localIdentifier, false, usingDef);
+        libraryBuilder.pushIdentifierForHiding(localIdentifier, usingDef);
         return usingDef;
     }
 
@@ -198,7 +198,7 @@ public Object visitIncludeDefinition(cqlParser.IncludeDefinitionContext ctx) {
         }
 
         libraryBuilder.addInclude(library);
-        libraryBuilder.pushIdentifierForHiding(library.getLocalIdentifier(), false, library);
+        libraryBuilder.pushIdentifierForHiding(library.getLocalIdentifier(), library);
 
         return library;
     }
@@ -235,7 +235,7 @@ public ParameterDef visitParameterDefinition(cqlParser.ParameterDefinitionContex
         }
 
         libraryBuilder.addParameter(param);
-        libraryBuilder.pushIdentifierForHiding(param.getName(), false, param);
+        libraryBuilder.pushIdentifierForHiding(param.getName(), param);
 
         return param;
     }
@@ -278,7 +278,7 @@ public CodeSystemDef visitCodesystemDefinition(cqlParser.CodesystemDefinitionCon
         }
 
         libraryBuilder.addCodeSystem(cs);
-        libraryBuilder.pushIdentifierForHiding(cs.getName(), false, cs);
+        libraryBuilder.pushIdentifierForHiding(cs.getName(), cs);
         return cs;
     }
 
@@ -350,7 +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.pushIdentifierForHiding(vs.getName(), vs);
 
         return vs;
     }
@@ -372,7 +372,7 @@ public CodeDef visitCodeDefinition(cqlParser.CodeDefinitionContext ctx) {
 
         cd.setResultType(libraryBuilder.resolveTypeName("Code"));
         libraryBuilder.addCode(cd);
-        libraryBuilder.pushIdentifierForHiding(cd.getName(), false, cd);
+        libraryBuilder.pushIdentifierForHiding(cd.getName(), cd);
 
         return cd;
     }
@@ -526,7 +526,7 @@ public ExpressionDef internalVisitExpressionDefinition(cqlParser.ExpressionDefin
         final ExpressionDef hollowExpressionDef = of.createExpressionDef()
                 .withName(identifier)
                 .withContext(getCurrentContext());
-        libraryBuilder.pushIdentifierForHiding(identifier, false, hollowExpressionDef);
+        libraryBuilder.pushIdentifierForHiding(identifier, hollowExpressionDef);
         if (def == null || isImplicitContextExpressionDef(def)) {
             if (def != null && isImplicitContextExpressionDef(def)) {
                 libraryBuilder.removeExpression(def);
@@ -575,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
@@ -3076,7 +3078,7 @@ public Object visitQuery(cqlParser.QueryContext ctx) {
             queryContext.addPrimaryQuerySources(sources);
 
             for (AliasedQuerySource source : sources) {
-                libraryBuilder.pushIdentifierForHiding(source.getAlias(), false, source);
+                libraryBuilder.pushIdentifierForHiding(source.getAlias(), source);
             }
 
             // If we are evaluating a population-level query whose source ranges over any patient-context expressions,
@@ -3095,7 +3097,7 @@ public Object visitQuery(cqlParser.QueryContext ctx) {
 
                 if (dfcx != null) {
                     for (LetClause letClause : dfcx) {
-                        libraryBuilder.pushIdentifierForHiding(letClause.getIdentifier(), false, letClause);
+                        libraryBuilder.pushIdentifierForHiding(letClause.getIdentifier(), letClause);
                     }
                 }
 
@@ -4051,10 +4053,10 @@ 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);
+        libraryBuilder.pushIdentifierForHiding(fun.getName(), fun);
         final List<OperandDef> operand = op.getFunctionDef().getOperand();
         for (OperandDef operandDef : operand) {
-            libraryBuilder.pushIdentifierForHiding(operandDef.getName(), false, operandDef);
+            libraryBuilder.pushIdentifierForHiding(operandDef.getName(), operandDef);
         }
 
         if (ctx.functionBody() != null) {

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

@@ -0,0 +1,52 @@
+package org.cqframework.cql.cql2elm;
+
+import org.cqframework.cql.elm.tracking.Trackable;
+
+import java.util.Objects;
+import java.util.StringJoiner;
+
+/**
+ * Simple POJO using for identifier hider that maintains the identifier and Trackable type of the construct being evaluated.
+ */
+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,6 +4,7 @@
 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;
@@ -100,7 +101,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 final Deque<HidingIdentifierContext> hidingIdentifiersContexts = new ArrayDeque<>();
     private int literalContext = 0;
     private int typeSpecifierContext = 0;
     private NamespaceInfo namespaceInfo = null;
@@ -1427,7 +1428,7 @@ private Expression convertListExpression(Expression expression, Conversion conve
         return query;
     }
 
-    private void reportWarning(String message, Element expression) {
+    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);
@@ -2379,6 +2380,9 @@ else if (element instanceof OperandDef) {
         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]";
     }
@@ -2984,26 +2988,33 @@ private DataType getExpressionDefResultType(ExpressionDef expressionDef) {
      * <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 onlyOnce   Special case to deal with overloaded functions, which are out scope for hiding.
-     * @param element    The construct element, for example {@link ExpressionRef}.
+     * @param trackable  The construct trackable, for example {@link ExpressionRef}.
      */
-    void pushIdentifierForHiding(String identifier, boolean onlyOnce, Element element) {
-        final boolean hasRelevantMatch = identifiersToCheckForHiding.stream()
-                .filter(innerIdentifier -> innerIdentifier.equals(identifier))
-                .peek(matchedIdentifier -> {
-                    if (onlyOnce) {
+    void pushIdentifierForHiding(String identifier, Trackable trackable) {
+        final boolean hasRelevantMatch = hidingIdentifiersContexts.stream()
+                .filter(innerContext -> innerContext.getIdentifier().equals(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;
                     }
 
-                    final String elementString = lookupElementWarning(element);
-                    final String message = String.format("%s identifier [%s] is hiding another identifier of the same name. %n", elementString, identifier);
-                    reportWarning(message, element);
+                    reportWarning(resolveWarningMessage(matchedContext.getIdentifier(), identifier, trackable), trackable);
                 }).findAny()
                 .isPresent();
-        if (! onlyOnce || ! hasRelevantMatch) {
-            identifiersToCheckForHiding.push(identifier);
+
+        // We will only add function definitions once
+        if (! (trackable instanceof FunctionDef) || ! hasRelevantMatch) {
+            if (shouldPushHidingContext(trackable)) {
+                final Class<? extends Trackable> trackableOrNull = trackable != null ? trackable.getClass() : null;
+                // Sometimes the underlying Trackable doesn't resolve in the calling code
+                hidingIdentifiersContexts.push(new HidingIdentifierContext(identifier, trackable != null ? trackable.getClass() : null));
+            }
         }
     }
 
@@ -3012,7 +3023,22 @@ void pushIdentifierForHiding(String identifier, boolean onlyOnce, Element elemen
      * falls out of scope, for an example, an alias within an expression or function body
      */
     void popIdentifierForHiding() {
-        identifiersToCheckForHiding.pop();
+        hidingIdentifiersContexts.pop();
+    }
+
+    // TODO:  consider other structures that should only trigger a readonly check of identifier hiding
+    private boolean shouldPushHidingContext(Trackable trackable) {
+        return ! (trackable instanceof Literal);
+    }
+
+    private String resolveWarningMessage(String matchedIdentifier, String identifierParam, Trackable trackable) {
+        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 String.format("%s identifier [%s] is hiding another identifier of the same name. %n", elementString, identifierParam);
     }
 
     private class Scope {

Src/java/cql-to-elm/src/test/java/org/cqframework/cql/cql2elm/HidingTests.java

@@ -34,20 +34,36 @@ public void testHidingUnionWithSameAlias() throws IOException {
         final CqlTranslator translator = TestUtils.runSemanticTestNoErrors("HidingTests/TestHidingUnionSameAlias.cql");
         final List<CqlCompilerException> warnings = translator.getWarnings();
 
-        assertThat(warnings.toString(), translator.getWarnings().size(), is(0));
+        final List<String> warningMessages = warnings.stream().map(Throwable::getMessage).collect(Collectors.toList());
+        assertThat(warningMessages.toString(), translator.getWarnings().size(), is(2));
+
+        final List<String> distinct = translator.getWarnings().stream().map(Throwable::getMessage).distinct().collect(Collectors.toList());
+
+        assertThat(distinct.size(), is(2));
+
+        final String first = "You used a string literal: [X] here that matches an identifier in scope: [X]. Did you mean to use the identifier instead? \n";
+        final String second = "You used a string literal: [Y] here that matches an identifier in scope: [Y]. Did you mean to use the identifier instead? \n";
+
+        assertThat(distinct.toString(), distinct, containsInAnyOrder(first, second));
     }
 
     @Test
     public void testHidingUnionWithSameAliasEachHides() throws IOException {
         final CqlTranslator translator = TestUtils.runSemanticTestNoErrors("HidingTests/TestHidingUnionSameAliasEachHides.cql");
         final List<CqlCompilerException> warnings = translator.getWarnings();
 
-        assertThat(warnings.toString(), translator.getWarnings().size(), is(2));
+        final List<String> warningMessages = warnings.stream().map(Throwable::getMessage).collect(Collectors.toList());
+        assertThat(warningMessages.toString(), translator.getWarnings().size(), is(4));
 
         final List<String> distinct = translator.getWarnings().stream().map(Throwable::getMessage).distinct().collect(Collectors.toList());
 
-        assertThat(distinct.size(), is(1));
-        assertThat(distinct, contains("An alias identifier [IWantToBeHidden] is hiding another identifier of the same name. \n"));
+        assertThat(distinct.size(), is(3));
+
+        final String first = "You used a string literal: [X] here that matches an identifier in scope: [X]. Did you mean to use the identifier instead? \n";
+        final String second = "You used a string literal: [Y] here that matches an identifier in scope: [Y]. Did you mean to use the identifier instead? \n";
+        final String third = "An alias identifier [IWantToBeHidden] is hiding another identifier of the same name. \n";
+
+        assertThat(distinct.toString(), distinct, containsInAnyOrder(first, second, third));
     }
 
     @Test
@@ -150,4 +166,19 @@ public void testHidingCommaMissingInListConstruction() throws IOException {
         assertThat(distinctWarningMessages.toString(), distinctWarningMessages.size(), is(1));
         assertThat(distinctWarningMessages, contains("An alias identifier [5] is hiding another identifier of the same name. \n"));
     }
+
+    @Test
+    public void testHidingStringLiteral() throws IOException {
+        final CqlTranslator translator = TestUtils.runSemanticTestNoErrors("HidingTests/TestHidingStringLiteral.cql");
+        final List<CqlCompilerException> warnings = translator.getWarnings();
+        final List<String> warningMessages = warnings.stream().map(Throwable::getMessage).collect(Collectors.toList());
+        assertThat(warningMessages.toString(), warnings.size(), is(3));
+
+        final List<String> distinctWarningMessages = warningMessages.stream().distinct().collect(Collectors.toList());
+        assertThat(distinctWarningMessages.toString(), distinctWarningMessages.size(), is(2));
+
+        final String stringLiteralIWantToBeHidden = "You used a string literal: [IWantToBeHidden] here that matches an identifier in scope: [IWantToBeHidden]. Did you mean to use the identifier instead? \n";
+        final String stringLiteralIWantToHide = "You used a string literal: [IWantToHide] here that matches an identifier in scope: [IWantToHide]. Did you mean to use the identifier instead? \n";
+        assertThat(distinctWarningMessages, containsInAnyOrder(stringLiteralIWantToBeHidden, stringLiteralIWantToHide));
+    }
 }