Skip to content

Commit

Permalink
Full implementation of detection of identifier hiding (#1277)
Browse files Browse the repository at this point in the history
* Full implementation of detection of identifier hiding, including exact and case-insensitive matches as well as a lot of new unit tests.

* Add test that proves that 1254 is fixed.

* Code review suggestions.

* More code review suggestions.
  • Loading branch information
lukedegruchy authored Nov 15, 2023
1 parent a856e39 commit 9d92e5b
Show file tree
Hide file tree
Showing 25 changed files with 571 additions and 29 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down Expand Up @@ -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);
return usingDef;
}

public Model getModel() {
Expand Down Expand Up @@ -196,6 +198,7 @@ public Object visitIncludeDefinition(cqlParser.IncludeDefinitionContext ctx) {
}

libraryBuilder.addInclude(library);
libraryBuilder.pushIdentifierForHiding(library.getLocalIdentifier(), false, library);

return library;
}
Expand Down Expand Up @@ -232,6 +235,7 @@ public ParameterDef visitParameterDefinition(cqlParser.ParameterDefinitionContex
}

libraryBuilder.addParameter(param);
libraryBuilder.pushIdentifierForHiding(param.getName(), false, param);

return param;
}
Expand Down Expand Up @@ -274,6 +278,7 @@ public CodeSystemDef visitCodesystemDefinition(cqlParser.CodesystemDefinitionCon
}

libraryBuilder.addCodeSystem(cs);
libraryBuilder.pushIdentifierForHiding(cs.getName(), false, cs);
return cs;
}

Expand Down Expand Up @@ -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);

return vs;
}
Expand All @@ -366,6 +372,7 @@ public CodeDef visitCodeDefinition(cqlParser.CodeDefinitionContext ctx) {

cd.setResultType(libraryBuilder.resolveTypeName("Code"));
libraryBuilder.addCode(cd);
libraryBuilder.pushIdentifierForHiding(cd.getName(), false, cd);

return cd;
}
Expand Down Expand Up @@ -514,6 +521,12 @@ private void removeImplicitContextExpressionDef(ExpressionDef def) {
public ExpressionDef internalVisitExpressionDefinition(cqlParser.ExpressionDefinitionContext ctx) {
String identifier = parseString(ctx.identifier());
ExpressionDef def = libraryBuilder.resolveExpressionRef(identifier);

// lightweight ExpressionDef to be used to output a hiding warning message
final ExpressionDef hollowExpressionDef = of.createExpressionDef()
.withName(identifier)
.withContext(getCurrentContext());
libraryBuilder.pushIdentifierForHiding(identifier, false, hollowExpressionDef);
if (def == null || isImplicitContextExpressionDef(def)) {
if (def != null && isImplicitContextExpressionDef(def)) {
libraryBuilder.removeExpression(def);
Expand Down Expand Up @@ -3049,9 +3062,9 @@ public Object visitSourceClause(cqlParser.SourceClauseContext ctx) {
public Object visitQuery(cqlParser.QueryContext ctx) {
QueryContext queryContext = new QueryContext();
libraryBuilder.pushQueryContext(queryContext);
List<AliasedQuerySource> sources = null;
try {

List<AliasedQuerySource> sources;
queryContext.enterSourceClause();
try {
sources = (List<AliasedQuerySource>)visit(ctx.sourceClause());
Expand All @@ -3062,6 +3075,10 @@ public Object visitQuery(cqlParser.QueryContext ctx) {

queryContext.addPrimaryQuerySources(sources);

for (AliasedQuerySource source : sources) {
libraryBuilder.pushIdentifierForHiding(source.getAlias(), false, source);
}

// If we are evaluating a population-level query whose source ranges over any patient-context expressions,
// then references to patient context expressions within the iteration clauses of the query can be accessed
// at the patient, rather than the population, context.
Expand All @@ -3072,9 +3089,15 @@ public Object visitQuery(cqlParser.QueryContext ctx) {
expressionContextPushed = true;
}
*/
List<LetClause> dfcx = null;
try {
dfcx = ctx.letClause() != null ? (List<LetClause>) visit(ctx.letClause()) : null;

List<LetClause> dfcx = ctx.letClause() != null ? (List<LetClause>) visit(ctx.letClause()) : null;
if (dfcx != null) {
for (LetClause letClause : dfcx) {
libraryBuilder.pushIdentifierForHiding(letClause.getIdentifier(), false, letClause);
}
}

List<RelationshipClause> qicx = new ArrayList<>();
if (ctx.queryInclusionClause() != null) {
Expand Down Expand Up @@ -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();
}
}
}
}

Expand Down Expand Up @@ -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);
final List<OperandDef> operand = op.getFunctionDef().getOperand();
for (OperandDef operandDef : operand) {
libraryBuilder.pushIdentifierForHiding(operandDef.getName(), false, operandDef);
}

if (ctx.functionBody() != null) {
libraryBuilder.beginFunctionDef(fun);
Expand All @@ -4033,6 +4072,9 @@ public FunctionDef compileFunctionDefinition(cqlParser.FunctionDefinitionContext
libraryBuilder.popExpressionContext();
}
} finally {
for (OperandDef operandDef : operand) {
libraryBuilder.popIdentifierForHiding();
}
libraryBuilder.endFunctionDef();
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -101,6 +100,7 @@ public List<CqlCompilerException> getExceptions() {
private final Stack<String> expressionContext = new Stack<>();
private final ExpressionDefinitionContextStack expressionDefinitions = new ExpressionDefinitionContextStack();
private final Stack<FunctionDef> functionDefs = new Stack<>();
private final Deque<String> identifiersToCheckForHiding = new ArrayDeque<>();
private int literalContext = 0;
private int typeSpecifierContext = 0;
private NamespaceInfo namespaceInfo = null;
Expand Down Expand Up @@ -1427,8 +1427,8 @@ private Expression convertListExpression(Expression expression, Conversion conve
return query;
}

private void reportWarning(String message, Expression expression) {
TrackBack trackback = expression.getTrackbacks() != null && expression.getTrackbacks().size() > 0 ? expression.getTrackbacks().get(0) : null;
private void reportWarning(String message, 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);
}
Expand Down Expand Up @@ -2344,6 +2344,45 @@ public Expression resolveIdentifier(String identifier, boolean mustResolve) {
return null;
}

private static String lookupElementWarning(Object element) {
// TODO: this list is not exhaustive and may need to be updated
if (element instanceof ExpressionDef) {
return "An expression";
}
else if (element instanceof ParameterDef) {
return "A parameter";
}
else if (element instanceof ValueSetDef) {
return "A valueset";
}
else if (element instanceof CodeSystemDef) {
return "A codesystem";
}
else if (element instanceof CodeDef) {
return "A code";
}
else if (element instanceof ConceptDef) {
return "A concept";
}
else if (element instanceof IncludeDef) {
return "An include";
}
else if (element instanceof AliasedQuerySource) {
return "An alias";
}
else if (element instanceof LetClause) {
return "A let";
}
else if (element instanceof OperandDef) {
return "An operand";
}
else if (element instanceof UsingDef) {
return "A using";
}
//default message if no match is made:
return "An [unknown structure]";
}

/**
* An implicit context is one where the context has the same name as a parameter. Implicit contexts are used to
* allow FHIRPath expressions to resolve on the implicit context of the expression
Expand Down Expand Up @@ -2935,6 +2974,47 @@ private DataType getExpressionDefResultType(ExpressionDef expressionDef) {
throw new IllegalArgumentException(String.format("Invalid context reference from %s context to %s context.", currentExpressionContext(), expressionDef.getContext()));
}

/**
* Add an identifier to the deque to indicate that we are considering it for consideration for identifier hiding and
* adding a compiler warning if this is the case.
* <p/>
* For example, if an alias within an expression body has the same name as a parameter, execution would have
* added the parameter identifier and the next execution would consider an alias with the same name, thus resulting
* in a warning.
* <p/>
* Exact case matching as well as case-insensitive matching are considered. If known, the type of the structure
* in question will be considered in crafting the warning message, as per the {@link Element} parameter.
*
* @param identifier The identifier belonging to the parameter, expression, function, alias, etc, to be evaluated.
* @param onlyOnce Special case to deal with overloaded functions, which are out scope for hiding.
* @param element The construct element, for example {@link ExpressionRef}.
*/
void pushIdentifierForHiding(String identifier, boolean onlyOnce, Element element) {
final boolean hasRelevantMatch = identifiersToCheckForHiding.stream()
.filter(innerIdentifier -> innerIdentifier.equals(identifier))
.peek(matchedIdentifier -> {
if (onlyOnce) {
return;
}

final String elementString = lookupElementWarning(element);
final String message = String.format("%s identifier [%s] is hiding another identifier of the same name. %n", elementString, identifier);
reportWarning(message, element);
}).findAny()
.isPresent();
if (! onlyOnce || ! hasRelevantMatch) {
identifiersToCheckForHiding.push(identifier);
}
}

/**
* Pop the last resolved identifier off the deque. This is needed in case of a context in which an identifier
* falls out of scope, for an example, an alias within an expression or function body
*/
void popIdentifierForHiding() {
identifiersToCheckForHiding.pop();
}

private class Scope {
private final Stack<Expression> targets = new Stack<>();
private final Stack<QueryContext> queries = new Stack<>();
Expand Down
Loading

0 comments on commit 9d92e5b

Please sign in to comment.