From 659e040d82bfea8a6bd80636785c7d24176e61e0 Mon Sep 17 00:00:00 2001 From: Max Obrist Date: Sun, 5 Apr 2026 18:46:42 +0700 Subject: [PATCH] fix(scribe): fix bug in using resolution --- .github/workflows/build-test.yml | 4 + Directory.Build.props | 19 +-- Directory.Build.targets | 24 +--- Scribe.Tests/QuillTests.cs | 118 ++++++++---------- Scribe/Quill.Usings.cs | 202 ++++++------------------------- 5 files changed, 90 insertions(+), 277 deletions(-) diff --git a/.github/workflows/build-test.yml b/.github/workflows/build-test.yml index f8afda3..763f69a 100644 --- a/.github/workflows/build-test.yml +++ b/.github/workflows/build-test.yml @@ -57,6 +57,10 @@ jobs: - name: Test run: dotnet test Scribe.slnx --configuration Release --no-build --verbosity normal --logger "trx;LogFileName=test-results.trx" + - name: Pack + if: github.event_name == 'push' || github.event_name == 'workflow_dispatch' + run: dotnet pack Scribe.slnx --configuration Release --no-build --output .artifacts/packages + - name: Upload test results if: always() uses: actions/upload-artifact@v4 diff --git a/Directory.Build.props b/Directory.Build.props index 52d10ea..16daa5a 100644 --- a/Directory.Build.props +++ b/Directory.Build.props @@ -5,12 +5,8 @@ that builds and packs in isolation. ============================================================ --> - - true - true - false @@ -55,17 +51,7 @@ Version is managed by Nerdbank.GitVersioning (version.json). ============================================================ --> - - true - false - + $(ArtifactsPath)packages/ Max Obrist Bullets for Humanity @@ -74,9 +60,6 @@ https://github.com/BulletsForHumanity/Scribe https://github.com/BulletsForHumanity/Scribe README.md - - true - snupkg true true diff --git a/Directory.Build.targets b/Directory.Build.targets index bdeaaf8..5858de9 100644 --- a/Directory.Build.targets +++ b/Directory.Build.targets @@ -1,25 +1,3 @@ - - - - - <_DevBuildStamp>$([System.DateTime]::UtcNow.ToString("yyyyMMddHHmmss")) - $(PackageVersion)-dev.$(_DevBuildStamp) - $(NuGetPackageVersion)-dev.$(_DevBuildStamp) - - + diff --git a/Scribe.Tests/QuillTests.cs b/Scribe.Tests/QuillTests.cs index 0054ae6..a3f0dac 100644 --- a/Scribe.Tests/QuillTests.cs +++ b/Scribe.Tests/QuillTests.cs @@ -560,11 +560,16 @@ public void ContentResult_ImplicitConversion_AllowsFluent() } // ── Global Reference Resolution ──────────────────────────────────── + // + // ResolveGlobalReferences only shortens global:: references whose namespace + // is already registered via Using()/Usings(). Unregistered global:: refs + // stay as-is — the generator author must register the namespaces they need. [Fact] public void Global_ResolvesGlobalRefs_ToShortNames() { var q = new Quill(); + q.Using("System.Collections.Generic"); q.Line("var x = new global::System.Collections.Generic.List();"); var result = q.Inscribe(); @@ -577,6 +582,7 @@ public void Global_ResolvesGlobalRefs_ToShortNames() public void Global_ResolvesMultipleDistinctRefs() { var q = new Quill(); + q.Usings("System", "System.Text.Json"); q.Line("global::System.Guid id = default;"); q.Line("global::System.Text.Json.JsonSerializer.Serialize(id);"); var result = q.Inscribe(); @@ -589,37 +595,21 @@ public void Global_ResolvesMultipleDistinctRefs() } [Fact] - public void Global_ConflictingTypeNames_CreatesAliases() + public void Global_UnregisteredNamespace_StaysAsIs() { var q = new Quill(); - q.Line("global::Foo.Bar.Widget w1 = default;"); - q.Line("global::Baz.Qux.Widget w2 = default;"); + // No Using() call — global:: should stay + q.Line("global::Foo.Bar.Widget w = default;"); var result = q.Inscribe(); - result.ShouldNotContain("global::"); - result.ShouldContain("BarWidget w1 = default;"); - result.ShouldContain("QuxWidget w2 = default;"); - result.ShouldContain("using BarWidget = Foo.Bar.Widget;"); - result.ShouldContain("using QuxWidget = Baz.Qux.Widget;"); - } - - [Fact] - public void Global_ConflictingTypeNames_WalksUpNamespace_UntilUnique() - { - var q = new Quill(); - q.Line("global::A.Utils.Helper h1 = default;"); - q.Line("global::B.Utils.Helper h2 = default;"); - var result = q.Inscribe(); - - result.ShouldNotContain("global::"); - result.ShouldContain("AUtilsHelper h1 = default;"); - result.ShouldContain("BUtilsHelper h2 = default;"); + result.ShouldContain("global::Foo.Bar.Widget w = default;"); } [Fact] public void Global_DeduplicatesSameTypeUsedMultipleTimes() { var q = new Quill(); + q.Using("System"); q.Line("global::System.Guid a = default;"); q.Line("global::System.Guid b = default;"); var result = q.Inscribe(); @@ -636,7 +626,7 @@ public void Global_DeduplicatesSameTypeUsedMultipleTimes() public void Global_PreservesManualUsings_AlongsideResolved() { var q = new Quill(); - q.Using("System.Linq"); + q.Usings("System", "System.Linq"); q.Line("global::System.Guid id = default;"); var result = q.Inscribe(); @@ -657,36 +647,37 @@ public void Global_NoGlobalRefs_DoesNotAddUsings() } [Fact] - public void Global_MultipleRefsResolved_WithoutRegistration() + public void Global_MemberAccess_ResolvedCorrectlyWithUsing() { + // global::System.Globalization.CultureInfo.InvariantCulture — CultureInfo is a type, + // InvariantCulture is a static property. With "System.Globalization" registered, + // the global:: prefix is stripped and the remainder is CultureInfo.InvariantCulture. var q = new Quill(); - q.Line("global::System.Guid id = default;"); - q.Line("global::System.Text.Json.JsonSerializer.Serialize(id);"); + q.Using("System.Globalization"); + q.Line("var c = global::System.Globalization.CultureInfo.InvariantCulture;"); var result = q.Inscribe(); - result.ShouldContain("using System;"); - result.ShouldContain("using System.Text.Json;"); + result.ShouldContain("using System.Globalization;"); + result.ShouldContain("var c = CultureInfo.InvariantCulture;"); result.ShouldNotContain("global::"); } [Fact] - public void Global_MemberAccess_UseUsingForChainedAccess() + public void Global_MemberAccess_WithoutUsing_StaysAsIs() { - // Auto-discovery can't distinguish type boundaries from member access chains. - // For global::Type.Property.Method(...) patterns, use Using() + short type name instead. + // Without a registered using, global:: stays — no guessing. var q = new Quill(); - q.Using("System"); - q.Line("return StringComparer.Ordinal.GetHashCode(Key);"); + q.Line("var c = global::System.Globalization.CultureInfo.InvariantCulture;"); var result = q.Inscribe(); - result.ShouldContain("using System;"); - result.ShouldContain("return StringComparer.Ordinal.GetHashCode(Key);"); + result.ShouldContain("global::System.Globalization.CultureInfo.InvariantCulture"); } [Fact] public void Global_InGenericTypeArgument_ResolvesCorrectly() { var q = new Quill(); + q.Usings("System", "System.Collections.Generic"); q.Line("var items = new global::System.Collections.Generic.List();"); var result = q.Inscribe(); @@ -700,6 +691,7 @@ public void Global_InGenericTypeArgument_ResolvesCorrectly() public void Global_InNestedGenerics_ResolvesAll() { var q = new Quill(); + q.Using("System.Collections.Generic"); q.Line("var x = new global::System.Collections.Generic.Dictionary>();"); var result = q.Inscribe(); @@ -712,6 +704,7 @@ public void Global_InNestedGenerics_ResolvesAll() public void Global_InInterfaceList_ResolvesCorrectly() { var q = new Quill(); + q.Using("System"); q.Line("public partial record Amount : global::System.IParsable, global::System.IComparable"); var result = q.Inscribe(); @@ -726,6 +719,7 @@ public void Global_InInterfaceList_ResolvesCorrectly() public void Global_NullableType_ResolvesCorrectly() { var q = new Quill(); + q.Using("System"); q.Line("public static bool TryParse(string? s, global::System.IFormatProvider? provider, out Amount result)"); var result = q.Inscribe(); @@ -738,6 +732,7 @@ public void Global_NullableType_ResolvesCorrectly() public void Global_InConstructor_ResolvesCorrectly() { var q = new Quill(); + q.Using("System"); q.Line("throw new global::System.ArgumentException(\"bad\", nameof(value));"); var result = q.Inscribe(); @@ -750,6 +745,7 @@ public void Global_InConstructor_ResolvesCorrectly() public void Global_InTypeof_ResolvesCorrectly() { var q = new Quill(); + q.Using("System"); q.Line("if (type == typeof(global::System.Guid))"); var result = q.Inscribe(); @@ -761,6 +757,7 @@ public void Global_InTypeof_ResolvesCorrectly() public void Global_SameNamespaceMultipleTypes_SingleUsing() { var q = new Quill(); + q.Using("System"); q.Line("global::System.Guid id = default;"); q.Line("throw new global::System.ArgumentException(\"bad\");"); q.Line("global::System.IntPtr ptr = default;"); @@ -773,50 +770,28 @@ public void Global_SameNamespaceMultipleTypes_SingleUsing() } [Fact] - public void Global_ThreeWayConflict_DisambiguatesAll() - { - var q = new Quill(); - q.Line("global::A.X.Config a = default;"); - q.Line("global::B.Y.Config b = default;"); - q.Line("global::C.Z.Config c = default;"); - var result = q.Inscribe(); - - result.ShouldNotContain("global::"); - // All three should have unique aliases - result.ShouldContain("XConfig a = default;"); - result.ShouldContain("YConfig b = default;"); - result.ShouldContain("ZConfig c = default;"); - result.ShouldContain("using XConfig = A.X.Config;"); - result.ShouldContain("using YConfig = B.Y.Config;"); - result.ShouldContain("using ZConfig = C.Z.Config;"); - } - - [Fact] - public void Global_ConflictAndNonConflict_MixedCorrectly() + public void Global_LongestNamespaceMatchWins() { + // When both "System" and "System.Text.Json" are registered, + // "global::System.Text.Json.JsonSerializer" should match the longer namespace. var q = new Quill(); - q.Line("global::System.Guid id = default;"); - q.Line("global::Foo.Widget a = default;"); - q.Line("global::Bar.Widget b = default;"); + q.Usings("System", "System.Text.Json"); + q.Line("global::System.Text.Json.JsonSerializer.Serialize(id);"); var result = q.Inscribe(); + result.ShouldContain("JsonSerializer.Serialize(id);"); + // Should NOT produce "Text.Json.JsonSerializer" from matching "System" only + result.ShouldNotContain("Text.Json.JsonSerializer"); result.ShouldNotContain("global::"); - result.ShouldContain("using System;"); - result.ShouldContain("Guid id = default;"); - result.ShouldContain("FooWidget a = default;"); - result.ShouldContain("BarWidget b = default;"); - result.ShouldContain("using FooWidget = Foo.Widget;"); - result.ShouldContain("using BarWidget = Bar.Widget;"); } [Fact] public void Global_InMultiLineBlock_ResolvesAcrossLines() { var q = new Quill(); - q.Using("System.Threading.Tasks"); + q.Usings("System.Threading", "System.Threading.Tasks"); using (q.Block("public global::System.Threading.Tasks.Task RunAsync(global::System.Threading.CancellationToken cancellationToken)")) { - // Use short name for member access chains (Task.CompletedTask) q.Line("return Task.CompletedTask;"); } @@ -830,24 +805,25 @@ public void Global_InMultiLineBlock_ResolvesAcrossLines() } [Fact] - public void Global_WithStaticPropertyAccess_UseUsingForEnumMembers() + public void Global_WithStaticPropertyAccess_ResolvesWithRegisteredUsing() { - // global::System.StringComparison.Ordinal is ambiguous — the scanner can't tell - // that StringComparison is the type and Ordinal is the enum value. - // Use Using() + short name for these patterns. + // With the namespace registered, global::System.StringComparison.Ordinal + // correctly becomes StringComparison.Ordinal. var q = new Quill(); q.Using("System"); - q.Line("string.Equals(a, b, StringComparison.Ordinal);"); + q.Line("string.Equals(a, b, global::System.StringComparison.Ordinal);"); var result = q.Inscribe(); result.ShouldContain("using System;"); result.ShouldContain("string.Equals(a, b, StringComparison.Ordinal);"); + result.ShouldNotContain("global::"); } [Fact] public void Global_UsingsAreSorted() { var q = new Quill(); + q.Usings("System", "System.Collections.Generic", "System.Threading.Tasks"); q.Line("global::System.Threading.Tasks.Task t = default;"); q.Line("global::System.Collections.Generic.List l = default;"); q.Line("global::System.Guid g = default;"); @@ -927,6 +903,7 @@ public void Alias_ExplicitAlias_ReturnValue_IsAliasName() public void Alias_CoexistsWithGlobalResolution() { var q = new Quill(); + q.Using("System"); var w = q.Alias("Foo.Bar", "Widget", "FooWidget"); q.Line($"{w} a = default;"); q.Line("global::System.Guid id = default;"); @@ -945,6 +922,7 @@ public void Alias_CoexistsWithGlobalResolution() public void FullIntegration_ProducesCorrectOutput() { var q = new Quill(); + q.Using("System"); q.FileNamespace("TestDomain"); using (q.Block("public partial record Amount : global::System.IParsable")) diff --git a/Scribe/Quill.Usings.cs b/Scribe/Quill.Usings.cs index 81d0de7..f4b7c84 100644 --- a/Scribe/Quill.Usings.cs +++ b/Scribe/Quill.Usings.cs @@ -96,192 +96,62 @@ private string GenerateAliasName(string ns, string typeName) // ── Global Reference Resolution ───────────────────────────────────── /// - /// Scans the body for registered global:: type references, resolves them to short - /// names (adding using directives), and disambiguates conflicts with aliases. + /// Replaces global::Ns.Type references in the body with short names + /// when Ns matches a namespace already registered via + /// or . Unmatched global:: references are left as-is. + /// Registered namespaces are tried longest-first (by segment count) so that + /// System.Text.Json is preferred over System.Text. /// private void ResolveGlobalReferences() { - var body = _body.ToString(); - - // Auto-discover all global:: references in the body - var refs = new Dictionary( - StringComparer.Ordinal - ); - var searchFrom = 0; - while (true) - { - var idx = body.IndexOf("global::", searchFrom, StringComparison.Ordinal); - if (idx < 0) - break; - - // Extract the full dotted identifier: global::Some.Namespace.TypeName - // Stops at the first character that is not part of a qualified C# identifier. - // Member access chains (global::System.StringComparer.Ordinal) will be consumed - // whole — use Using() + short names for those patterns. - var start = idx; - var end = idx + "global::".Length; - while (end < body.Length && (char.IsLetterOrDigit(body[end]) || body[end] == '.' || body[end] == '_')) - end++; - - // Trim trailing dot if the identifier ended on one - while (end > start + "global::".Length && body[end - 1] == '.') - end--; - - var fqn = body.Substring(start, end - start); - - // If '(' follows immediately, the last segment MAY be a method call — peel it - // back so the type reference is what remains. But don't peel when the global:: - // is preceded by 'new ' or 'typeof(' — those indicate the whole thing is a type. - if (end < body.Length && body[end] == '(') - { - var isConstructor = false; - if (start >= 4) - { - var before = body.Substring(Math.Max(0, start - 7), Math.Min(7, start)); - isConstructor = before.EndsWith("new ", StringComparison.Ordinal) - || before.EndsWith("typeof(", StringComparison.Ordinal); - } - - if (!isConstructor) - { - var dotPos = fqn.LastIndexOf('.'); - if (dotPos > "global::".Length) - { - fqn = fqn.Substring(0, dotPos); - end = start + fqn.Length; - } - } - } - - searchFrom = end; - - if (refs.ContainsKey(fqn)) - continue; - - var path = fqn.Substring("global::".Length); - var lastDot = path.LastIndexOf('.'); - if (lastDot < 0) - continue; - - refs[fqn] = (path.Substring(0, lastDot), path.Substring(lastDot + 1)); - } + if (_usings.Count == 0) + return; - if (refs.Count == 0) + var body = _body.ToString(); + if (body.IndexOf("global::", StringComparison.Ordinal) < 0) return; - // Group by short type name to detect conflicts - var byTypeName = new Dictionary< - string, - List> - >(StringComparer.Ordinal); - foreach (var kvp in refs) + // Sort registered namespaces longest-first by segment count, then by length, + // so "System.Text.Json.Serialization" is tried before "System.Text.Json". + var orderedNamespaces = new List(_usings.Count); + foreach (var ns in _usings) { - if (!byTypeName.TryGetValue(kvp.Value.TypeName, out var list)) - { - list = new List>(); - byTypeName[kvp.Value.TypeName] = list; - } - - list.Add(kvp); + // Skip alias entries ("AliasName = Ns.Type") — they are not plain namespaces. + if (ns.IndexOf('=') >= 0) + continue; + orderedNamespaces.Add(ns); } - // Build replacement map - var replacements = new Dictionary(StringComparer.Ordinal); - foreach (var group in byTypeName) + orderedNamespaces.Sort((a, b) => { - if (group.Value.Count == 1) - { - // No conflict — add using, replace with short name - var entry = group.Value[0]; - _usings.Add(entry.Value.Namespace); - replacements[entry.Key] = entry.Value.TypeName; - } - else - { - // Conflict — create disambiguated aliases - var aliases = DisambiguateAliases(group.Value); - foreach (var (globalRef, alias, ns, typeName) in aliases) - { - _usings.Add($"{alias} = {ns}.{typeName}"); - replacements[globalRef] = alias; - } - } - } - - // Apply replacements (longest match first to avoid partial replacements) - foreach (var from in replacements.Keys.OrderByDescending(static k => k.Length)) + var aSeg = CountDots(a) + 1; + var bSeg = CountDots(b) + 1; + if (bSeg != aSeg) + return bSeg.CompareTo(aSeg); + return b.Length.CompareTo(a.Length); + }); + + // For each registered namespace, find and replace "global::." with "" + // effectively leaving just the remainder (type name + any member access). + foreach (var ns in orderedNamespaces) { - body = body.Replace(from, replacements[from]); + var token = "global::" + ns + "."; + body = body.Replace(token, ""); } _body.Clear(); _body.Append(body); } - /// - /// Given a set of conflicting types (same short name, different namespaces), produces - /// disambiguated alias names by walking up namespace segments until each alias is unique. - /// - private static List<( - string GlobalRef, - string Alias, - string Namespace, - string TypeName - )> DisambiguateAliases(List> entries) + private static int CountDots(string s) { - var typeName = entries[0].Value.TypeName; - var nsParts = new List(entries.Count); - foreach (var e in entries) - nsParts.Add(e.Value.Namespace.Split('.')); - - // Walk backwards through namespace segments until aliases are unique - var depth = 1; - while (depth <= nsParts.Max(static p => p.Length)) - { - var candidates = new List(entries.Count); - for (var i = 0; i < entries.Count; i++) - { - var parts = nsParts[i]; - // Take the last `depth` namespace segments and prepend to type name - var startIdx = Math.Max(0, parts.Length - depth); - var prefix = string.Join("", parts, startIdx, parts.Length - startIdx); - candidates.Add(prefix + typeName); - } - - if (candidates.Distinct(StringComparer.Ordinal).Count() == candidates.Count) - { - // All aliases are unique at this depth - var result = new List<(string, string, string, string)>(entries.Count); - for (var i = 0; i < entries.Count; i++) - result.Add( - ( - entries[i].Key, - candidates[i], - entries[i].Value.Namespace, - entries[i].Value.TypeName - ) - ); - return result; - } - - depth++; - } - - // Fallback: use full namespace as prefix (should never happen in practice) - var fallback = new List<(string, string, string, string)>(entries.Count); - for (var i = 0; i < entries.Count; i++) + var count = 0; + for (var i = 0; i < s.Length; i++) { - var fullPrefix = entries[i].Value.Namespace.Replace(".", ""); - fallback.Add( - ( - entries[i].Key, - fullPrefix + typeName, - entries[i].Value.Namespace, - entries[i].Value.TypeName - ) - ); + if (s[i] == '.') + count++; } - return fallback; + return count; } }