fix(scribe): fix bug in using resolution#1
Conversation
There was a problem hiding this comment.
Pull request overview
This PR changes Quill’s global:: reference resolution strategy in Inscribe() and updates the associated unit tests, alongside some build/CI packaging adjustments.
Changes:
- Reworked
ResolveGlobalReferences()to only shortenglobal::Ns.*whenNsis already registered viaUsing()/Usings(), using longest-namespace-first replacement. - Updated Quill tests to reflect the new “no guessing / no auto-adding usings” behavior and added coverage for longest-namespace match.
- Simplified local build props/targets and added an explicit
dotnet packstep to CI on pushes / workflow dispatch.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
Scribe/Quill.Usings.cs |
Replaces the global reference scanning + aliasing algorithm with a namespace-prefix stripping approach based on pre-registered usings. |
Scribe.Tests/QuillTests.cs |
Updates tests to require explicit Using()/Usings() for shortening and removes conflict/alias expectations from global resolution. |
Directory.Build.targets |
Removes local dev version suffixing target; leaves file intentionally empty to stop parent traversal. |
Directory.Build.props |
Removes local-build packaging toggles/symbol settings; keeps package output path and metadata. |
.github/workflows/build-test.yml |
Adds dotnet pack on push/workflow_dispatch and uploads packages as an artifact. |
| // For each registered namespace, find and replace "global::<ns>." 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, ""); | ||
| } |
There was a problem hiding this comment.
ResolveGlobalReferences now blindly strips the registered namespace prefix from any matching global::Ns.* token. If the body contains two different global::A.Widget and global::B.Widget references and both namespaces are registered via Using/Usings, this will collapse both to Widget and produce ambiguous/incorrect output. Consider restoring conflict detection/alias generation, or at minimum avoid shortening when multiple distinct FQNs share the same short type name (leave them as global::... or require/emit aliases).
| /// <summary> | ||
| /// Scans the body for registered <c>global::</c> type references, resolves them to short | ||
| /// names (adding <c>using</c> directives), and disambiguates conflicts with aliases. | ||
| /// Replaces <c>global::Ns.Type</c> references in the body with short names | ||
| /// when <c>Ns</c> matches a namespace already registered via <see cref="Using"/> | ||
| /// or <see cref="Usings"/>. Unmatched <c>global::</c> references are left as-is. | ||
| /// Registered namespaces are tried longest-first (by segment count) so that | ||
| /// <c>System.Text.Json</c> is preferred over <c>System.Text</c>. |
There was a problem hiding this comment.
The global:: resolution behavior has changed from “auto-discover global refs, add required using directives, and disambiguate conflicts via aliases” to “only shorten when the namespace is already registered”. Several public docs and API XML comments still describe the old behavior (e.g., docs/quill-reference.md, docs/architecture-quill.md, docs/writing-generators.md, and Quill.Inscribe() XML remarks). Please update those in the same PR to keep documentation in sync with the new contract (or revert the behavioral change).
| <Project> | ||
| <!-- | ||
| On local (non-CI) builds, append -dev to the package version so locally produced | ||
| packages are never version-identical to NuGet.org releases. The condition guards | ||
| against double-suffixing: NBGV already adds a git-hash pre-release tag on | ||
| non-public branches, so we only append -dev when the version is still clean. | ||
| --> | ||
| <Target | ||
| Name="AppendDevVersionSuffix" | ||
| AfterTargets="GetBuildVersion" | ||
| Condition="'$(IsLocalBuild)' == 'true' and '$(IsPackable)' != 'false'" | ||
| > | ||
| <PropertyGroup> | ||
| <!-- Timestamp gives a unique, always-incrementing counter per build without needing a counter file. | ||
| Format: yyyyMMddHHmmss — a valid SemVer2 numeric identifier that sorts chronologically. --> | ||
| <_DevBuildStamp>$([System.DateTime]::UtcNow.ToString("yyyyMMddHHmmss"))</_DevBuildStamp> | ||
| <PackageVersion Condition="!$(PackageVersion.Contains('-'))" | ||
| >$(PackageVersion)-dev.$(_DevBuildStamp)</PackageVersion | ||
| > | ||
| <NuGetPackageVersion Condition="!$(NuGetPackageVersion.Contains('-'))" | ||
| >$(NuGetPackageVersion)-dev.$(_DevBuildStamp)</NuGetPackageVersion | ||
| > | ||
| </PropertyGroup> | ||
| </Target> | ||
| <!-- Intentionally empty — stops MSBuild from walking up to parent directories. --> | ||
| </Project> |
There was a problem hiding this comment.
This PR is titled as a Quill using-resolution bug fix, but it also removes local version-suffixing/auto-pack logic (Directory.Build.props/targets) and changes CI to run dotnet pack on pushes. If these build/pipeline changes are required, consider documenting the rationale in the PR description or splitting into a separate PR to keep the scope focused.
No description provided.