From b1b2854f80fdb5b0d342cccc92bb4a51058223aa Mon Sep 17 00:00:00 2001 From: Luke deGruchy Date: Thu, 9 Nov 2023 15:00:41 -0500 Subject: [PATCH 1/5] Full implementation of detection of identifier hiding, including exact and case-insensitive matches as well as a lot of new unit tests. --- .../cql/cql2elm/Cql2ElmVisitor.java | 50 +++++- .../cql/cql2elm/LibraryBuilder.java | 166 +++++++++++++++++- .../cql/cql2elm/model/MatchType.java | 17 ++ .../cqframework/cql/cql2elm/HidingTests.java | 147 ++++++++++++++++ .../cql/cql2elm/SemanticTests.java | 13 +- .../cqframework/cql/cql2elm/TestUtils.java | 18 +- .../cql/cql2elm/TranslationTests.java | 45 +++-- .../cql/cql2elm/qicore/v411/BaseTest.java | 18 +- .../cql/cql2elm/qicore/v500/BaseTest.java | 18 +- .../TestHiddenIdentifierArgumentToAlias.cql | 6 + .../TestHiddenIdentifierFromReturn.cql | 9 + .../TestHidingCaseInsensitiveWarning.cql | 12 ++ ...tHidingFunctionDefinitionWithOverloads.cql | 7 + .../TestHidingIncludeDefinition.cql | 9 + .../HidingTests/TestHidingLetAlias.cql | 6 + .../TestHidingParameterDefinition.cql | 7 + ...nArgumentNotConsideredHiddenIdentifier.cql | 3 + .../TestHidingSoMuchNestingHidingComplex.cql | 22 +++ .../TestHidingSoMuchNestingHidingSimple.cql | 6 + .../TestHidingSoMuchNestingNormal.cql | 26 +++ .../HidingTests/TestHidingUnionSameAlias.cql | 7 + .../TestHidingUnionSameAliasEachHides.cql | 8 + .../HidingTests/TestHidingVariousUseCases.cql | 60 +++++++ .../cql2elm/TestIdentifierCaseMismatch.cql | 6 + .../TestIncorrectParameterType1204.cql | 3 +- 25 files changed, 661 insertions(+), 28 deletions(-) create mode 100644 Src/java/cql-to-elm/src/main/java/org/cqframework/cql/cql2elm/model/MatchType.java create mode 100644 Src/java/cql-to-elm/src/test/java/org/cqframework/cql/cql2elm/HidingTests.java create mode 100644 Src/java/cql-to-elm/src/test/resources/org/cqframework/cql/cql2elm/HidingTests/TestHiddenIdentifierArgumentToAlias.cql create mode 100644 Src/java/cql-to-elm/src/test/resources/org/cqframework/cql/cql2elm/HidingTests/TestHiddenIdentifierFromReturn.cql create mode 100644 Src/java/cql-to-elm/src/test/resources/org/cqframework/cql/cql2elm/HidingTests/TestHidingCaseInsensitiveWarning.cql create mode 100644 Src/java/cql-to-elm/src/test/resources/org/cqframework/cql/cql2elm/HidingTests/TestHidingFunctionDefinitionWithOverloads.cql create mode 100644 Src/java/cql-to-elm/src/test/resources/org/cqframework/cql/cql2elm/HidingTests/TestHidingIncludeDefinition.cql create mode 100644 Src/java/cql-to-elm/src/test/resources/org/cqframework/cql/cql2elm/HidingTests/TestHidingLetAlias.cql create mode 100644 Src/java/cql-to-elm/src/test/resources/org/cqframework/cql/cql2elm/HidingTests/TestHidingParameterDefinition.cql create mode 100644 Src/java/cql-to-elm/src/test/resources/org/cqframework/cql/cql2elm/HidingTests/TestHidingReturnArgumentNotConsideredHiddenIdentifier.cql create mode 100644 Src/java/cql-to-elm/src/test/resources/org/cqframework/cql/cql2elm/HidingTests/TestHidingSoMuchNestingHidingComplex.cql create mode 100644 Src/java/cql-to-elm/src/test/resources/org/cqframework/cql/cql2elm/HidingTests/TestHidingSoMuchNestingHidingSimple.cql create mode 100644 Src/java/cql-to-elm/src/test/resources/org/cqframework/cql/cql2elm/HidingTests/TestHidingSoMuchNestingNormal.cql create mode 100644 Src/java/cql-to-elm/src/test/resources/org/cqframework/cql/cql2elm/HidingTests/TestHidingUnionSameAlias.cql create mode 100644 Src/java/cql-to-elm/src/test/resources/org/cqframework/cql/cql2elm/HidingTests/TestHidingUnionSameAliasEachHides.cql create mode 100644 Src/java/cql-to-elm/src/test/resources/org/cqframework/cql/cql2elm/HidingTests/TestHidingVariousUseCases.cql create mode 100644 Src/java/cql-to-elm/src/test/resources/org/cqframework/cql/cql2elm/TestIdentifierCaseMismatch.cql diff --git a/Src/java/cql-to-elm/src/main/java/org/cqframework/cql/cql2elm/Cql2ElmVisitor.java b/Src/java/cql-to-elm/src/main/java/org/cqframework/cql/cql2elm/Cql2ElmVisitor.java index 83459ae18..b81d4ffc0 100755 --- a/Src/java/cql-to-elm/src/main/java/org/cqframework/cql/cql2elm/Cql2ElmVisitor.java +++ b/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 sources = null; try { - List sources; queryContext.enterSourceClause(); try { sources = (List)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 dfcx = null; try { + dfcx = ctx.letClause() != null ? (List) visit(ctx.letClause()) : null; - List dfcx = ctx.letClause() != null ? (List) visit(ctx.letClause()) : null; + if (dfcx != null) { + for (LetClause letClause : dfcx) { + libraryBuilder.pushIdentifierForHiding(letClause.getIdentifier(), false, letClause, libraryBuilder.getQueryLetRef(letClause)); + } + } List 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 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(); } diff --git a/Src/java/cql-to-elm/src/main/java/org/cqframework/cql/cql2elm/LibraryBuilder.java b/Src/java/cql-to-elm/src/main/java/org/cqframework/cql/cql2elm/LibraryBuilder.java index ec0390a2c..56a17d755 100644 --- a/Src/java/cql-to-elm/src/main/java/org/cqframework/cql/cql2elm/LibraryBuilder.java +++ b/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 getExceptions() { private final Stack expressionContext = new Stack<>(); private final ExpressionDefinitionContextStack expressionDefinitions = new ExpressionDefinitionContextStack(); private final Stack functionDefs = new Stack<>(); + private final Deque 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. + *

+ * 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. + *

+ * 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 targets = new Stack<>(); private final Stack queries = new Stack<>(); diff --git a/Src/java/cql-to-elm/src/main/java/org/cqframework/cql/cql2elm/model/MatchType.java b/Src/java/cql-to-elm/src/main/java/org/cqframework/cql/cql2elm/model/MatchType.java new file mode 100644 index 000000000..13ed734b9 --- /dev/null +++ b/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; + } +} \ No newline at end of file diff --git a/Src/java/cql-to-elm/src/test/java/org/cqframework/cql/cql2elm/HidingTests.java b/Src/java/cql-to-elm/src/test/java/org/cqframework/cql/cql2elm/HidingTests.java new file mode 100644 index 000000000..7eb7fd151 --- /dev/null +++ b/Src/java/cql-to-elm/src/test/java/org/cqframework/cql/cql2elm/HidingTests.java @@ -0,0 +1,147 @@ +package org.cqframework.cql.cql2elm; + +import org.testng.annotations.Test; + +import java.io.IOException; +import java.util.List; +import java.util.Set; +import java.util.stream.Collectors; + +import static org.hamcrest.MatcherAssert.assertThat; +import static org.hamcrest.Matchers.*; + +public class HidingTests { + + @Test + public void testCaseInsensitiveWarning() throws IOException { + final CqlTranslator translator = TestUtils.runSemanticTest("HidingTests/TestHidingCaseInsensitiveWarning.cql", 0, LibraryBuilder.SignatureLevel.All); + final List warnings = translator.getWarnings(); + assertThat(warnings.toString(), translator.getWarnings().size(), is(1)); + final Set warningMessages = warnings.stream().map(Throwable::getMessage).collect(Collectors.toSet()); + assertThat(warningMessages, contains("Identifier hiding detected: Identifier for identifiers: [patients] resolved as an expression definition with case insensitive matching.\n")); + } + + @Test + public void testHiddenIdentifierFromReturn() throws IOException { + final CqlTranslator translator = TestUtils.runSemanticTestNoErrors("HidingTests/TestHiddenIdentifierFromReturn.cql"); + final List warnings = translator.getWarnings(); + + assertThat(warnings.toString(), translator.getWarnings().size(), is(1)); + final Set warningMessages = warnings.stream().map(Throwable::getMessage).collect(Collectors.toSet()); + assertThat(warningMessages, contains("Identifier hiding detected: Identifier for identifiers: [var] resolved as a let of a query with exact case matching.\n")); + } + + @Test + public void testHidingUnionWithSameAlias() throws IOException { + final CqlTranslator translator = TestUtils.runSemanticTestNoErrors("HidingTests/TestHidingUnionSameAlias.cql"); + final List warnings = translator.getWarnings(); + + assertThat(warnings.toString(), translator.getWarnings().size(), is(0)); + } + + @Test + public void testHidingUnionWithSameAliasEachHides() throws IOException { + final CqlTranslator translator = TestUtils.runSemanticTestNoErrors("HidingTests/TestHidingUnionSameAliasEachHides.cql"); + final List warnings = translator.getWarnings(); + + assertThat(warnings.toString(), translator.getWarnings().size(), is(2)); + + final List distinct = translator.getWarnings().stream().map(Throwable::getMessage).distinct().collect(Collectors.toList()); + + assertThat(distinct.size(), is(1)); + + final String first = "Identifier hiding detected: Identifier for identifiers: [IWantToBeHidden] resolved as an alias of a query with exact case matching.\n"; + + assertThat(distinct, containsInAnyOrder(first)); + } + + @Test + public void testSoMuchNestingNormal() throws IOException { + final CqlTranslator translator = TestUtils.runSemanticTestNoErrors("HidingTests/TestHidingSoMuchNestingNormal.cql"); + final List warnings = translator.getWarnings(); + + assertThat(warnings.toString(), translator.getWarnings().size(), is(0)); + } + + @Test + public void testSoMuchNestingHidingSimple() throws IOException { + final CqlTranslator translator = TestUtils.runSemanticTestNoErrors("HidingTests/TestHidingSoMuchNestingHidingSimple.cql"); + final List warnings = translator.getWarnings(); + + assertThat(warnings.toString(), translator.getWarnings().size(), is(1)); + assertThat(warnings.stream().map(Throwable::getMessage).collect(Collectors.toList()), containsInAnyOrder("Identifier hiding detected: Identifier for identifiers: [SoMuchNesting] resolved as an alias of a query with exact case matching.\n")); + } + + @Test + public void testSoMuchNestingHidingComplex() throws IOException { + final CqlTranslator translator = TestUtils.runSemanticTestNoErrors("HidingTests/TestHidingSoMuchNestingHidingComplex.cql"); + final List warnings = translator.getWarnings(); + + final List collect = warnings.stream().map(Throwable::getMessage).collect(Collectors.toList()); + assertThat(collect.toString(), translator.getWarnings().size(), is(2)); + + final List distinct = translator.getWarnings().stream().map(Throwable::getMessage).distinct().collect(Collectors.toList()); + + assertThat(distinct.size(), is(2)); + + final String first = "Identifier hiding detected: Identifier for identifiers: [SoMuchNesting] resolved as an alias of a query with exact case matching.\n"; + final String second = "Identifier hiding detected: Identifier for identifiers: [SoMuchNesting] resolved as a let of a query with exact case matching.\n"; + + assertThat(distinct, containsInAnyOrder(first, second)); + } + + @Test + public void testHidingLetAlias() throws IOException { + final CqlTranslator translator = TestUtils.runSemanticTestNoErrors("HidingTests/TestHidingLetAlias.cql"); + final List warnings = translator.getWarnings(); + + final List warningMessages = warnings.stream().map(Throwable::getMessage).collect(Collectors.toList()); + assertThat(warningMessages.toString(), translator.getWarnings().size(), is(1)); + assertThat(warningMessages, contains("Identifier hiding detected: Identifier for identifiers: [Alias] resolved as a let of a query with exact case matching.\n")); + } + + @Test + public void testHiddenIdentifierArgumentToAlias() throws IOException { + final CqlTranslator translator = TestUtils.runSemanticTestNoErrors("HidingTests/TestHiddenIdentifierArgumentToAlias.cql"); + + assertThat(translator.getWarnings().size(), is(1)); + assertThat(translator.getWarnings() + .stream() + .map(Throwable::getMessage) + .collect(Collectors.toList()), + contains("Identifier hiding detected: Identifier for identifiers: [testOperand] resolved as an alias of a query with exact case matching.\n")); + } + + @Test + public void testReturnArgumentNotConsideredHiddenIdentifier() throws IOException { + final CqlTranslator translator = TestUtils.runSemanticTestNoErrors("HidingTests/TestHidingReturnArgumentNotConsideredHiddenIdentifier.cql"); + assertThat(translator.getWarnings().size(), is(0)); + } + + @Test + public void testHidingFunctionDefinitionWithOverloads() throws IOException { + final CqlTranslator translator = TestUtils.runSemanticTestNoErrors("HidingTests/TestHidingFunctionDefinitionWithOverloads.cql"); + final List warnings = translator.getWarnings(); + final List warningMessages = warnings.stream().map(Throwable::getMessage).collect(Collectors.toList()); + assertThat(warningMessages.toString(), warnings.size(), is(1)); + assertThat(warningMessages, contains("Identifier hiding detected: Identifier for identifiers: [IWantToBeHidden] resolved as an alias of a query with exact case matching.\n")); + } + + @Test + public void testHidingParameterDefinition() throws IOException { + final CqlTranslator translator = TestUtils.runSemanticTestNoErrors("HidingTests/TestHidingParameterDefinition.cql"); + final List warnings = translator.getWarnings(); + final List warningMessages = warnings.stream().map(Throwable::getMessage).collect(Collectors.toList()); + assertThat(warningMessages.toString(), warnings.size(), is(1)); + assertThat(warningMessages, contains("Identifier hiding detected: Identifier for identifiers: [Measurement Period] resolved as an alias of a query with exact case matching.\n")); + } + + @Test + public void testHidingIncludeDefinition() throws IOException { + final CqlTranslator translator = TestUtils.runSemanticTestNoErrors("HidingTests/TestHidingIncludeDefinition.cql"); + final List warnings = translator.getWarnings(); + final List warningMessages = warnings.stream().map(Throwable::getMessage).collect(Collectors.toList()); + assertThat(warningMessages.toString(), warnings.size(), is(1)); + assertThat(warningMessages, contains("Identifier hiding detected: Identifier for identifiers: [FHIRHelpers] resolved as an alias of a query with exact case matching.\n")); + } +} diff --git a/Src/java/cql-to-elm/src/test/java/org/cqframework/cql/cql2elm/SemanticTests.java b/Src/java/cql-to-elm/src/test/java/org/cqframework/cql/cql2elm/SemanticTests.java index c0093dc27..6e811a533 100644 --- a/Src/java/cql-to-elm/src/test/java/org/cqframework/cql/cql2elm/SemanticTests.java +++ b/Src/java/cql-to-elm/src/test/java/org/cqframework/cql/cql2elm/SemanticTests.java @@ -12,6 +12,7 @@ import java.util.HashMap; import java.util.List; import java.util.Map; +import java.util.stream.Collectors; import static org.hamcrest.MatcherAssert.assertThat; import static org.hamcrest.Matchers.*; @@ -652,7 +653,17 @@ public void testIncorrectParameterType1204() throws IOException { final List errors = translator.getErrors(); - assertTrue(errors.stream().map(Throwable::getMessage).anyMatch("Could not find type for model: FHIR and name: Code"::equals)); + assertTrue(errors.stream().map(Throwable::getMessage).collect(Collectors.toSet()).toString(), errors.stream().map(Throwable::getMessage).anyMatch("Could not find type for model: null and name: ThisTypeDoesNotExist"::equals)); + } + + @Test + public void testIdentifierCaseMismatch() throws IOException { + final CqlTranslator translator = runSemanticTest("TestIdentifierCaseMismatch.cql", 2); + + final List errors = translator.getErrors(); + + // Make it clear we treat a Library type with a mismatched case the same as a non-existent type + assertTrue(errors.stream().map(Throwable::getMessage).collect(Collectors.toSet()).toString(), errors.stream().map(Throwable::getMessage).anyMatch("Could not find type for model: FHIR and name: Code"::equals)); } @Test diff --git a/Src/java/cql-to-elm/src/test/java/org/cqframework/cql/cql2elm/TestUtils.java b/Src/java/cql-to-elm/src/test/java/org/cqframework/cql/cql2elm/TestUtils.java index 461a23fe8..26a6fea07 100644 --- a/Src/java/cql-to-elm/src/test/java/org/cqframework/cql/cql2elm/TestUtils.java +++ b/Src/java/cql-to-elm/src/test/java/org/cqframework/cql/cql2elm/TestUtils.java @@ -123,6 +123,10 @@ public static CqlTranslator runSemanticTest(String testFileName, int expectedErr return runSemanticTest(null, testFileName, expectedErrors, options); } + public static CqlTranslator runSemanticTestNoErrors(String testFileName, CqlCompilerOptions.Options... options) throws IOException { + return runSemanticTest(null, testFileName, 0, options); + } + public static CqlTranslator runSemanticTest(String testFileName, int expectedErrors, SignatureLevel nullableSignatureLevel, CqlCompilerOptions.Options... options) throws IOException { final CqlCompilerOptions cqlCompilerOptions = new CqlCompilerOptions(options); Optional.ofNullable(nullableSignatureLevel).ifPresent(cqlCompilerOptions::setSignatureLevel); @@ -150,7 +154,19 @@ public static CqlTranslator runSemanticTest(NamespaceInfo namespaceInfo, String System.err.printf("(%d,%d): %s%n", error.getLocator().getStartLine(), error.getLocator().getStartChar(), error.getMessage()); } - assertThat(translator.getErrors().size(), is(expectedErrors)); + // We want to defer asserting on errors to the unit test by passing -1 + if (expectedErrors != -1) { + assertThat(translator.getErrors().toString(), translator.getErrors().size(), is(expectedErrors)); + } + return translator; + } + + public static CqlTranslator runSemanticTestWithOrWithoutErrors(NamespaceInfo namespaceInfo, String testFileName, CqlCompilerOptions.Options... options) throws IOException { + CqlTranslator translator = createTranslator(namespaceInfo, testFileName, new CqlCompilerOptions(options)); + for (CqlCompilerException error : translator.getErrors()) { + System.err.printf("(%d,%d): %s%n", + error.getLocator().getStartLine(), error.getLocator().getStartChar(), error.getMessage()); + } return translator; } diff --git a/Src/java/cql-to-elm/src/test/java/org/cqframework/cql/cql2elm/TranslationTests.java b/Src/java/cql-to-elm/src/test/java/org/cqframework/cql/cql2elm/TranslationTests.java index 2d0c2a297..4704378cb 100644 --- a/Src/java/cql-to-elm/src/test/java/org/cqframework/cql/cql2elm/TranslationTests.java +++ b/Src/java/cql-to-elm/src/test/java/org/cqframework/cql/cql2elm/TranslationTests.java @@ -14,6 +14,7 @@ import java.util.List; import java.util.Scanner; import java.util.concurrent.CompletableFuture; +import java.util.stream.Collectors; import static org.hamcrest.MatcherAssert.assertThat; import static org.hamcrest.Matchers.*; @@ -264,21 +265,45 @@ public void testForwardDeclarationSameTypeDifferentNamespaceGenericTypes() throw public void multiThreadedTranslation() throws IOException { List> futures = new ArrayList<>(); for (int i = 0; i < 10; i++) { - futures.add(CompletableFuture.runAsync(this::createTranslator)); + futures.add(CompletableFuture.runAsync(() -> { + try { + TestUtils.createTranslator("CMS146v2_Test_CQM.cql"); + } + catch(IOException e) { + throw new RuntimeException(e); + } + })); } - @SuppressWarnings("rawtypes") - CompletableFuture[] cfs = futures.toArray(new CompletableFuture[futures.size()]); + CompletableFuture[] cfs = futures.toArray(new CompletableFuture[0]); CompletableFuture.allOf(cfs).join(); } - private CqlTranslator createTranslator() { - try { - return TestUtils.createTranslator("CMS146v2_Test_CQM.cql"); - } - catch(IOException e) { - throw new RuntimeException(e); - } + @Test + public void testHidingVariousUseCases() throws IOException { + final CqlTranslator translator = TestUtils.runSemanticTest("HidingTests/TestHidingVariousUseCases.cql", 0); + final List warnings = translator.getWarnings(); + final List warningMessages = warnings.stream().map(Throwable::getMessage).collect(Collectors.toList()); + + assertThat(warningMessages.toString(), translator.getWarnings().size(), is(13)); + + final List distinct = warningMessages.stream().distinct().collect(Collectors.toList()); + + assertThat(warningMessages.toString(), distinct.size(), is(11)); + + final String hidingDefinition = "Identifier hiding detected: Identifier for identifiers: [Definition] resolved as an alias of a query with exact case matching.\n"; + final String hidingVarLet = "Identifier hiding detected: Identifier for identifiers: [var] resolved as a let of a query with exact case matching.\n"; + final String hidingContextValueSet = "Identifier hiding detected: Identifier for identifiers: [ValueSet] resolved as an alias of a query with exact case matching.\n"; + final String hidingLetValueSet = "Identifier hiding detected: Identifier for identifiers: [ValueSet] resolved as a let of a query with exact case matching.\n"; + final String hidingContextCode = "Identifier hiding detected: Identifier for identifiers: [Code] resolved as an alias of a query with exact case matching.\n"; + final String hidingLetCode = "Identifier hiding detected: Identifier for identifiers: [Code] resolved as a let of a query with exact case matching.\n"; + final String hidingContextCodeSystem = "Identifier hiding detected: Identifier for identifiers: [CodeSystem] resolved as an alias of a query with exact case matching.\n"; + final String hidingLetCodeSystem = "Identifier hiding detected: Identifier for identifiers: [CodeSystem] resolved as a let of a query with exact case matching.\n"; + final String hidingContextFhir = "Identifier hiding detected: Identifier for identifiers: [FHIR] resolved as an alias of a query with exact case matching.\n"; + final String hidingLetFhir = "Identifier hiding detected: Identifier for identifiers: [FHIR] resolved as a let of a query with exact case matching.\n"; + final String hidingAliasLet = "Identifier hiding detected: Identifier for identifiers: [Alias] resolved as a let of a query with exact case matching.\n"; + + assertThat(distinct, containsInAnyOrder(hidingDefinition, hidingVarLet, hidingContextValueSet, hidingLetValueSet, hidingContextCode, hidingLetCode, hidingContextCodeSystem, hidingLetCodeSystem, hidingContextFhir, hidingLetFhir, hidingAliasLet)); } } diff --git a/Src/java/cql-to-elm/src/test/java/org/cqframework/cql/cql2elm/qicore/v411/BaseTest.java b/Src/java/cql-to-elm/src/test/java/org/cqframework/cql/cql2elm/qicore/v411/BaseTest.java index b6afbb2c0..213b2800b 100644 --- a/Src/java/cql-to-elm/src/test/java/org/cqframework/cql/cql2elm/qicore/v411/BaseTest.java +++ b/Src/java/cql-to-elm/src/test/java/org/cqframework/cql/cql2elm/qicore/v411/BaseTest.java @@ -9,20 +9,30 @@ import java.io.IOException; import java.util.HashMap; import java.util.Map; +import java.util.List; +import java.util.stream.Collectors; import static org.hamcrest.CoreMatchers.equalTo; import static org.hamcrest.CoreMatchers.notNullValue; import static org.hamcrest.MatcherAssert.assertThat; -import static org.hamcrest.Matchers.instanceOf; -import static org.hamcrest.Matchers.is; +import static org.hamcrest.Matchers.*; import static org.junit.Assert.assertNotNull; public class BaseTest { @Test public void testAuthoringPatterns() throws IOException { - CqlTranslator translator = TestUtils.runSemanticTest("qicore/v411/AuthoringPatterns.cql", 0, LibraryBuilder.SignatureLevel.Overloads); + final CqlTranslator translator = TestUtils.runSemanticTest("qicore/v411/AuthoringPatterns.cql", 0, LibraryBuilder.SignatureLevel.Overloads); - assertThat(translator.getWarnings().size(), is(0)); + assertThat(translator.getWarnings().stream().map(Throwable::getMessage).collect(Collectors.toList()).toString(), translator.getWarnings().size(), is(2)); + + final List distinct = translator.getWarnings().stream().map(Throwable::getMessage).distinct().collect(Collectors.toList()); + + assertThat(distinct.size(), is(2)); + + final String first = "Identifier hiding detected: Identifier for identifiers: [Diabetes] resolved as an alias of a query with exact case matching.\n"; + final String second = "Identifier hiding detected: Identifier for identifiers: [Application of Intermittent Pneumatic Compression Devices (IPC)] resolved as a value set with case insensitive matching.\n"; + + assertThat(distinct, containsInAnyOrder(first, second)); } @Test diff --git a/Src/java/cql-to-elm/src/test/java/org/cqframework/cql/cql2elm/qicore/v500/BaseTest.java b/Src/java/cql-to-elm/src/test/java/org/cqframework/cql/cql2elm/qicore/v500/BaseTest.java index 8aa780b9b..d9c8acc58 100644 --- a/Src/java/cql-to-elm/src/test/java/org/cqframework/cql/cql2elm/qicore/v500/BaseTest.java +++ b/Src/java/cql-to-elm/src/test/java/org/cqframework/cql/cql2elm/qicore/v500/BaseTest.java @@ -8,13 +8,14 @@ import java.io.IOException; import java.util.HashMap; +import java.util.List; import java.util.Map; +import java.util.stream.Collectors; import static org.hamcrest.CoreMatchers.equalTo; import static org.hamcrest.CoreMatchers.notNullValue; import static org.hamcrest.MatcherAssert.assertThat; -import static org.hamcrest.Matchers.instanceOf; -import static org.hamcrest.Matchers.is; +import static org.hamcrest.Matchers.*; public class BaseTest { @Test @@ -29,9 +30,18 @@ public void testCQMCommon() throws IOException { @Test public void testAuthoringPatterns() throws IOException { - CqlTranslator translator = TestUtils.runSemanticTest("qicore/v500/AuthoringPatterns.cql", 0, LibraryBuilder.SignatureLevel.Overloads); + final CqlTranslator translator = TestUtils.runSemanticTest("qicore/v500/AuthoringPatterns.cql", 0, LibraryBuilder.SignatureLevel.Overloads); - assertThat(translator.getWarnings().size(), is(0)); + assertThat(translator.getWarnings().toString(), translator.getWarnings().size(), is(2)); + + final List distinct = translator.getWarnings().stream().map(Throwable::getMessage).distinct().collect(Collectors.toList()); + + assertThat(distinct.size(), is(2)); + + final String first = "Identifier hiding detected: Identifier for identifiers: [Diabetes] resolved as an alias of a query with exact case matching.\n"; + final String second = "Identifier hiding detected: Identifier for identifiers: [Application of Intermittent Pneumatic Compression Devices (IPC)] resolved as a value set with case insensitive matching.\n"; + + assertThat(distinct, containsInAnyOrder(first, second)); } @Test diff --git a/Src/java/cql-to-elm/src/test/resources/org/cqframework/cql/cql2elm/HidingTests/TestHiddenIdentifierArgumentToAlias.cql b/Src/java/cql-to-elm/src/test/resources/org/cqframework/cql/cql2elm/HidingTests/TestHiddenIdentifierArgumentToAlias.cql new file mode 100644 index 000000000..2e0ff74e8 --- /dev/null +++ b/Src/java/cql-to-elm/src/test/resources/org/cqframework/cql/cql2elm/HidingTests/TestHiddenIdentifierArgumentToAlias.cql @@ -0,0 +1,6 @@ +library TestHiddenIdentifierArgumentToAlias + +//// Should result in a "Hiding operand identifier" +define function TestOperandHiding(testOperand Integer): + ({1, 2, 3}) testOperand + return testOperand diff --git a/Src/java/cql-to-elm/src/test/resources/org/cqframework/cql/cql2elm/HidingTests/TestHiddenIdentifierFromReturn.cql b/Src/java/cql-to-elm/src/test/resources/org/cqframework/cql/cql2elm/HidingTests/TestHiddenIdentifierFromReturn.cql new file mode 100644 index 000000000..234531ac9 --- /dev/null +++ b/Src/java/cql-to-elm/src/test/resources/org/cqframework/cql/cql2elm/HidingTests/TestHiddenIdentifierFromReturn.cql @@ -0,0 +1,9 @@ +library TestHiddenIdentifierFromReturn + +define "HiddenIdentifiersFromReturn": + ({1, 2, 3}) X + let var : X + 1 + return + var varalias + let var : varalias + 2 + return varalias \ No newline at end of file diff --git a/Src/java/cql-to-elm/src/test/resources/org/cqframework/cql/cql2elm/HidingTests/TestHidingCaseInsensitiveWarning.cql b/Src/java/cql-to-elm/src/test/resources/org/cqframework/cql/cql2elm/HidingTests/TestHidingCaseInsensitiveWarning.cql new file mode 100644 index 000000000..a9452ec01 --- /dev/null +++ b/Src/java/cql-to-elm/src/test/resources/org/cqframework/cql/cql2elm/HidingTests/TestHidingCaseInsensitiveWarning.cql @@ -0,0 +1,12 @@ +library TestCaseInsensitiveWarning + +using FHIR version '4.0.1' +include FHIRHelpers version '4.0.1' + +context Patient + +define "Patients" :[Patient] +define "patients": [Patient] P where P.gender = 'male' + +define function findPatients() : + "patients" P where P.birthDate > Date(1980,1,1) \ No newline at end of file diff --git a/Src/java/cql-to-elm/src/test/resources/org/cqframework/cql/cql2elm/HidingTests/TestHidingFunctionDefinitionWithOverloads.cql b/Src/java/cql-to-elm/src/test/resources/org/cqframework/cql/cql2elm/HidingTests/TestHidingFunctionDefinitionWithOverloads.cql new file mode 100644 index 000000000..f7e4acb34 --- /dev/null +++ b/Src/java/cql-to-elm/src/test/resources/org/cqframework/cql/cql2elm/HidingTests/TestHidingFunctionDefinitionWithOverloads.cql @@ -0,0 +1,7 @@ +library TestHidingFunctionDefinitionWithOverloads + +define function IWantToBeHidden(value Integer): value + +define function IWantToBeHidden(value String): + ({1, 2, 3}) "IWantToBeHidden" + return "IWantToBeHidden" + 1 diff --git a/Src/java/cql-to-elm/src/test/resources/org/cqframework/cql/cql2elm/HidingTests/TestHidingIncludeDefinition.cql b/Src/java/cql-to-elm/src/test/resources/org/cqframework/cql/cql2elm/HidingTests/TestHidingIncludeDefinition.cql new file mode 100644 index 000000000..78a388550 --- /dev/null +++ b/Src/java/cql-to-elm/src/test/resources/org/cqframework/cql/cql2elm/HidingTests/TestHidingIncludeDefinition.cql @@ -0,0 +1,9 @@ +library TestHidingIncludeDefinition + +using FHIR version '4.0.1' +include FHIRHelpers version '4.0.1' + +//// Should result in a "Hiding operand identifier" +define function HidingIncludeHiding(value Integer): + ({1, 2, 3}) "FHIRHelpers" + return "FHIRHelpers" diff --git a/Src/java/cql-to-elm/src/test/resources/org/cqframework/cql/cql2elm/HidingTests/TestHidingLetAlias.cql b/Src/java/cql-to-elm/src/test/resources/org/cqframework/cql/cql2elm/HidingTests/TestHidingLetAlias.cql new file mode 100644 index 000000000..e8939c176 --- /dev/null +++ b/Src/java/cql-to-elm/src/test/resources/org/cqframework/cql/cql2elm/HidingTests/TestHidingLetAlias.cql @@ -0,0 +1,6 @@ +library TestHidingLetAlias + +define "AliasHidden": + ({1, 2, 3}) "Alias" + let "Alias": "Alias" + 1 // Warn, hides 49 + return "Alias" diff --git a/Src/java/cql-to-elm/src/test/resources/org/cqframework/cql/cql2elm/HidingTests/TestHidingParameterDefinition.cql b/Src/java/cql-to-elm/src/test/resources/org/cqframework/cql/cql2elm/HidingTests/TestHidingParameterDefinition.cql new file mode 100644 index 000000000..0fa442fb6 --- /dev/null +++ b/Src/java/cql-to-elm/src/test/resources/org/cqframework/cql/cql2elm/HidingTests/TestHidingParameterDefinition.cql @@ -0,0 +1,7 @@ +library TestHidingParameterDefinition + +parameter "Measurement Period" Interval + +define function HidingParameterDefinition(testOperand Integer): + ({1, 2, 3}) "Measurement Period" + return "Measurement Period" diff --git a/Src/java/cql-to-elm/src/test/resources/org/cqframework/cql/cql2elm/HidingTests/TestHidingReturnArgumentNotConsideredHiddenIdentifier.cql b/Src/java/cql-to-elm/src/test/resources/org/cqframework/cql/cql2elm/HidingTests/TestHidingReturnArgumentNotConsideredHiddenIdentifier.cql new file mode 100644 index 000000000..1b3a8170a --- /dev/null +++ b/Src/java/cql-to-elm/src/test/resources/org/cqframework/cql/cql2elm/HidingTests/TestHidingReturnArgumentNotConsideredHiddenIdentifier.cql @@ -0,0 +1,3 @@ +library TestReturnArgumentNotConsideredHiddenIdentifier + +define function TestAny(a Integer): a diff --git a/Src/java/cql-to-elm/src/test/resources/org/cqframework/cql/cql2elm/HidingTests/TestHidingSoMuchNestingHidingComplex.cql b/Src/java/cql-to-elm/src/test/resources/org/cqframework/cql/cql2elm/HidingTests/TestHidingSoMuchNestingHidingComplex.cql new file mode 100644 index 000000000..b1a7dc638 --- /dev/null +++ b/Src/java/cql-to-elm/src/test/resources/org/cqframework/cql/cql2elm/HidingTests/TestHidingSoMuchNestingHidingComplex.cql @@ -0,0 +1,22 @@ +library TestSoMuchNestingHidingComplex + +define "SoMuchNesting": + ({ + { + {1, 2, 3}, + {1, 2, 3 } + }, + { + {4, 5, 6}, + {4, 5, 6} + }, + { + {7, 8, 9}, + {7, 8, 9} + } + }) + "SoMuchNesting" + let "SoMuchNesting": + First("SoMuchNesting") + return + "SoMuchNesting" diff --git a/Src/java/cql-to-elm/src/test/resources/org/cqframework/cql/cql2elm/HidingTests/TestHidingSoMuchNestingHidingSimple.cql b/Src/java/cql-to-elm/src/test/resources/org/cqframework/cql/cql2elm/HidingTests/TestHidingSoMuchNestingHidingSimple.cql new file mode 100644 index 000000000..e11c4607b --- /dev/null +++ b/Src/java/cql-to-elm/src/test/resources/org/cqframework/cql/cql2elm/HidingTests/TestHidingSoMuchNestingHidingSimple.cql @@ -0,0 +1,6 @@ +library TestSoMuchNestingHidingSimple + +define "SoMuchNesting": + ({1, 2, 3}) + "SoMuchNesting" + return "SoMuchNesting" + 1 diff --git a/Src/java/cql-to-elm/src/test/resources/org/cqframework/cql/cql2elm/HidingTests/TestHidingSoMuchNestingNormal.cql b/Src/java/cql-to-elm/src/test/resources/org/cqframework/cql/cql2elm/HidingTests/TestHidingSoMuchNestingNormal.cql new file mode 100644 index 000000000..825247ff8 --- /dev/null +++ b/Src/java/cql-to-elm/src/test/resources/org/cqframework/cql/cql2elm/HidingTests/TestHidingSoMuchNestingNormal.cql @@ -0,0 +1,26 @@ +library TestSoMuchNestingHidingNormal + +define "SoMuchNesting": + ({ + { + {1, 2, 3}, + {1, 2, 3 } + }, + { + {4, 5, 6}, + {4, 5, 6} + }, + { + {7, 8, 9}, + {7, 8, 9} + } + }) + oneLevel + let twoLevel: + First(oneLevel) + return + twoLevel + threeLevel + let fourLevel: + Add(threeLevel, 1) + return fourLevel \ No newline at end of file diff --git a/Src/java/cql-to-elm/src/test/resources/org/cqframework/cql/cql2elm/HidingTests/TestHidingUnionSameAlias.cql b/Src/java/cql-to-elm/src/test/resources/org/cqframework/cql/cql2elm/HidingTests/TestHidingUnionSameAlias.cql new file mode 100644 index 000000000..e4a8ba3a4 --- /dev/null +++ b/Src/java/cql-to-elm/src/test/resources/org/cqframework/cql/cql2elm/HidingTests/TestHidingUnionSameAlias.cql @@ -0,0 +1,7 @@ +library TestHidingUnionSameAlias + +define "X" : 'X' +define "Y" : 'Y' + +define "UnionSameAlias": + ("X" E) union ("Y" E) diff --git a/Src/java/cql-to-elm/src/test/resources/org/cqframework/cql/cql2elm/HidingTests/TestHidingUnionSameAliasEachHides.cql b/Src/java/cql-to-elm/src/test/resources/org/cqframework/cql/cql2elm/HidingTests/TestHidingUnionSameAliasEachHides.cql new file mode 100644 index 000000000..7d31eb836 --- /dev/null +++ b/Src/java/cql-to-elm/src/test/resources/org/cqframework/cql/cql2elm/HidingTests/TestHidingUnionSameAliasEachHides.cql @@ -0,0 +1,8 @@ +library TestHidingUnionSameAliasEachHides + +define "X" : 'X' +define "Y" : 'Y' +define IWantToBeHidden : 'E' + +define "UnionSameAlias": + ("X" IWantToBeHidden) union ("Y" IWantToBeHidden) diff --git a/Src/java/cql-to-elm/src/test/resources/org/cqframework/cql/cql2elm/HidingTests/TestHidingVariousUseCases.cql b/Src/java/cql-to-elm/src/test/resources/org/cqframework/cql/cql2elm/HidingTests/TestHidingVariousUseCases.cql new file mode 100644 index 000000000..d59caab37 --- /dev/null +++ b/Src/java/cql-to-elm/src/test/resources/org/cqframework/cql/cql2elm/HidingTests/TestHidingVariousUseCases.cql @@ -0,0 +1,60 @@ +library TestHidingVariousUseCases + +using FHIR version '4.0.1' +include "FHIRHelpers" version '4.0.1' + +codesystem "CodeSystem": 'ABC' +valueset "ValueSet": '123' +code "Code": 'XYZ' from "CodeSystem" + +define "CodeSystemHidden": + ({1, 2, 3}) "CodeSystem" return "CodeSystem" //Warn, hides line 6 + +define "CodeSystemHidden2": + ({1, 2, 3}) X + let "CodeSystem" : X + 1 return "CodeSystem" //Warn, hides line 6 + +define "ValueSetHidden": + ({1, 2, 3}) "ValueSet" return "ValueSet" //Warn, hides line 7 + +define "ValueSetHidden2": + ({1, 2, 3}) X + let "ValueSet" : X + 1 return "ValueSet" //Warn, hides line 7 + +define "CodeHidden": + ({1, 2, 3}) "Code" return "Code" //Warn, hides line 8 + +define "CodeHidden2": + ({1, 2, 3}) X + let "Code" : X + 1 return "Code" //Warn, hides line 8 + +define "FHIRHelpersHidden": + ({1, 2, 3}) "Code" return "Code" //Warn, hides line 8 + +define "FHIRHelpersHidden2": + ({1, 2, 3}) X + let "Code" : X + 1 return "Code" //Warn, hides line 8 + +define "FHIRHidden": + ({1, 2, 3}) FHIR return FHIR //Warn, hides line 3 + +define "FHIRHidden2": + ({1, 2, 3}) X + let FHIR : X + 1 return FHIR //Warn, hides line 3 + +define "Definition": + ({1, 2, 3}) "Definition" return "Definition" //Warn, hides line 45 + +define "AliasHidden": + ({1, 2, 3}) "Alias" + let "Alias": "Alias" + 1 // Warn, hides 49 + return "Alias" + +define "VariableHidden": + ({1, 2, 3}) X + let var : X + 1 + return + var varalias + let var : varalias + 2 // Warn, hides 55 + return varalias + diff --git a/Src/java/cql-to-elm/src/test/resources/org/cqframework/cql/cql2elm/TestIdentifierCaseMismatch.cql b/Src/java/cql-to-elm/src/test/resources/org/cqframework/cql/cql2elm/TestIdentifierCaseMismatch.cql new file mode 100644 index 000000000..882dd9726 --- /dev/null +++ b/Src/java/cql-to-elm/src/test/resources/org/cqframework/cql/cql2elm/TestIdentifierCaseMismatch.cql @@ -0,0 +1,6 @@ +library TestIdentifierCaseMismatch + +using FHIR version '4.0.1' + +// FHIR.Code is wrong: it should be FHIR.code +define function badFuncOne(value FHIR.Code) : null diff --git a/Src/java/cql-to-elm/src/test/resources/org/cqframework/cql/cql2elm/TestIncorrectParameterType1204.cql b/Src/java/cql-to-elm/src/test/resources/org/cqframework/cql/cql2elm/TestIncorrectParameterType1204.cql index 3f4f872dc..ab1fcce6c 100644 --- a/Src/java/cql-to-elm/src/test/resources/org/cqframework/cql/cql2elm/TestIncorrectParameterType1204.cql +++ b/Src/java/cql-to-elm/src/test/resources/org/cqframework/cql/cql2elm/TestIncorrectParameterType1204.cql @@ -5,5 +5,4 @@ library TestQuotedForwards using FHIR version '4.0.1' -// FHIR.Code is wrong: it should be FHIR.code -define function badFuncOne(value FHIR.Code) : null +define function badFuncOne(value ThisTypeDoesNotExist) : null From 2b0320f1797667c526bd3cbc9c6ce6bd740a40ad Mon Sep 17 00:00:00 2001 From: Luke deGruchy Date: Thu, 9 Nov 2023 15:51:47 -0500 Subject: [PATCH 2/5] Add test that proves that 1254 is fixed. --- .../org/cqframework/cql/cql2elm/HidingTests.java | 11 +++++++++++ .../TestHidingCommaMissingInListConstruction.cql | 12 ++++++++++++ 2 files changed, 23 insertions(+) create mode 100644 Src/java/cql-to-elm/src/test/resources/org/cqframework/cql/cql2elm/HidingTests/TestHidingCommaMissingInListConstruction.cql diff --git a/Src/java/cql-to-elm/src/test/java/org/cqframework/cql/cql2elm/HidingTests.java b/Src/java/cql-to-elm/src/test/java/org/cqframework/cql/cql2elm/HidingTests.java index 7eb7fd151..43c121645 100644 --- a/Src/java/cql-to-elm/src/test/java/org/cqframework/cql/cql2elm/HidingTests.java +++ b/Src/java/cql-to-elm/src/test/java/org/cqframework/cql/cql2elm/HidingTests.java @@ -144,4 +144,15 @@ public void testHidingIncludeDefinition() throws IOException { assertThat(warningMessages.toString(), warnings.size(), is(1)); assertThat(warningMessages, contains("Identifier hiding detected: Identifier for identifiers: [FHIRHelpers] resolved as an alias of a query with exact case matching.\n")); } + + @Test + public void testHidingCommaMissingInListConstruction() throws IOException { + final CqlTranslator translator = TestUtils.runSemanticTestNoErrors("HidingTests/TestHidingCommaMissingInListConstruction.cql"); + final List warnings = translator.getWarnings(); + final List warningMessages = warnings.stream().map(Throwable::getMessage).collect(Collectors.toList()); + assertThat(warningMessages.toString(), warnings.size(), is(2)); + final List distinctWarningMessages = warningMessages.stream().distinct().collect(Collectors.toList()); + assertThat(distinctWarningMessages.toString(), distinctWarningMessages.size(), is(1)); + assertThat(distinctWarningMessages, contains("Identifier hiding detected: Identifier for identifiers: [5] resolved as an alias of a query with exact case matching.\n")); + } } diff --git a/Src/java/cql-to-elm/src/test/resources/org/cqframework/cql/cql2elm/HidingTests/TestHidingCommaMissingInListConstruction.cql b/Src/java/cql-to-elm/src/test/resources/org/cqframework/cql/cql2elm/HidingTests/TestHidingCommaMissingInListConstruction.cql new file mode 100644 index 000000000..8da42cd11 --- /dev/null +++ b/Src/java/cql-to-elm/src/test/resources/org/cqframework/cql/cql2elm/HidingTests/TestHidingCommaMissingInListConstruction.cql @@ -0,0 +1,12 @@ +library TestHidingCommaMissingInListConstruction + +define "1": 1 +define "2": 2 +define "3": 3 +define "4": 4 +define "5": 5 + +define "Four With Five": "4" "5" // "5" is interpreted as an alias here, result is 4 +define "Query Four": ({ "4" }) "X" return "X" // This is how the above expression looks to the CQL compiler +define "Four With Alias": "4" "15" return Add("15", 1) // "15" doesn't exist, It's a alias for "4". result is 5 (4+1) +define "ListTest6": {"1", "2", "3", "4" "5"} // Valid, "5" is an alias for "4" \ No newline at end of file From ea44f8a06e3da013d917db976fbf82b631d18239 Mon Sep 17 00:00:00 2001 From: Luke deGruchy Date: Thu, 9 Nov 2023 16:14:34 -0500 Subject: [PATCH 3/5] Code review suggestions. --- .../cql/cql2elm/Cql2ElmVisitor.java | 24 +++---- .../cql/cql2elm/LibraryBuilder.java | 70 ++----------------- 2 files changed, 17 insertions(+), 77 deletions(-) diff --git a/Src/java/cql-to-elm/src/main/java/org/cqframework/cql/cql2elm/Cql2ElmVisitor.java b/Src/java/cql-to-elm/src/main/java/org/cqframework/cql/cql2elm/Cql2ElmVisitor.java index b81d4ffc0..9ede7c54e 100755 --- a/Src/java/cql-to-elm/src/main/java/org/cqframework/cql/cql2elm/Cql2ElmVisitor.java +++ b/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, null); + libraryBuilder.pushIdentifierForHiding(localIdentifier, false, usingDef); return usingDef; } @@ -198,7 +198,7 @@ public Object visitIncludeDefinition(cqlParser.IncludeDefinitionContext ctx) { } libraryBuilder.addInclude(library); - libraryBuilder.pushIdentifierForHiding(library.getLocalIdentifier(), false, library, null); + libraryBuilder.pushIdentifierForHiding(library.getLocalIdentifier(), false, library); return library; } @@ -235,7 +235,7 @@ public ParameterDef visitParameterDefinition(cqlParser.ParameterDefinitionContex } libraryBuilder.addParameter(param); - libraryBuilder.pushIdentifierForHiding(param.getName(), false, param, libraryBuilder.resolveParameterRef(param)); + libraryBuilder.pushIdentifierForHiding(param.getName(), false, param); return param; } @@ -278,7 +278,7 @@ public CodeSystemDef visitCodesystemDefinition(cqlParser.CodesystemDefinitionCon } libraryBuilder.addCodeSystem(cs); - libraryBuilder.pushIdentifierForHiding(cs.getName(), false, cs, libraryBuilder.getCodeSystemRef(cs)); + libraryBuilder.pushIdentifierForHiding(cs.getName(), false, 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.getValueSetRef(vs)); + libraryBuilder.pushIdentifierForHiding(vs.getName(), false, 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.getCodeRef(cd)); + libraryBuilder.pushIdentifierForHiding(cd.getName(), false, cd); return cd; } @@ -523,10 +523,10 @@ public ExpressionDef internalVisitExpressionDefinition(cqlParser.ExpressionDefin ExpressionDef def = libraryBuilder.resolveExpressionRef(identifier); // lightweight ExpressionDef to be used to output a hiding warning message - final ExpressionDef hallowExpressionDef = of.createExpressionDef() + final ExpressionDef hollowExpressionDef = of.createExpressionDef() .withName(identifier) .withContext(getCurrentContext()); - libraryBuilder.pushIdentifierForHiding(identifier, false, hallowExpressionDef, null); + libraryBuilder.pushIdentifierForHiding(identifier, false, hollowExpressionDef); if (def == null || isImplicitContextExpressionDef(def)) { if (def != null && isImplicitContextExpressionDef(def)) { libraryBuilder.removeExpression(def); @@ -3076,7 +3076,7 @@ public Object visitQuery(cqlParser.QueryContext ctx) { queryContext.addPrimaryQuerySources(sources); for (AliasedQuerySource source : sources) { - libraryBuilder.pushIdentifierForHiding(source.getAlias(), false, source, source.getExpression()); + libraryBuilder.pushIdentifierForHiding(source.getAlias(), false, source); } // If we are evaluating a population-level query whose source ranges over any patient-context expressions, @@ -3095,7 +3095,7 @@ public Object visitQuery(cqlParser.QueryContext ctx) { if (dfcx != null) { for (LetClause letClause : dfcx) { - libraryBuilder.pushIdentifierForHiding(letClause.getIdentifier(), false, letClause, libraryBuilder.getQueryLetRef(letClause)); + libraryBuilder.pushIdentifierForHiding(letClause.getIdentifier(), false, letClause); } } @@ -4051,10 +4051,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, fun.getExpression()); + libraryBuilder.pushIdentifierForHiding(fun.getName(), true, fun); final List operand = op.getFunctionDef().getOperand(); for (OperandDef operandDef : operand) { - libraryBuilder.pushIdentifierForHiding(operandDef.getName(), false, operandDef, libraryBuilder.resolveOperandRef(operandDef)); + libraryBuilder.pushIdentifierForHiding(operandDef.getName(), false, operandDef); } if (ctx.functionBody() != null) { diff --git a/Src/java/cql-to-elm/src/main/java/org/cqframework/cql/cql2elm/LibraryBuilder.java b/Src/java/cql-to-elm/src/main/java/org/cqframework/cql/cql2elm/LibraryBuilder.java index 56a17d755..53ca1e965 100644 --- a/Src/java/cql-to-elm/src/main/java/org/cqframework/cql/cql2elm/LibraryBuilder.java +++ b/Src/java/cql-to-elm/src/main/java/org/cqframework/cql/cql2elm/LibraryBuilder.java @@ -1427,7 +1427,7 @@ private Expression convertListExpression(Expression expression, Conversion conve return query; } - private void reportWarning(String message, Expression expression) { + private void reportWarning(String message, Element 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,64 +2344,6 @@ 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: @@ -3055,12 +2997,10 @@ private DataType getExpressionDefResultType(ExpressionDef expressionDef) { * 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. + * @param onlyOnce Special case to deal with overloaded functions, which are out scope for hiding. + * @param element The construct element, for {@link ExpressionRef}. */ - void pushIdentifierForHiding(String identifier, boolean onlyOnce, Element element, Expression nullableExpression) { + void pushIdentifierForHiding(String identifier, boolean onlyOnce, Element element) { final MatchType matchType = identifiersToCheckForHiding.stream() .map(innerIdentifier -> { if (innerIdentifier.equals(identifier)) { @@ -3081,7 +3021,7 @@ void pushIdentifierForHiding(String identifier, boolean onlyOnce, Element elemen final String message = String.format("Identifier hiding detected: Identifier for identifiers: %s%s", String.format(lookupElementWarning(element), identifier), formatMatchedMessage(matchType)+ "\n"); - reportWarning(message, nullableExpression); + reportWarning(message, element); } if (! onlyOnce || MatchType.NONE == matchType) { From 416ef861b50b454dba318ea43eb29bb58c9ef71d Mon Sep 17 00:00:00 2001 From: Luke deGruchy Date: Fri, 10 Nov 2023 12:44:14 -0500 Subject: [PATCH 4/5] More code review suggestions. --- .../cql/cql2elm/LibraryBuilder.java | 72 +++++++------------ .../cql/cql2elm/model/MatchType.java | 17 ----- .../cqframework/cql/cql2elm/HidingTests.java | 27 ++++--- .../cql/cql2elm/TranslationTests.java | 22 +++--- .../cql/cql2elm/qicore/v411/BaseTest.java | 4 +- .../cql/cql2elm/qicore/v500/BaseTest.java | 4 +- 6 files changed, 54 insertions(+), 92 deletions(-) delete mode 100644 Src/java/cql-to-elm/src/main/java/org/cqframework/cql/cql2elm/model/MatchType.java diff --git a/Src/java/cql-to-elm/src/main/java/org/cqframework/cql/cql2elm/LibraryBuilder.java b/Src/java/cql-to-elm/src/main/java/org/cqframework/cql/cql2elm/LibraryBuilder.java index 53ca1e965..b0097c575 100644 --- a/Src/java/cql-to-elm/src/main/java/org/cqframework/cql/cql2elm/LibraryBuilder.java +++ b/Src/java/cql-to-elm/src/main/java/org/cqframework/cql/cql2elm/LibraryBuilder.java @@ -2344,54 +2344,43 @@ public Expression resolveIdentifier(String identifier, boolean mustResolve) { return null; } - 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"; + return "An expression"; } else if (element instanceof ParameterDef) { - return "[%s] resolved as a parameter"; + return "A parameter"; } else if (element instanceof ValueSetDef) { - return "[%s] resolved as a value set"; + return "A valueset"; } else if (element instanceof CodeSystemDef) { - return "[%s] resolved as a code system"; + return "A codesystem"; } else if (element instanceof CodeDef) { - return "[%s] resolved as a code"; + return "A code"; } else if (element instanceof ConceptDef) { - return "[%s] resolved as a concept"; + return "A concept"; } else if (element instanceof IncludeDef) { - return "[%s] resolved as a library"; + return "An include"; } else if (element instanceof AliasedQuerySource) { - return "[%s] resolved as an alias of a query"; + return "An alias"; } else if (element instanceof LetClause) { - return "[%s] resolved as a let of a query"; + return "A let"; } else if (element instanceof OperandDef) { - return "[%s] resolved as an operand to a function"; + return "An operand"; } else if (element instanceof UsingDef) { - return "[%s] resolved as a using definition"; + return "A using"; } //default message if no match is made: - return "[%s] resolved more than once: " + ((element != null) ? element.getClass() : "[null]"); + return "An [unknown structure]"; } /** @@ -2998,33 +2987,26 @@ private DataType getExpressionDefResultType(ExpressionDef expressionDef) { * * @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 {@link ExpressionRef}. + * @param element The construct element, for example {@link ExpressionRef}. */ void pushIdentifierForHiding(String identifier, boolean onlyOnce, Element element) { - final MatchType matchType = identifiersToCheckForHiding.stream() - .map(innerIdentifier -> { - if (innerIdentifier.equals(identifier)) { - return MatchType.EXACT; + final boolean hasRelevantMatch = identifiersToCheckForHiding.stream() + .filter(innerIdentifier -> innerIdentifier.equalsIgnoreCase(identifier)) + .peek(matchedIdentifier -> { + if (onlyOnce) { + return; } - 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, element); - } + final boolean isExactMatch = matchedIdentifier.equals(identifier); + final String elementString = lookupElementWarning(element); + final String message= isExactMatch + ? String.format("%s identifier [%s] is hiding another identifier of the same name. %n", elementString, identifier) + : String.format("Are you sure you mean to use %s identifier [%s], instead of [%s]? %n", elementString.toLowerCase(), identifier, matchedIdentifier) ; - if (! onlyOnce || MatchType.NONE == matchType) { + reportWarning(message, element); + }).findAny() + .isPresent(); + if (! onlyOnce || ! hasRelevantMatch) { identifiersToCheckForHiding.push(identifier); } } diff --git a/Src/java/cql-to-elm/src/main/java/org/cqframework/cql/cql2elm/model/MatchType.java b/Src/java/cql-to-elm/src/main/java/org/cqframework/cql/cql2elm/model/MatchType.java deleted file mode 100644 index 13ed734b9..000000000 --- a/Src/java/cql-to-elm/src/main/java/org/cqframework/cql/cql2elm/model/MatchType.java +++ /dev/null @@ -1,17 +0,0 @@ -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; - } -} \ No newline at end of file diff --git a/Src/java/cql-to-elm/src/test/java/org/cqframework/cql/cql2elm/HidingTests.java b/Src/java/cql-to-elm/src/test/java/org/cqframework/cql/cql2elm/HidingTests.java index 43c121645..d3d76358b 100644 --- a/Src/java/cql-to-elm/src/test/java/org/cqframework/cql/cql2elm/HidingTests.java +++ b/Src/java/cql-to-elm/src/test/java/org/cqframework/cql/cql2elm/HidingTests.java @@ -18,7 +18,7 @@ public void testCaseInsensitiveWarning() throws IOException { final List warnings = translator.getWarnings(); assertThat(warnings.toString(), translator.getWarnings().size(), is(1)); final Set warningMessages = warnings.stream().map(Throwable::getMessage).collect(Collectors.toSet()); - assertThat(warningMessages, contains("Identifier hiding detected: Identifier for identifiers: [patients] resolved as an expression definition with case insensitive matching.\n")); + assertThat(warningMessages, contains("Are you sure you mean to use an expression identifier [patients], instead of [Patients]? \n")); } @Test @@ -28,7 +28,7 @@ public void testHiddenIdentifierFromReturn() throws IOException { assertThat(warnings.toString(), translator.getWarnings().size(), is(1)); final Set warningMessages = warnings.stream().map(Throwable::getMessage).collect(Collectors.toSet()); - assertThat(warningMessages, contains("Identifier hiding detected: Identifier for identifiers: [var] resolved as a let of a query with exact case matching.\n")); + assertThat(warningMessages, contains("A let identifier [var] is hiding another identifier of the same name. \n")); } @Test @@ -49,10 +49,7 @@ public void testHidingUnionWithSameAliasEachHides() throws IOException { final List distinct = translator.getWarnings().stream().map(Throwable::getMessage).distinct().collect(Collectors.toList()); assertThat(distinct.size(), is(1)); - - final String first = "Identifier hiding detected: Identifier for identifiers: [IWantToBeHidden] resolved as an alias of a query with exact case matching.\n"; - - assertThat(distinct, containsInAnyOrder(first)); + assertThat(distinct, contains("An alias identifier [IWantToBeHidden] is hiding another identifier of the same name. \n")); } @Test @@ -69,7 +66,7 @@ public void testSoMuchNestingHidingSimple() throws IOException { final List warnings = translator.getWarnings(); assertThat(warnings.toString(), translator.getWarnings().size(), is(1)); - assertThat(warnings.stream().map(Throwable::getMessage).collect(Collectors.toList()), containsInAnyOrder("Identifier hiding detected: Identifier for identifiers: [SoMuchNesting] resolved as an alias of a query with exact case matching.\n")); + assertThat(warnings.stream().map(Throwable::getMessage).collect(Collectors.toList()), containsInAnyOrder("An alias identifier [SoMuchNesting] is hiding another identifier of the same name. \n")); } @Test @@ -84,8 +81,8 @@ public void testSoMuchNestingHidingComplex() throws IOException { assertThat(distinct.size(), is(2)); - final String first = "Identifier hiding detected: Identifier for identifiers: [SoMuchNesting] resolved as an alias of a query with exact case matching.\n"; - final String second = "Identifier hiding detected: Identifier for identifiers: [SoMuchNesting] resolved as a let of a query with exact case matching.\n"; + final String first = "An alias identifier [SoMuchNesting] is hiding another identifier of the same name. \n"; + final String second = "A let identifier [SoMuchNesting] is hiding another identifier of the same name. \n"; assertThat(distinct, containsInAnyOrder(first, second)); } @@ -97,7 +94,7 @@ public void testHidingLetAlias() throws IOException { final List warningMessages = warnings.stream().map(Throwable::getMessage).collect(Collectors.toList()); assertThat(warningMessages.toString(), translator.getWarnings().size(), is(1)); - assertThat(warningMessages, contains("Identifier hiding detected: Identifier for identifiers: [Alias] resolved as a let of a query with exact case matching.\n")); + assertThat(warnings.stream().map(Throwable::getMessage).collect(Collectors.toList()), containsInAnyOrder("A let identifier [Alias] is hiding another identifier of the same name. \n")); } @Test @@ -109,7 +106,7 @@ public void testHiddenIdentifierArgumentToAlias() throws IOException { .stream() .map(Throwable::getMessage) .collect(Collectors.toList()), - contains("Identifier hiding detected: Identifier for identifiers: [testOperand] resolved as an alias of a query with exact case matching.\n")); + contains("An alias identifier [testOperand] is hiding another identifier of the same name. \n")); } @Test @@ -124,7 +121,7 @@ public void testHidingFunctionDefinitionWithOverloads() throws IOException { final List warnings = translator.getWarnings(); final List warningMessages = warnings.stream().map(Throwable::getMessage).collect(Collectors.toList()); assertThat(warningMessages.toString(), warnings.size(), is(1)); - assertThat(warningMessages, contains("Identifier hiding detected: Identifier for identifiers: [IWantToBeHidden] resolved as an alias of a query with exact case matching.\n")); + assertThat(warningMessages, contains("An alias identifier [IWantToBeHidden] is hiding another identifier of the same name. \n")); } @Test @@ -133,7 +130,7 @@ public void testHidingParameterDefinition() throws IOException { final List warnings = translator.getWarnings(); final List warningMessages = warnings.stream().map(Throwable::getMessage).collect(Collectors.toList()); assertThat(warningMessages.toString(), warnings.size(), is(1)); - assertThat(warningMessages, contains("Identifier hiding detected: Identifier for identifiers: [Measurement Period] resolved as an alias of a query with exact case matching.\n")); + assertThat(warningMessages, contains("An alias identifier [Measurement Period] is hiding another identifier of the same name. \n")); } @Test @@ -142,7 +139,7 @@ public void testHidingIncludeDefinition() throws IOException { final List warnings = translator.getWarnings(); final List warningMessages = warnings.stream().map(Throwable::getMessage).collect(Collectors.toList()); assertThat(warningMessages.toString(), warnings.size(), is(1)); - assertThat(warningMessages, contains("Identifier hiding detected: Identifier for identifiers: [FHIRHelpers] resolved as an alias of a query with exact case matching.\n")); + assertThat(warningMessages, contains("An alias identifier [FHIRHelpers] is hiding another identifier of the same name. \n")); } @Test @@ -153,6 +150,6 @@ public void testHidingCommaMissingInListConstruction() throws IOException { assertThat(warningMessages.toString(), warnings.size(), is(2)); final List distinctWarningMessages = warningMessages.stream().distinct().collect(Collectors.toList()); assertThat(distinctWarningMessages.toString(), distinctWarningMessages.size(), is(1)); - assertThat(distinctWarningMessages, contains("Identifier hiding detected: Identifier for identifiers: [5] resolved as an alias of a query with exact case matching.\n")); + assertThat(distinctWarningMessages, contains("An alias identifier [5] is hiding another identifier of the same name. \n")); } } diff --git a/Src/java/cql-to-elm/src/test/java/org/cqframework/cql/cql2elm/TranslationTests.java b/Src/java/cql-to-elm/src/test/java/org/cqframework/cql/cql2elm/TranslationTests.java index 4704378cb..476b0cf4a 100644 --- a/Src/java/cql-to-elm/src/test/java/org/cqframework/cql/cql2elm/TranslationTests.java +++ b/Src/java/cql-to-elm/src/test/java/org/cqframework/cql/cql2elm/TranslationTests.java @@ -292,17 +292,17 @@ public void testHidingVariousUseCases() throws IOException { assertThat(warningMessages.toString(), distinct.size(), is(11)); - final String hidingDefinition = "Identifier hiding detected: Identifier for identifiers: [Definition] resolved as an alias of a query with exact case matching.\n"; - final String hidingVarLet = "Identifier hiding detected: Identifier for identifiers: [var] resolved as a let of a query with exact case matching.\n"; - final String hidingContextValueSet = "Identifier hiding detected: Identifier for identifiers: [ValueSet] resolved as an alias of a query with exact case matching.\n"; - final String hidingLetValueSet = "Identifier hiding detected: Identifier for identifiers: [ValueSet] resolved as a let of a query with exact case matching.\n"; - final String hidingContextCode = "Identifier hiding detected: Identifier for identifiers: [Code] resolved as an alias of a query with exact case matching.\n"; - final String hidingLetCode = "Identifier hiding detected: Identifier for identifiers: [Code] resolved as a let of a query with exact case matching.\n"; - final String hidingContextCodeSystem = "Identifier hiding detected: Identifier for identifiers: [CodeSystem] resolved as an alias of a query with exact case matching.\n"; - final String hidingLetCodeSystem = "Identifier hiding detected: Identifier for identifiers: [CodeSystem] resolved as a let of a query with exact case matching.\n"; - final String hidingContextFhir = "Identifier hiding detected: Identifier for identifiers: [FHIR] resolved as an alias of a query with exact case matching.\n"; - final String hidingLetFhir = "Identifier hiding detected: Identifier for identifiers: [FHIR] resolved as a let of a query with exact case matching.\n"; - final String hidingAliasLet = "Identifier hiding detected: Identifier for identifiers: [Alias] resolved as a let of a query with exact case matching.\n"; + final String hidingDefinition = "An alias identifier [Definition] is hiding another identifier of the same name. \n"; + final String hidingVarLet = "A let identifier [var] is hiding another identifier of the same name. \n"; + final String hidingContextValueSet = "An alias identifier [ValueSet] is hiding another identifier of the same name. \n"; + final String hidingLetValueSet = "A let identifier [ValueSet] is hiding another identifier of the same name. \n"; + final String hidingContextCode = "An alias identifier [Code] is hiding another identifier of the same name. \n"; + final String hidingLetCode = "A let identifier [Code] is hiding another identifier of the same name. \n"; + final String hidingContextCodeSystem = "An alias identifier [CodeSystem] is hiding another identifier of the same name. \n"; + final String hidingLetCodeSystem = "A let identifier [CodeSystem] is hiding another identifier of the same name. \n"; + final String hidingContextFhir = "An alias identifier [FHIR] is hiding another identifier of the same name. \n"; + final String hidingLetFhir = "A let identifier [FHIR] is hiding another identifier of the same name. \n"; + final String hidingAliasLet = "A let identifier [Alias] is hiding another identifier of the same name. \n"; assertThat(distinct, containsInAnyOrder(hidingDefinition, hidingVarLet, hidingContextValueSet, hidingLetValueSet, hidingContextCode, hidingLetCode, hidingContextCodeSystem, hidingLetCodeSystem, hidingContextFhir, hidingLetFhir, hidingAliasLet)); } diff --git a/Src/java/cql-to-elm/src/test/java/org/cqframework/cql/cql2elm/qicore/v411/BaseTest.java b/Src/java/cql-to-elm/src/test/java/org/cqframework/cql/cql2elm/qicore/v411/BaseTest.java index 213b2800b..66a270691 100644 --- a/Src/java/cql-to-elm/src/test/java/org/cqframework/cql/cql2elm/qicore/v411/BaseTest.java +++ b/Src/java/cql-to-elm/src/test/java/org/cqframework/cql/cql2elm/qicore/v411/BaseTest.java @@ -29,8 +29,8 @@ public void testAuthoringPatterns() throws IOException { assertThat(distinct.size(), is(2)); - final String first = "Identifier hiding detected: Identifier for identifiers: [Diabetes] resolved as an alias of a query with exact case matching.\n"; - final String second = "Identifier hiding detected: Identifier for identifiers: [Application of Intermittent Pneumatic Compression Devices (IPC)] resolved as a value set with case insensitive matching.\n"; + final String first = "An alias identifier [Diabetes] is hiding another identifier of the same name. \n"; + final String second = "Are you sure you mean to use a valueset identifier [Application of Intermittent Pneumatic Compression Devices (IPC)], instead of [Application of intermittent pneumatic compression devices (IPC)]? \n"; assertThat(distinct, containsInAnyOrder(first, second)); } diff --git a/Src/java/cql-to-elm/src/test/java/org/cqframework/cql/cql2elm/qicore/v500/BaseTest.java b/Src/java/cql-to-elm/src/test/java/org/cqframework/cql/cql2elm/qicore/v500/BaseTest.java index d9c8acc58..7b5af139a 100644 --- a/Src/java/cql-to-elm/src/test/java/org/cqframework/cql/cql2elm/qicore/v500/BaseTest.java +++ b/Src/java/cql-to-elm/src/test/java/org/cqframework/cql/cql2elm/qicore/v500/BaseTest.java @@ -38,8 +38,8 @@ public void testAuthoringPatterns() throws IOException { assertThat(distinct.size(), is(2)); - final String first = "Identifier hiding detected: Identifier for identifiers: [Diabetes] resolved as an alias of a query with exact case matching.\n"; - final String second = "Identifier hiding detected: Identifier for identifiers: [Application of Intermittent Pneumatic Compression Devices (IPC)] resolved as a value set with case insensitive matching.\n"; + final String first = "An alias identifier [Diabetes] is hiding another identifier of the same name. \n"; + final String second = "Are you sure you mean to use a valueset identifier [Application of Intermittent Pneumatic Compression Devices (IPC)], instead of [Application of intermittent pneumatic compression devices (IPC)]? \n"; assertThat(distinct, containsInAnyOrder(first, second)); } From 8a22db26414f198a3e748451aaf020299fe9f73d Mon Sep 17 00:00:00 2001 From: Luke deGruchy Date: Mon, 13 Nov 2023 16:55:48 -0500 Subject: [PATCH 5/5] Ignore case-insensitive mismatches. --- .../java/org/cqframework/cql/cql2elm/LibraryBuilder.java | 8 ++------ .../java/org/cqframework/cql/cql2elm/HidingTests.java | 4 +--- .../cqframework/cql/cql2elm/qicore/v411/BaseTest.java | 9 ++------- .../cqframework/cql/cql2elm/qicore/v500/BaseTest.java | 9 ++------- 4 files changed, 7 insertions(+), 23 deletions(-) diff --git a/Src/java/cql-to-elm/src/main/java/org/cqframework/cql/cql2elm/LibraryBuilder.java b/Src/java/cql-to-elm/src/main/java/org/cqframework/cql/cql2elm/LibraryBuilder.java index b0097c575..a5c633efe 100644 --- a/Src/java/cql-to-elm/src/main/java/org/cqframework/cql/cql2elm/LibraryBuilder.java +++ b/Src/java/cql-to-elm/src/main/java/org/cqframework/cql/cql2elm/LibraryBuilder.java @@ -2991,18 +2991,14 @@ private DataType getExpressionDefResultType(ExpressionDef expressionDef) { */ void pushIdentifierForHiding(String identifier, boolean onlyOnce, Element element) { final boolean hasRelevantMatch = identifiersToCheckForHiding.stream() - .filter(innerIdentifier -> innerIdentifier.equalsIgnoreCase(identifier)) + .filter(innerIdentifier -> innerIdentifier.equals(identifier)) .peek(matchedIdentifier -> { if (onlyOnce) { return; } - final boolean isExactMatch = matchedIdentifier.equals(identifier); final String elementString = lookupElementWarning(element); - final String message= isExactMatch - ? String.format("%s identifier [%s] is hiding another identifier of the same name. %n", elementString, identifier) - : String.format("Are you sure you mean to use %s identifier [%s], instead of [%s]? %n", elementString.toLowerCase(), identifier, matchedIdentifier) ; - + final String message = String.format("%s identifier [%s] is hiding another identifier of the same name. %n", elementString, identifier); reportWarning(message, element); }).findAny() .isPresent(); diff --git a/Src/java/cql-to-elm/src/test/java/org/cqframework/cql/cql2elm/HidingTests.java b/Src/java/cql-to-elm/src/test/java/org/cqframework/cql/cql2elm/HidingTests.java index d3d76358b..27470f353 100644 --- a/Src/java/cql-to-elm/src/test/java/org/cqframework/cql/cql2elm/HidingTests.java +++ b/Src/java/cql-to-elm/src/test/java/org/cqframework/cql/cql2elm/HidingTests.java @@ -16,9 +16,7 @@ public class HidingTests { public void testCaseInsensitiveWarning() throws IOException { final CqlTranslator translator = TestUtils.runSemanticTest("HidingTests/TestHidingCaseInsensitiveWarning.cql", 0, LibraryBuilder.SignatureLevel.All); final List warnings = translator.getWarnings(); - assertThat(warnings.toString(), translator.getWarnings().size(), is(1)); - final Set warningMessages = warnings.stream().map(Throwable::getMessage).collect(Collectors.toSet()); - assertThat(warningMessages, contains("Are you sure you mean to use an expression identifier [patients], instead of [Patients]? \n")); + assertThat(warnings.toString(), translator.getWarnings().size(), is(0)); } @Test diff --git a/Src/java/cql-to-elm/src/test/java/org/cqframework/cql/cql2elm/qicore/v411/BaseTest.java b/Src/java/cql-to-elm/src/test/java/org/cqframework/cql/cql2elm/qicore/v411/BaseTest.java index 66a270691..a64484b5b 100644 --- a/Src/java/cql-to-elm/src/test/java/org/cqframework/cql/cql2elm/qicore/v411/BaseTest.java +++ b/Src/java/cql-to-elm/src/test/java/org/cqframework/cql/cql2elm/qicore/v411/BaseTest.java @@ -23,16 +23,11 @@ public class BaseTest { public void testAuthoringPatterns() throws IOException { final CqlTranslator translator = TestUtils.runSemanticTest("qicore/v411/AuthoringPatterns.cql", 0, LibraryBuilder.SignatureLevel.Overloads); - assertThat(translator.getWarnings().stream().map(Throwable::getMessage).collect(Collectors.toList()).toString(), translator.getWarnings().size(), is(2)); - - final List distinct = translator.getWarnings().stream().map(Throwable::getMessage).distinct().collect(Collectors.toList()); - - assertThat(distinct.size(), is(2)); + assertThat(translator.getWarnings().toString(), translator.getWarnings().size(), is(1)); final String first = "An alias identifier [Diabetes] is hiding another identifier of the same name. \n"; - final String second = "Are you sure you mean to use a valueset identifier [Application of Intermittent Pneumatic Compression Devices (IPC)], instead of [Application of intermittent pneumatic compression devices (IPC)]? \n"; - assertThat(distinct, containsInAnyOrder(first, second)); + assertThat(translator.getWarnings().stream().map(Throwable::getMessage).collect(Collectors.toList()), contains(first)); } @Test diff --git a/Src/java/cql-to-elm/src/test/java/org/cqframework/cql/cql2elm/qicore/v500/BaseTest.java b/Src/java/cql-to-elm/src/test/java/org/cqframework/cql/cql2elm/qicore/v500/BaseTest.java index 7b5af139a..156fd1326 100644 --- a/Src/java/cql-to-elm/src/test/java/org/cqframework/cql/cql2elm/qicore/v500/BaseTest.java +++ b/Src/java/cql-to-elm/src/test/java/org/cqframework/cql/cql2elm/qicore/v500/BaseTest.java @@ -32,16 +32,11 @@ public void testCQMCommon() throws IOException { public void testAuthoringPatterns() throws IOException { final CqlTranslator translator = TestUtils.runSemanticTest("qicore/v500/AuthoringPatterns.cql", 0, LibraryBuilder.SignatureLevel.Overloads); - assertThat(translator.getWarnings().toString(), translator.getWarnings().size(), is(2)); - - final List distinct = translator.getWarnings().stream().map(Throwable::getMessage).distinct().collect(Collectors.toList()); - - assertThat(distinct.size(), is(2)); + assertThat(translator.getWarnings().toString(), translator.getWarnings().size(), is(1)); final String first = "An alias identifier [Diabetes] is hiding another identifier of the same name. \n"; - final String second = "Are you sure you mean to use a valueset identifier [Application of Intermittent Pneumatic Compression Devices (IPC)], instead of [Application of intermittent pneumatic compression devices (IPC)]? \n"; - assertThat(distinct, containsInAnyOrder(first, second)); + assertThat(translator.getWarnings().stream().map(Throwable::getMessage).collect(Collectors.toList()), contains(first)); } @Test