Release 3.6.0, HAPI 6.10.2, fixes for build warnings

Src/java/.vscode/settings.json

@@ -9,18 +9,23 @@
         "bools",
         "Bryn",
         "checkstyle",
+        "Codeable",
+        "Codesystem",
         "cqframework",
         "datumedge",
         "fhir",
         "fhirpath",
         "hamcrest",
         "Inferencing",
         "Instancio",
+        "JAXB",
+        "modelinfo",
         "Objenesis",
         "opdef",
         "opencds",
         "qicore",
         "Randomizer",
+        "redeclaration",
         "testng",
         "tngtech",
         "trackback",

Src/java/buildSrc/src/main/groovy/cql.sca-conventions.gradle

@@ -27,6 +27,7 @@ tasks.withType(JavaCompile).configureEach  {
 
 tasks {
     compileTestJava {
+        // TODO: Talk to the team about warnings in tests
         options.errorprone.disableAllWarnings = true
         options.errorprone.disableWarningsInGeneratedCode = true
     }

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

@@ -7,11 +7,11 @@
 /**
  * Simple POJO using for identifier hider that maintains the identifier and Trackable type of the construct being evaluated.
  */
-public class HidingIdentifierContext {
+public class IdentifierContext {
     private final String identifier;
     private final Class<? extends Trackable> elementSubclass;
 
-    public HidingIdentifierContext(String identifier, Class<? extends Trackable> elementSubclass) {
+    public IdentifierContext(String identifier, Class<? extends Trackable> elementSubclass) {
         this.identifier = identifier;
         this.elementSubclass = elementSubclass;
     }
@@ -32,7 +32,7 @@ public boolean equals(Object other) {
         if (other == null || getClass() != other.getClass()) {
             return false;
         }
-        HidingIdentifierContext that = (HidingIdentifierContext) other;
+        IdentifierContext that = (IdentifierContext) other;
         return Objects.equals(identifier, that.identifier) && Objects.equals(elementSubclass, that.elementSubclass);
     }
 
@@ -43,7 +43,7 @@ public int hashCode() {
 
     @Override
     public String toString() {
-        return new StringJoiner(", ", HidingIdentifierContext.class.getSimpleName() + "[", "]")
+        return new StringJoiner(", ", IdentifierContext.class.getSimpleName() + "[", "]")
                 .add("identifier='" + identifier + "'")
                 .add("elementSubclass=" + elementSubclass)
                 .toString();

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

@@ -20,7 +20,7 @@
  * Created by Bryn on 12/29/2016.
  */
 public class LibraryBuilder {
-    public static enum SignatureLevel {
+    public enum SignatureLevel {
         /*
         Indicates signatures will never be included in operator invocations
          */
@@ -116,7 +116,8 @@ public LibraryManager getLibraryManager() {
     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 final Deque<IdentifierContext> globalIdentifiers = new ArrayDeque<>();
+    private final Stack<Deque<IdentifierContext>> localIdentifierStack = new Stack<>();
     private int literalContext = 0;
     private int typeSpecifierContext = 0;
     private final NamespaceInfo namespaceInfo;
@@ -3127,6 +3128,11 @@ private DataType getExpressionDefResultType(ExpressionDef expressionDef) {
                 currentExpressionContext(), expressionDef.getContext()));
     }
 
+    public enum IdentifierScope {
+        GLOBAL,
+        LOCAL
+    }
+
     /**
      * 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.
@@ -3140,46 +3146,93 @@ private DataType getExpressionDefResultType(ExpressionDef expressionDef) {
      * <p/>
      * Also, special case function overloads so that only a single overloaded function name is taken into account.
      *
+     * Default scope is {@link IdentifierScope#LOCAL}
+     *
      * @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().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;
-                    }
+    void pushIdentifier(String identifier, Trackable trackable) {
+        pushIdentifier(identifier, trackable, IdentifierScope.LOCAL);
+    }
 
-                    reportWarning(
-                            resolveWarningMessage(matchedContext.getIdentifier(), identifier, trackable), trackable);
-                })
-                .findAny()
-                .isPresent();
+    /**
+     * 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}.
+     * @param scope the scope of the current identifier
+     */
+    void pushIdentifier(String identifier, Trackable trackable, IdentifierScope scope) {
+        var localMatch = !localIdentifierStack.isEmpty()
+                ? findMatchingIdentifierContext(localIdentifierStack.peek(), identifier)
+                : Optional.<IdentifierContext>empty();
+        var globalMatch = findMatchingIdentifierContext(globalIdentifiers, identifier);
+
+        if (globalMatch.isPresent() || localMatch.isPresent()) {
+            var matchedContext = globalMatch.isPresent() ? globalMatch.get() : localMatch.get();
+
+            boolean matchedOnFunctionOverloads =
+                    matchedContext.getTrackableSubclass().equals(FunctionDef.class) && trackable instanceof FunctionDef;
+
+            if (!matchedOnFunctionOverloads) {
+                reportWarning(resolveWarningMessage(matchedContext.getIdentifier(), identifier, trackable), trackable);
+            }
+        }
 
-        // 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));
+        if (shouldAddIdentifierContext(trackable)) {
+            final Class<? extends Trackable> trackableOrNull = trackable != null ? trackable.getClass() : null;
+            // Sometimes the underlying Trackable doesn't resolve in the calling code
+            if (scope == IdentifierScope.GLOBAL) {
+                globalIdentifiers.push(new IdentifierContext(identifier, trackableOrNull));
+            } else {
+                localIdentifierStack.peek().push(new IdentifierContext(identifier, trackableOrNull));
             }
         }
     }
 
+    private Optional<IdentifierContext> findMatchingIdentifierContext(
+            Collection<IdentifierContext> identifierContext, String identifier) {
+        return identifierContext.stream()
+                .filter(innerContext -> innerContext.getIdentifier().equals(identifier))
+                .findFirst();
+    }
+
     /**
      * 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
+     * falls out of scope, for an example, an alias within an expression or function body.
      */
-    void popIdentifierForHiding() {
-        hidingIdentifiersContexts.pop();
+    void popIdentifier() {
+        popIdentifier(IdentifierScope.LOCAL);
+    }
+
+    void popIdentifier(IdentifierScope scope) {
+        if (scope == IdentifierScope.GLOBAL) {
+            globalIdentifiers.pop();
+        } else {
+            localIdentifierStack.peek().pop();
+        }
+    }
+
+    void pushIdentifierScope() {
+        localIdentifierStack.push(new ArrayDeque<>());
+    }
+
+    void popIdentifierScope() {
+        localIdentifierStack.pop();
     }
 
     // TODO:  consider other structures that should only trigger a readonly check of identifier hiding
-    private boolean shouldPushHidingContext(Trackable trackable) {
+    private boolean shouldAddIdentifierContext(Trackable trackable) {
         return !(trackable instanceof Literal);
     }
 

Src/java/cql-to-elm/src/test/java/org/cqframework/cql/cql2elm/fhir/r4/BaseTest.java

@@ -5,12 +5,14 @@
 import static org.cqframework.cql.cql2elm.matchers.QuickDataType.quickDataType;
 import static org.hamcrest.MatcherAssert.assertThat;
 import static org.hamcrest.Matchers.*;
+import static org.junit.Assert.assertEquals;
 
 import java.io.IOException;
 import java.util.HashMap;
 import java.util.Map;
 import org.cqframework.cql.cql2elm.CqlCompilerOptions;
 import org.cqframework.cql.cql2elm.CqlTranslator;
+import org.cqframework.cql.cql2elm.LibraryBuilder.SignatureLevel;
 import org.cqframework.cql.cql2elm.TestUtils;
 import org.cqframework.cql.cql2elm.model.CompiledLibrary;
 import org.hl7.elm.r1.*;
@@ -323,4 +325,11 @@ public void testRetrieveWithConcept() throws IOException {
         ToList toList = (ToList) retrieve.getCodes();
         assertThat(toList.getOperand(), instanceOf(CodeRef.class));
     }
+
+    @Test
+    public void testExm108IdentifierHiding() throws IOException {
+        var translator = TestUtils.runSemanticTest("fhir/r4/exm108/EXM108.cql", 0, SignatureLevel.All);
+        // Should only be one identifier being hid after fixes, "Warafin"
+        assertEquals(1, translator.getExceptions().size());
+    }
 }