fix(scriptanalysis): compile gate reads diagnostics only and never caches a faulted verdict (review 2026-10-21 04/M1, 04/C1)

RoslynScriptCompiler.Compile now uses Script.GetCompilation().GetDiagnostics()
with one shared, never-loading InteractiveAssemblyLoader instead of
Script.Compile(), which emitted and loaded a non-collectible assembly (and a
fresh AssemblyLoadContext) per distinct script. The catch-all that turned any
exception into a 'Compilation exception' diagnostic is gone; TryCompile and
CheckExpressionSyntax report a fault for that call only, uncached.
This commit is contained in:
Joseph Doherty
2026-10-22 06:15:35 -04:00
parent 75149e83b0
commit ae1282ed9d
8 changed files with 274 additions and 68 deletions
@@ -206,4 +206,65 @@ public class RoslynScriptCompilerTests
Assert.Empty(RoslynScriptCompiler.Compile("return 1 + 1;"));
Assert.Empty(RoslynScriptCompiler.Compile("return 2 + 2;"));
}
/// <summary>
/// Review 2026-10-21 04/M1: the design-time gate only reads diagnostics, so it must
/// never emit + load an assembly. Each distinct script used to add a permanent,
/// non-collectible <see cref="System.Runtime.Loader.AssemblyLoadContext"/> via
/// <c>Script.Compile()</c> → <c>InteractiveAssemblyLoader</c>.
/// </summary>
[Fact]
public void Compile_DistinctScripts_DoesNotLoadAnAssemblyPerScript()
{
const int n = 12;
// Warm up any one-off loads (Roslyn itself, the surface assembly).
RoslynScriptCompiler.Compile("return 0;", typeof(ScriptCompileSurface));
var contextsBefore = System.Runtime.Loader.AssemblyLoadContext.All.Count();
var assembliesBefore = AppDomain.CurrentDomain.GetAssemblies().Length;
for (var i = 0; i < n; i++)
Assert.Empty(RoslynScriptCompiler.Compile($"return {i} + {Guid.NewGuid().GetHashCode()};",
typeof(ScriptCompileSurface)));
// Tolerance for unrelated loads made by tests running in parallel; a per-script
// load would grow by at least n.
Assert.True(System.Runtime.Loader.AssemblyLoadContext.All.Count() - contextsBefore < n / 2,
"a distinct-script compile created an AssemblyLoadContext");
Assert.True(AppDomain.CurrentDomain.GetAssemblies().Length - assembliesBefore < n / 2,
"a distinct-script compile loaded an assembly");
}
/// <summary>
/// Review 2026-10-21 04/C1: an unexpected exception inside the compile gate must
/// propagate rather than come back as a "Compilation exception" diagnostic — the
/// diagnostic would be memoised as a sticky, process-wide "failed to compile"
/// verdict by the design-time verdict cache.
/// </summary>
[Fact]
public void Compile_UnexpectedException_Propagates_NotReturnedAsDiagnostic()
{
// A reference whose metadata read faults — a stand-in for any non-diagnostic
// failure (I/O reading metadata, OOM, a loader fault).
Assert.Throws<InvalidOperationException>(() =>
RoslynScriptCompiler.Compile("return 1;", extraReferences: [new FaultingReference()]));
}
/// <summary>A metadata reference whose metadata cannot be produced.</summary>
private sealed class FaultingReference : Microsoft.CodeAnalysis.PortableExecutableReference
{
public FaultingReference()
: base(Microsoft.CodeAnalysis.MetadataReferenceProperties.Assembly, "faulting.dll")
{
}
protected override Microsoft.CodeAnalysis.Metadata GetMetadataImpl()
=> throw new InvalidOperationException("transient metadata fault");
protected override Microsoft.CodeAnalysis.DocumentationProvider CreateDocumentationProvider()
=> Microsoft.CodeAnalysis.DocumentationProvider.Default;
protected override Microsoft.CodeAnalysis.PortableExecutableReference WithPropertiesImpl(
Microsoft.CodeAnalysis.MetadataReferenceProperties properties) => new FaultingReference();
}
}
@@ -4,9 +4,8 @@ namespace ZB.MOM.WW.ScadaBridge.TemplateEngine.Tests.Validation;
/// <summary>
/// The verdict cache's eviction policy. Overflow used to <c>Clear()</c> the
/// whole cache, which re-opened the non-collectible <c>InteractiveAssemblyLoader</c>
/// leak the cache exists to bound — every hot script had to be recompiled, and every
/// recompile loads another assembly that can never be unloaded. These tests pin the
/// whole cache, so every hot script had to be re-analysed and recompiled at once.
/// These tests pin the
/// replacement: eviction is segmented, so entries in active use survive it.
///
/// <para>
@@ -259,4 +259,53 @@ public class ScriptCompilerTests
Assert.Contains("'Beta'", second.Error);
Assert.Contains("compile", second.Error, StringComparison.OrdinalIgnoreCase);
}
// --- Review 2026-10-21 04/C1: transient faults must not become sticky verdicts ---
/// <summary>Verifies that a factory that throws leaves the cache slot empty, so the next call re-runs it.</summary>
[Fact]
public void GetOrAdd_FactoryThrows_NextCallReRunsFactory()
{
ScriptCompileVerdictCache.Clear();
Assert.Throws<InvalidOperationException>(() =>
ScriptCompileVerdictCache.GetOrAdd("SurfaceC1", "return 7;",
() => throw new InvalidOperationException("transient")));
var calls = 0;
var verdict = ScriptCompileVerdictCache.GetOrAdd("SurfaceC1", "return 7;", () =>
{
calls++;
return (true, null);
});
Assert.Equal(1, calls);
Assert.True(verdict.Ok);
Assert.Equal(0, ScriptCompileVerdictCache.Hits);
}
/// <summary>
/// Verifies that a compile gate that faults once reports a failure for that call
/// only — the next call with the same code re-runs the compile and succeeds.
/// </summary>
[Fact]
public void TryCompile_CompileFaultsOnce_FailureIsNotCached()
{
ScriptCompileVerdictCache.Clear();
var faults = 1;
var c = new ScriptCompiler(_ =>
{
if (faults-- > 0)
throw new System.IO.IOException("metadata read failed");
return [];
});
const string code = "return 8;";
var first = c.TryCompile(code, "Flaky");
Assert.True(first.IsFailure);
Assert.Contains("'Flaky'", first.Error);
Assert.Contains("metadata read failed", first.Error);
Assert.Equal(0, ScriptCompileVerdictCache.Count); // nothing memoised
Assert.True(c.TryCompile(code, "Flaky").IsSuccess); // pre-fix: sticky "failed to compile"
}
}