Merge pull request #1026 from FirelyTeam/1008-fix-qicore-2025-bugs-revisit-aliasrefresulttypespecifier-resolver-logic

Refactor library preprocessing with new classes

Author: baseTwo

Cql/CodeGeneration.NET/Toolkit/Internal/ElmToolkitServices.cs

@@ -8,6 +8,7 @@
 
 using Hl7.Cql.Abstractions;
 using Hl7.Cql.Compiler;
+using Hl7.Cql.Compiler.Preprocessing;
 using Hl7.Cql.Fhir;
 using Hl7.Cql.Runtime.Hosting;
 using Hl7.Cql.Runtime.Logging;
@@ -70,6 +71,7 @@ public static IServiceCollection AddCqlCompilerServices(
             return converter;
         });
 
+        services.TryAddSingleton<LibraryPreprocessorBuilder>();
         services.TryAddSingleton<CqlOperatorsBinder>();
         services.TryAddSingleton<CqlContextBinder>();
         services.TryAddSingleton(_ => expressionBuilderSettings);

Cql/CoreTests/ElmPreprocessorTest.cs renamed to Cql/CoreTests/LibraryPreprocessorTest.cs

@@ -7,12 +7,14 @@
  */
 
 using Hl7.Cql.Compiler;
+using Hl7.Cql.Compiler.Preprocessing;
 using Hl7.Cql.Elm;
+using Microsoft.Extensions.Logging.Abstractions;
 
 namespace CoreTests
 {
     [TestClass]
-    public class ElmPreprocessorTest
+    public class LibraryPreprocessorTest
     {
         private Library FHIRHelpers() =>
             Library.LoadFromJson(new FileInfo(Path.Combine("..", "..", "..", "..", "..", "LibrarySets", "Demo", "Elm", "FHIRHelpers.json")));
@@ -30,8 +32,8 @@ public void MATGlobal_Add_ResultTypeSpecifier()
             rtsChecker.Nodes.Should().NotBeEmpty();
 
             var ls = new LibrarySet("", FHIRHelpers(), lib);
-            var pp = new ElmPreprocessor(ls);
-            pp.Preprocess(lib);
+            var pp = new LibraryPreprocessor(ls, NullLoggerFactory.Instance);
+            pp.PreprocessLibrary(lib);
             rtsChecker = new ResultTypeSpecifierChecker();
             rtsChecker.Start(lib);
             // after pre-processing the library, there should be no such nodes remaining.

Cql/Cql.Compiler/AmbiguousOverloadCorrector.cs

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2024, NCQA and contributors
+ * Copyright (c) 2024, Firely, NCQA and contributors
  * See the file CONTRIBUTORS for details.
  *
  * This file is licensed under the BSD 3-Clause license
@@ -98,12 +98,6 @@ public void ProcessExpressionDef(
                         null);
                 }
 
-                // Pre-process expression to fix missing resultTypeSpecifier on AliasRef elements
-                if (expressionDef.expression != null)
-                {
-                    FixMissingAliasRefTypeSpecifiers(expressionDef.expression);
-                }
-
                 var expressionKey = $"{_libraryContext.LibraryVersionedIdentifier}.{expressionDefName}";
                 Type[] parameterTypes = [];
                 ParameterExpression[] parameters = [CqlExpressions.ParameterExpression];
@@ -137,7 +131,16 @@ public void ProcessExpressionDef(
                     }
                 }
 
-                CreateAndAddDefinition(expressionDefName, parameters, parameterTypes, originalParameterNames);
+                if (expressionDef.expression is { } expressionDefExpression)
+                {
+                    var bodyExpression = TranslateArg(expressionDefExpression);
+                    var lambda = Expression.Lambda(bodyExpression, parameters);
+                    var tags = BuildTags();
+                    var def = expressionDef is FunctionDef
+                                  ? new CqlFunctionDefinition(lambda, expressionDefName, originalParameterNames, tags)
+                                  : new CqlExpressionDefinition(lambda, expressionDefName, tags);
+                    _libraryContext.LibraryDefinitions.AddDefinition(_libraryContext.LibraryVersionedIdentifier, new(expressionDefName, parameterTypes), def);
+                }
             }
         });
 
@@ -212,21 +215,6 @@ void HandleExternalFunction(
 
             return (parameterTypes, originalNames);
         }
-
-        void CreateAndAddDefinition(
-            string expressionDefName,
-            ParameterExpression[] parameters,
-            Type[] parameterTypes,
-            IReadOnlyDictionary<string, string> originalParameterNames)
-        {
-            var bodyExpression = TranslateArg(expressionDef.expression);
-            var lambda = Expression.Lambda(bodyExpression, parameters);
-            var tags = BuildTags();
-            var def = expressionDef is FunctionDef
-                          ? new CqlFunctionDefinition(lambda, expressionDefName, originalParameterNames, tags)
-                          : new CqlExpressionDefinition(lambda, expressionDefName, tags);
-            _libraryContext.LibraryDefinitions.AddDefinition(_libraryContext.LibraryVersionedIdentifier, new(expressionDefName, parameterTypes), def);
-        }
     }
 
     public void ProcessIncludes(

Cql/Cql.Compiler/ElmPreprocessor.cs

@@ -6,6 +6,7 @@
  * available at https://raw.githubusercontent.com/FirelyTeam/firely-cql-sdk/main/LICENSE
  */
 
+using Hl7.Cql.Compiler.Preprocessing;
 using Hl7.Cql.Elm;
 
 namespace Hl7.Cql.Compiler;
@@ -15,7 +16,8 @@ namespace Hl7.Cql.Compiler;
 /// </summary>
 internal class LibraryExpressionBuilder(
     ILogger<LibraryExpressionBuilder> logger,
-    ExpressionBuilder expressionBuilder)
+    ExpressionBuilder expressionBuilder,
+    LibraryPreprocessorBuilder libraryPreprocessorBuilder)
 {
     public CqlDefinitionDictionary ProcessLibrary(
         Library library,
@@ -28,7 +30,7 @@ public LibraryExpressionBuilderContext NewLibraryExpressionBuilderContext(
         Library library,
         CqlDefinitionDictionary? libraryDefinitions = null,
         LibrarySetExpressionBuilderContext? libsCtx = null) =>
-        new(logger, expressionBuilder, library, libraryDefinitions ?? new(), libsCtx);
+        new(logger, expressionBuilder, libraryPreprocessorBuilder, library, libraryDefinitions ?? new(), libsCtx);
 
     public ExpressionBuilderContext NewExpressionBuilderContext(
         Library library,

Cql/Cql.Compiler/ExpressionBuilderContext.LibraryDefs.cs

@@ -6,6 +6,7 @@
  * available at https://raw.githubusercontent.com/FirelyTeam/firely-cql-sdk/main/LICENSE
  */
 
+using Hl7.Cql.Compiler.Preprocessing;
 using Hl7.Cql.Elm;
 
 namespace Hl7.Cql.Compiler;
@@ -16,10 +17,12 @@ internal partial class LibraryExpressionBuilderContext
     private readonly ExpressionBuilder _expressionBuilder;
     private readonly CqlDefinitionDictionary _libraryDefinitions;
     private readonly LibrarySetExpressionBuilderContext? _libsCtx;
+    private readonly LibraryPreprocessor _preprocessor;
 
     public LibraryExpressionBuilderContext(
         ILogger<LibraryExpressionBuilder> logger,
         ExpressionBuilder expressionBuilder,
+        LibraryPreprocessorBuilder libraryPreprocessorBuilder,
         Library library,
         CqlDefinitionDictionary libraryDefinitions,
         LibrarySetExpressionBuilderContext? libsCtx = null)
@@ -30,10 +33,11 @@ public LibraryExpressionBuilderContext(
         _expressionBuilder = expressionBuilder;
         Library = library;
         LibraryVersionedIdentifier = Library.VersionedLibraryIdentifier;
+        _preprocessor =
+            LibrarySetContext?.Preprocessor
+            ?? libraryPreprocessorBuilder.Build(new LibrarySet(LibraryVersionedIdentifier, Library));
     }
 
-    private static readonly AmbiguousOverloadCorrector AmbiguousOverloadCorrector = new AmbiguousOverloadCorrector();
-
     /// <summary>
     /// Gets the library associated with the expression builder context.
     /// </summary>
@@ -48,20 +52,10 @@ public LibraryExpressionBuilderContext(
     public CqlDefinitionDictionary ProcessLibrary() =>
         this.CatchRethrowExpressionBuildingException(_ =>
         {
-            // Make sure all overloads in the library are unique.
-            // This is a fix for QICore-based CQL, where the functions only differ by profiles on the same resource.
-            // We should remove this when the compiler is fixed.
-            // See https://github.com/FirelyTeam/firely-cql-sdk/issues/438.
-            _logger.LogDebug("Preprocessing library '{library}' - AmbiguousOverloadCorrector", LibraryVersionedIdentifier);
-            AmbiguousOverloadCorrector.Fix(Library);
-
-            _logger.LogDebug("Preprocessing library '{library}' - ElmPreprocessor", LibraryVersionedIdentifier);
-            var ls = LibrarySetContext?.LibrarySet ?? new LibrarySet(LibraryVersionedIdentifier, Library);
-            var processor = new ElmPreprocessor(ls);
-            processor.Preprocess(Library);
-
             _logger.LogDebug("Building expressions for '{library}'", LibraryVersionedIdentifier);
 
+            _preprocessor.PreprocessLibrary(Library);
+
             if (Library.includes is { Length: > 0 } includeDefs)
             {
                 foreach (var includeDef in includeDefs)

Cql/Cql.Compiler/LibraryExpressionBuilder.cs

@@ -6,12 +6,15 @@
  * available at https://raw.githubusercontent.com/FirelyTeam/firely-cql-sdk/main/LICENSE
  */
 
+using Hl7.Cql.Compiler.Preprocessing;
 using Hl7.Cql.Elm;
 using Hl7.Cql.Runtime;
 
 namespace Hl7.Cql.Compiler;
 
-internal class LibrarySetExpressionBuilder(LibraryExpressionBuilder libraryExpressionBuilder)
+internal class LibrarySetExpressionBuilder(
+    LibraryExpressionBuilder libraryExpressionBuilder,
+    LibraryPreprocessorBuilder libraryPreprocessorBuilder)
 {
     public IEnumerable<(Library library, CqlDefinitionDictionary libraryDefinitions)> BuildEachLibraryDefinitions(
         LibrarySet librarySet,
@@ -24,5 +27,5 @@ internal class LibrarySetExpressionBuilder(LibraryExpressionBuilder libraryExpre
     private LibrarySetExpressionBuilderContext NewLibrarySetExpressionBuilderContext(
         LibrarySet librarySet,
         CqlDefinitionDictionary librarySetDefinitions) =>
-        new(libraryExpressionBuilder, librarySet, librarySetDefinitions);
+        new(libraryExpressionBuilder, libraryPreprocessorBuilder, librarySet, librarySetDefinitions);
 }

Cql/Cql.Compiler/LibraryExpressionBuilderContext.cs

@@ -6,6 +6,7 @@
  * available at https://raw.githubusercontent.com/FirelyTeam/firely-cql-sdk/main/LICENSE
  */
 
+using Hl7.Cql.Compiler.Preprocessing;
 using Hl7.Cql.Runtime;
 using Library = Hl7.Cql.Elm.Library;
 
@@ -17,12 +18,14 @@ internal partial class LibrarySetExpressionBuilderContext
 
     public LibrarySetExpressionBuilderContext(
         LibraryExpressionBuilder libraryExpressionBuilder,
+        LibraryPreprocessorBuilder libraryPreprocessorBuilder,
         LibrarySet librarySet,
         CqlDefinitionDictionary librarySetDefinitions)
     {
         _libraryExpressionBuilder = libraryExpressionBuilder;
         LibrarySetDefinitions = librarySetDefinitions;
         LibrarySet = librarySet;
+        Preprocessor = libraryPreprocessorBuilder.Build(librarySet);
         DebuggerInfo = new BuilderContextDebuggerInfo("LibrarySet", Name: LibrarySet!.Name!);
     }
 
@@ -36,6 +39,8 @@ public LibrarySetExpressionBuilderContext(
     /// </summary>
     public LibrarySet LibrarySet { get; }
 
+    public LibraryPreprocessor Preprocessor { get; }
+
     public IEnumerable<(Library library, CqlDefinitionDictionary libraryDefinitions)> BuildEachLibraryDefinitions(
         BatchProcessExceptionHandlingStrategyBuilder<Library>? buildExceptionHandlingStrategy = null,
         Action<Library>? prebuildLibraryHandler = null) =>

Cql/Cql.Compiler/LibrarySetExpressionBuilder.cs

@@ -0,0 +1,60 @@
+/*
+ * Copyright (c) 2024, NCQA and contributors
+ * See the file CONTRIBUTORS for details.
+ *
+ * This file is licensed under the BSD 3-Clause license
+ * available at https://raw.githubusercontent.com/FirelyTeam/firely-cql-sdk/main/LICENSE
+ */
+
+using Hl7.Cql.Elm;
+
+namespace Hl7.Cql.Compiler.Preprocessing;
+
+/// <summary>
+/// Ensures that there are no overloads with exactly the same signature.
+/// </summary>
+/// <remarks>This happens currently in QICore-based CQL, where the functions only differ
+/// by profiles on the same resource, which results in the same signature after the QICore
+/// profile names are erased by the compiler. This is a temporary fix that should be
+/// corrected in the compiler. Until that moment, we'll just pick the first overload.
+/// See https://github.com/FirelyTeam/firely-cql-sdk/issues/438 for more info.
+/// </remarks>
+internal class AmbiguousOverloadCorrector(ILogger<AmbiguousOverloadCorrector> logger)
+{
+    public void Fix(Library library)
+    {
+        // Make sure all overloads in the library are unique.
+        // This is a fix for QICore-based CQL, where the functions only differ by profiles on the same resource.
+        // We should remove this when the compiler is fixed.
+        // See https://github.com/FirelyTeam/firely-cql-sdk/issues/438.
+        logger.LogDebug("Preprocessing library '{library}'", library.VersionedLibraryIdentifier);
+
+        if (library.statements is { Length: > 0 } expressionDefs)
+        {
+            const LogLevel logLevel = LogLevel.Information;
+
+            if (logger.IsEnabled(logLevel))
+            {
+                // Use grouping to find duplicates, so we can log them.
+                var grouped = expressionDefs.GroupBy(def => def, ExpressionSignatureComparer.Instance).ToList();
+                var duplicates = grouped
+                                 .Where(g => g.Count() > 1)
+                                 .Select(g => g.Key.Name)
+                                 .ToList();
+                if (duplicates.Count > 0)
+                {
+                    logger.Log(logLevel, "Duplicate overloads found in library '{Library}': {Duplicates}", library.VersionedLibraryIdentifier, string.Join(", ", duplicates));
+                    library.statements = grouped.Select(g => (ExpressionDef)g.Key).ToArray(); // Since we just only put ExpressionDefs into the HashSet, we can safely cast them back.
+                }
+            }
+            else
+            {
+                var statementSet = new HashSet<IDefinitionElement>(expressionDefs, ExpressionSignatureComparer.Instance);
+                if (statementSet.Count < expressionDefs.Length)
+                {
+                    library.statements = statementSet.Cast<ExpressionDef>().ToArray(); // Since we just only put ExpressionDefs into the HashSet, we can safely cast them back.
+                }
+            }
+        }
+    }
+}