Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
68 changes: 68 additions & 0 deletions .github/copilot-instructions.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,68 @@
# Copilot PR-review instructions for ANcpLua.Roslyn.Utilities

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

The Pull Request title and description do not match the changes provided in this patch. The title and summary describe CI workflow updates (specifically grep flags and YAML expression parentheses), but the actual change is the addition of a Copilot instructions markdown file. This discrepancy should be resolved to ensure the PR metadata accurately reflects its contents.


Utility library for Roslyn incremental generators, analyzers, and code-fix
providers. Ships five NuGet packages: a binary library, a `Sources` source-only
package (rewritten to `internal` at pack via `Transform-Sources.ps1`), `Polyfills`,
`Testing` (analyzer/generator/codefix harness), and `Testing.Aot`. Main library +
`Sources` target `netstandard2.0` so Roslyn-hosted consumers can absorb them;
`Testing` targets `net10.0`. `TreatWarningsAsErrors=true` repo-wide; all analyzer

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The text refers to net10.0 as a target framework. As of the current release cycle, .NET 9 is the latest version, and .NET 10 is not yet available. This is likely a typo for net8.0 or net9.0. This also appears on line 60.

diagnostics elevated to error in `.editorconfig`. This file scopes to PR review only.

## Flag

- New API in the main library (or `Sources`) that pulls in dependencies not
available on `netstandard2.0` — `System.Text.Json`, modern BCL methods, anything
ns2.1+. Such code can land in `Testing` or downstream consumers, but not in
the analyzer-hosted surface.
- New `public` type or member added to the `Sources` project without a
`#if ANCPLUA_ROSLYN_PUBLIC public #else internal #endif` guard — the Sources
package rewrites visibility on pack, so adding bare `public` breaks the
internal-on-pack contract.
- Storing `ISymbol`, `SyntaxNode`, `SyntaxTree`, or any Roslyn reference type as
a `HashSet<T>` / `Dictionary<TKey, …>` key — reference equality, cache misses
on rebuild, breaks generator incrementality.
- New allocations in generator hot paths: `.ToArray()` / `.ToList()` on a `Span`

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The Span<T> type does not have a .ToList() extension method in the standard .NET BCL. If the intent is to flag heap allocations from converting a span to a collection, .ToArray() is the correct method. If you intended to cover general collection allocations, consider clarifying the types involved.

Suggested change
- New allocations in generator hot paths: `.ToArray()` / `.ToList()` on a `Span`
- New allocations in generator hot paths: .ToArray() on a Span

inside `Append` loops, capturing closures in `Dict.GetOrAdd`/`GetOrInsert`,
boxing of value types via `object`. The `closure-free` `GetOrInsert<TContext>`
+ `static` lambda + `ValueStringBuilder` patterns are the established shape.
- Public types intended for generator payloads added without value equality —
use `readonly record struct` (or `EquatableArray<T>` for collections), never
classes or non-record structs.
- File-I/O via `System.IO.File` / `System.IO.Directory` outside `Guard.cs` —
`Guard.cs` is the single allow-listed call site (suppressed `RS1035`).

## utilities-specific

- The `Sources` package is for source generators that can't take a binary
reference. Any new helper meant for that consumer set goes in
`src/ANcpLua.Roslyn.Utilities/` (the shared tree) and gets exposed via the
`Sources` package on pack.
- `Polyfills` package supplies `init`, `required`, `Index`/`Range`, nullable +
trim attributes for `netstandard2.0` consumers. Don't duplicate polyfills in
the main library.
- `Testing` ships a fluent generator/analyzer/codefix harness. New test helpers
belong there, not in individual consumer test projects.
- `EquatableArray<T>` is `ref`-struct-like (value-equality wrapper); use it for

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

EquatableArray<T> is a regular readonly struct, not a ref struct. Describing it as "ref-struct-like" is technically inaccurate and potentially misleading for an AI reviewer, especially since ref struct types cannot be used as fields in records (which the following line explicitly recommends). It is better described as a "value-type wrapper" or as having "value semantics".

Suggested change
- `EquatableArray<T>` is `ref`-struct-like (value-equality wrapper); use it for
- EquatableArray<T> is a value-type wrapper (providing value equality); use it for

collection fields in generator records, not `ImmutableArray<T>` (no equality)
or `T[]` (reference equality).

## Do not flag

- Allow-listed suppressions in `Guard.cs`: `#pragma warning disable RS1035`
(file I/O legal here, non-analyzer call sites only).
- Polyfills suppressions: `CA1019`, `RCS1251`, `IDE0300`, `CA1064`, `CA1812`,
`SA1623`, `RCS1157` — all on shim types.
- Testing-csproj suppressions: `NU1903`, `RS1036`, `RS1038`, `RS1041`, `CA1019`,
`NU5104`, `CA1859`, `RS0030`, `CA1307`, `IDE1006`, `CA1002`, `CA1000`,
`CS1574`, `CS1591` — test-infra concessions.
- `[ExcludeFromCodeCoverage]` only appears on polyfills, not on mainline types
— that's intentional, mainline gets coverage.
- `Testing` package's use of `System.Text.Json` and modern BCL — it's `net10.0`,
not the constrained surface.

## Project context

Solo-dev repo. The Sources package is consumed by `ANcpLua.Analyzers` and
`ANcpLua.Agents.Testing` (and downstream generators); regressions ripple. Breaking
changes are allowed in the same session — bump major, fix consumers, ship.
Don't suggest backwards-compat shims or feature flags within a single PR.
4 changes: 2 additions & 2 deletions .github/workflows/nuget-publish.yml
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@ jobs:
MAJOR=${BASH_REMATCH[1]}
MINOR=${BASH_REMATCH[2]}
PATCH=${BASH_REMATCH[3]}
if git tag --points-at HEAD | grep -qx "$LATEST_TAG"; then
if git tag --points-at HEAD | grep -qFx "$LATEST_TAG"; then
VERSION="$MAJOR.$MINOR.$PATCH"
echo "HEAD is tagged $LATEST_TAG — reusing version: $VERSION"
else
Expand Down Expand Up @@ -116,7 +116,7 @@ jobs:
# 3. No NUGET_API_KEY secret required — OIDC handles auth.
publish:
needs: [version, build]
if: github.event_name == 'push' && github.ref == 'refs/heads/main' || github.event_name == 'workflow_dispatch'
if: (github.event_name == 'push' && github.ref == 'refs/heads/main') || github.event_name == 'workflow_dispatch'
runs-on: ubuntu-latest
environment: nuget
permissions:
Expand Down