From 6126f7fbf1460b4cb4e7f0caedba83f933650ee3 Mon Sep 17 00:00:00 2001 From: Kieron Lanning Date: Tue, 3 Nov 2026 14:31:55 +0000 Subject: [PATCH] fix(logging): honour level-specific attribute arguments --- .../SKILL.md | 146 +++++++++++ .../SKILL.md | 22 +- .../SKILL.md | 12 +- .../purview-sourcegenerator-testing/SKILL.md | 44 +++- .../SKILL.md | 18 +- AGENTS.md | 31 ++- CHANGELOG.md | 26 ++ Directory.Packages.props | 2 +- docs/wiki/Contributing.md | 2 +- package.json | 2 +- skills-lock.json | 14 +- skills/purview-telemetry-diagnostics/SKILL.md | 9 + skills/purview-telemetry-migration/SKILL.md | 22 +- .../Emitters/GeneratedTypesEmitter.cs | 8 +- .../LoggerGenTargetClassEmitter.Methods.cs | 13 +- .../Records/AttributeDataModels.cs | 2 + .../InstalledSkillTests.cs | 35 ++- ...gingTests.SpecificLogAttributeArguments.cs | 162 +++++++++++++ .../Records/LogAttributeDataModelTests.cs | 229 ++++++++++++++++++ 19 files changed, 759 insertions(+), 40 deletions(-) create mode 100644 .agents/skills/purview-sourcegenerator-attribute-data-models/SKILL.md create mode 100644 src/tests/SourceGenerator.IntegrationTests/Generators/TelemetrySourceGeneratorLoggingTests.SpecificLogAttributeArguments.cs create mode 100644 src/tests/SourceGenerator.IntegrationTests/Records/LogAttributeDataModelTests.cs diff --git a/.agents/skills/purview-sourcegenerator-attribute-data-models/SKILL.md b/.agents/skills/purview-sourcegenerator-attribute-data-models/SKILL.md new file mode 100644 index 00000000..714ddbff --- /dev/null +++ b/.agents/skills/purview-sourcegenerator-attribute-data-models/SKILL.md @@ -0,0 +1,146 @@ +--- +name: purview-sourcegenerator-attribute-data-models +description: >- + Use when declaring or fixing a [Generate] attribute-data model in Purview.SourceGeneratorFramework - + mapping constructor and named arguments with [Argument]/[Property], defaults, reusing a shape with + [NestedModel], reading an ADM0001-ADM0015 diagnostic, or testing that a model reads the values an + attribute application supplies. +metadata: + product: Purview.SourceGeneratorFramework + repository: purview-dev/sourcegenerator-framework +--- + +# Attribute-data models + +`AttributeDataModelGenerator` turns a `readonly partial record struct` marked `[Generate(typeof(TAttribute))]` +into a parser for `TAttribute`'s `AttributeData`: `Exists`, `Empty`, `TargetAttribute`, the guarded +`FromAttributeData`/`TryFromAttributeData`/`AllAttributeData` overloads, and `FromAttributeDataUnchecked`. +The generated model is a value type, so it is safe to hold in an incremental pipeline (an `AttributeData` +is not). + +The one thing to keep in mind: **the generated code looks names up case-sensitively at run time, and a +name it cannot find quietly yields the default value.** A mapping mistake compiles and produces `null`/`0`. +The naming analyzer (`ADM0011`–`ADM0015`) exists to report those mistakes where they are declared; do not +suppress it without understanding the finding, and test the model against real applications. + +## Mapping each property + +Each primary-constructor parameter becomes a property, PascalCased (`message_template` → `MessageTemplate`). +Its sources are read in this order, first hit wins: + +| Declaration | Reads | +| --- | --- | +| *(no mapping attribute)* | the named argument with the property's name: `[Log(EventId = 7)]` | +| `[Property]` / `[Property(Name = "X")]` | the named argument `X` (default: the property name) | +| `[Argument("messageTemplate")]` | the constructor parameter **named** `messageTemplate` | +| `[Argument]` | the constructor parameter named after the property, camel-cased: `MessageTemplate` → `messageTemplate` | +| `[Argument(1)]` | the constructor argument at **position** 1 | +| `[Argument(...)] [Property]` | the named argument first, then the constructor argument | +| `[NestedModel]` | another generated model, read from the same attribute (below) | +| `[GenericTypeArgument]` | a generic type argument of the attribute class, as a `TypeIdentity` | +| `[Exclude]` | nothing; the parameter is left out of the model | + +Methodology: + +- **Map a constructor parameter by name, not by index.** A name survives a new overload or a reordered + parameter list; an index silently starts reading a different parameter. +- **Map both sources when the attribute accepts both.** `[Argument("name")] [Property] string? Name` reads + `[A(name: "x")]`, `[A("x")]` and `[A(Name = "x")]`. The named argument is read first because it is assigned + after the constructor runs, so it is the attribute instance's effective value. +- **Only map `[Property]` to a public settable property or field.** A property with a private or `internal` + setter can never be a named argument; read it through the constructor parameter that sets it (`ADM0012`). +- **Match case exactly.** `[Argument("MessageTemplate")]` never matches a parameter called + `messageTemplate` (`ADM0011`). +- **Give every property that might be absent a `DefaultValue`**: `[Property(DefaultValue = -1)] int EventId`. + A missing argument takes it; without one, it takes `default(T)`. `Empty` always uses `default(T)`. +- **Use `IsEnum = true` with a `string` property** when the enum type is not available to the generator; a + bare member-name default (`DefaultValue = "Inherit"`) is expanded to the fully-qualified member. +- **Never name a property `Exists`, `Empty`, `TargetAttribute`, `AllAttributeData`, `FromAttributeData`, + `TryFromAttributeData` or `FromAttributeDataUnchecked`** — they are generated (`ADM0014`, generation is + skipped). Keyword names (`Event`, `Class`) are fine. + +## Reusing a shape with `[NestedModel]` + +A `[NestedModel]` parameter is populated by the nested model's `FromAttributeDataUnchecked`, from the +**outer** attribute's arguments, with no `TargetAttribute` check. The outer attribute does not have to derive +from the nested model's target; it only has to use the same constructor parameter and property names. + +```csharp +[Generate(typeof(LogAttribute))] +readonly partial record struct LogAttributeData( + [Argument("level")] [Property(DefaultValue = 99)] int Level, + [Argument("messageTemplate")] [Property] string? MessageTemplate, + [Argument("eventId")] [Property(DefaultValue = -1)] int EventId, + [Property] string? Name +); + +// InfoAttribute has messageTemplate, eventId and Name, but no level. +[Generate(typeof(InfoAttribute))] +readonly partial record struct InfoAttributeData([NestedModel] LogAttributeData Log); +``` + +- The nested model's `Exists` is always `true`; a mapping the outer attribute does not declare (`level` + here) takes its `DefaultValue`. `ADM0013` (info) lists such mappings, so an intended absence is visible + and an accidental one is caught. +- Map nested properties **by name**. A constructor index means a different parameter on each attribute + (`ADM0015`). +- `MatchByInheritance = true` is for using a model on its own against derived attributes. A nested model + does not need it. +- Call `FromAttributeData`/`TryFromAttributeData` from generator code. `FromAttributeDataUnchecked` skips the + type check and is for nested models. + +## Diagnostics + +| Rule | Severity | Meaning and fix | +| --- | --- | --- | +| `ADM0001` | Error | The `[Generate]` target cannot be resolved. Use `typeof`, a resolvable full name, or a type-library `…FullName` constant. | +| `ADM0002` | Error | Unsupported property type (array, pointer, function pointer). | +| `ADM0003` | Error | `[Argument("name")]`/`[Argument(index)]` names no constructor parameter of the target. | +| `ADM0004` | Error | A `[NestedModel]` type is not itself a `[Generate]` model. | +| `ADM0005` | Error | A `DefaultValue` cannot be emitted for the property type. | +| `ADM0006` | Error | A non-nullable reference property has no default. Add a `DefaultValue` or make it nullable. | +| `ADM0007` | Error | `AutoDiscover` needs the `typeof` target overload. | +| `ADM0008` | Error | A `[GenericTypeArgument]` property must be `TypeIdentity`. | +| `ADM0009` | Error | An `IsEnum` property must be `string`/`string?`. | +| `ADM0010` | Error | An `ISymbol`/`Type` property is not cacheable; use `TypeIdentity` or a string. | +| `ADM0011` | Warning | A mapped name matches an attribute member only when case is ignored, so it is never read. Fix the case. | +| `ADM0012` | Warning | The mapped named argument is not a public settable member of the attribute, or a bare `[Argument]` implies a constructor parameter it lacks. Map the member that actually carries the value. | +| `ADM0013` | Info | A nested model property reads only names the outer attribute lacks, so it always takes its default. Expected for a deliberately absent value; otherwise rename to match. | +| `ADM0014` | Error | A property has a generated member's name. Rename the parameter. | +| `ADM0015` | Warning | A nested model reads a constructor index that names a different parameter on the outer attribute. Map it by name. | + +## Testing a model + +Test the attribute side and the model side separately, against what Roslyn actually produces. Use +`AttributeDataFactory` from `Purview.SourceGeneratorFramework.Testing` to compile an application, and the +`AttributeDataAssertions` from the TUnit package to pin each argument by ordinal, parameter name or named +argument, including how it was supplied (`AttributeArgumentForm`): + +```csharp +var options = new MyTestOptions { AdditionalNamespaces = ["My.Attributes"] }; +var attribute = AttributeDataFactory.FromUsage("[Info(\"custom {Value}\", Name = \"N\")]", options); + +await Assert.That(attribute).HasConstructorArgument("messageTemplate", "custom {Value}"); +await Assert.That(attribute).HasConstructorArgument("eventId", -1, AttributeArgumentForm.Defaulted); +await Assert.That(attribute).HasNamedArgument("Name", "N"); + +var model = InfoAttributeData.FromAttributeData(attribute); +await Assert.That(model.Log.MessageTemplate).IsEqualTo("custom {Value}"); +await Assert.That(model.Log.Level).IsEqualTo(99); +``` + +Cover, for each model: positional, by-name and named-argument applications; an application omitting every +optional argument (proves the `DefaultValue`s); for a nested model, an outer attribute that lacks a mapped +member; and the nested target's own `FromAttributeData` returning `Empty` for the outer attribute. +`FromUsage` places the application on a declaration its `AttributeUsage` allows and throws on a compile +error, so a misspelt argument name fails the test. `FromSource` reads applications from a full source. + +Generated-code assertions (`Contains("FromAttributeDataUnchecked(attributeData)")`) prove *what* the +generator emits; the tests above prove the model *reads the right values*. Prefer the latter. + +## Related skills + +`purview-sourcegenerator-testing` for the runner layer, `AttributeDataFactory` and `AttributeArguments`; +`purview-sourcegenerator-tunit-tests` for the assertion extensions; `purview-sourcegenerator-pipeline` for +holding models in an incremental pipeline; `purview-sourcegenerator-diagnostics` for the other analyzer +families. diff --git a/.agents/skills/purview-sourcegenerator-diagnostics/SKILL.md b/.agents/skills/purview-sourcegenerator-diagnostics/SKILL.md index 21d9522c..b80ebe74 100644 --- a/.agents/skills/purview-sourcegenerator-diagnostics/SKILL.md +++ b/.agents/skills/purview-sourcegenerator-diagnostics/SKILL.md @@ -1,7 +1,7 @@ --- name: purview-sourcegenerator-diagnostics description: >- - Use when a Purview.SourceGeneratorFramework build reports a PSGFR, PSGF, PRSGD or TLB diagnostic, + Use when a Purview.SourceGeneratorFramework build reports a PSGFR, PSGF, PRSGD, TLB or ADM diagnostic, when choosing which code fix applies, or when a source generator, diagnostic analyzer or code fix provider is silently not running at all. metadata: @@ -106,8 +106,21 @@ Opt out with `PurviewSourceGeneratorFrameworkAnalyzerValidation=false` (`PRSGD00 ## Type-library and attribute-model diagnostics The bundled generators carry their own families: `TypeLibraryValidationAnalyzer` -(`TLB0001`–`TLB0019`, plus `TLB0021`) and the attribute-data-model validation analyzers. They are -documented on their own wiki pages, `Type-Library.md` and `Attribute-Data-Models.md`. +(`TLB0001`–`TLB0019`, plus `TLB0021`) and the attribute-data-model analyzers. They are documented on their +own wiki pages, `Type-Library.md` and `Attribute-Data-Models.md`. + +The attribute-data-model rules come from three analyzers: + +| Rules | Analyzer | Covers | +| --- | --- | --- | +| `ADM0001`–`ADM0009` | `AttributeDataModelValidationAnalyzer` | the target, property types, defaults, explicit constructor members | +| `ADM0010` | `AttributeDataModelSymbolPropertyAnalyzer` | non-cacheable `ISymbol`/`Type` properties | +| `ADM0011`–`ADM0015` | `AttributeDataModelNamingAnalyzer` | mapped names the generated code would never find, generated-member name clashes, `[NestedModel]` mappings the outer attribute does not line up with | + +Treat the naming rules as real defects: the generated code reads names case-sensitively and silently falls +back to a default, so a naming mistake never fails the build on its own. `ADM0013` is `Info` because a +nested model may deliberately read a value the outer attribute does not have; check each one. The +`purview-sourcegenerator-attribute-data-models` skill has the full table and the fix for each rule. ## Code fixes @@ -139,4 +152,5 @@ Most rules above have a fix in `Purview.SourceGeneratorFramework.CodeFixers`: ## Related skills `purview-sourcegenerator-codewriter` for the emission patterns the `CodeWriter` rules push you -towards, and `purview-sourcegenerator-testing` for proving a pipeline actually caches. +towards, `purview-sourcegenerator-testing` for proving a pipeline actually caches, and +`purview-sourcegenerator-attribute-data-models` for the `ADM` rules. diff --git a/.agents/skills/purview-sourcegenerator-packaging/SKILL.md b/.agents/skills/purview-sourcegenerator-packaging/SKILL.md index 92350db8..a0e7f802 100644 --- a/.agents/skills/purview-sourcegenerator-packaging/SKILL.md +++ b/.agents/skills/purview-sourcegenerator-packaging/SKILL.md @@ -76,10 +76,14 @@ identity. Both are `IsRoslynComponent`; only the generator references the framew - The referenced component is returned as **its own merged assembly** and is never IL-merged into the dependent one — that would duplicate its types, including `InternalsVisibleTo`-visible internals, inside a second analyzer in the same host. -- The framework assembly is declared as copy-to-output content beside the generator's `bin`, and - content with `CopyToOutputDirectory` flows transitively through `ProjectReference`. That is what - lets Visual Studio load the dependent code-fix provider from its own `bin` without a - `FileNotFoundException` for the framework assembly. +- The framework assembly is copied beside the generator's `bin` (a plain copy, not copy-to-output + content). A dependent component that references the generator as an assembly gets it copied + beside its own `bin` by ordinary assembly-reference resolution, which is what lets Visual Studio + load the dependent code-fix provider without a `FileNotFoundException` for the framework assembly. +- A project that references a component only as an analyzer (`ReferenceOutputAssembly="false"`) + never receives the framework assembly, and neither does any application downstream of it. Since + 1.2.2 it no longer leaks into app build output, `deps.json`, publish output, single-file bundles or + Blazor WebAssembly `_framework`, where it broke Native AOT and WebAssembly AOT publishing. - Visual Studio's project system resolves an `OutputItemType=Analyzer` project reference to the referenced project's **unmerged** `bin` assembly and adds it to the compiler's analyzers — the command-line build does not. `Purview.BuildSdk` removes that item before adding the resolved diff --git a/.agents/skills/purview-sourcegenerator-testing/SKILL.md b/.agents/skills/purview-sourcegenerator-testing/SKILL.md index aaeff09e..a6da554c 100644 --- a/.agents/skills/purview-sourcegenerator-testing/SKILL.md +++ b/.agents/skills/purview-sourcegenerator-testing/SKILL.md @@ -3,8 +3,9 @@ name: purview-sourcegenerator-testing description: >- Use when writing or fixing tests for source generators, diagnostic analyzers, code fixes or refactorings built on Purview.SourceGeneratorFramework - picking the right runner and base class, - configuring SourceGeneratorTestOptions, querying produced code with CodeQuery, and asserting - incremental pipeline caching stage by stage. + configuring SourceGeneratorTestOptions, querying produced code with CodeQuery, testing attribute-data + models against real AttributeData with AttributeDataFactory, and asserting incremental pipeline caching + stage by stage. metadata: product: Purview.SourceGeneratorFramework repository: purview-dev/sourcegenerator-framework @@ -167,8 +168,47 @@ an assertion, nor calling a helper that is handed the result — an extension on parameter: both hand the same instance back, so `await Assert.That(result).SomeAssertion()` and `result.AssertNoGenerationExceptions()` are not flagged. +## Testing attribute applications and attribute-data models + +A generator that reads attributes has two halves to test: what Roslyn gives it for an application, and what +its parsing (usually a generated attribute-data model) makes of that. `AttributeDataFactory` produces the +real `AttributeData` without running a generator; `AttributeArguments` describes it. + +```csharp +var options = new MyTestOptions { AdditionalNamespaces = ["My.Attributes"] }; + +// One application. Placed on a class, or on the first declaration its AttributeUsage allows. +AttributeData attribute = AttributeDataFactory.FromUsage("[Log(\"text\", eventId: 7, Name = \"N\")]", options); + +// Every application on a type, or on one of its members. +var onMember = AttributeDataFactory.FromSource(source, "Sample.IEvents", "Raised", options); + +var arguments = AttributeArguments.From(attribute); +arguments.TryGetConstructorArgument(0, out var byOrdinal); // #0 message: "text" +arguments.TryGetConstructorArgument("eventId", out var byName); // Form = NamedParameter +arguments.TryGetNamedArgument("Name", out var named); // Form = NamedArgument +arguments.ToString(); // [LogAttribute(#0 message: "text", #1 eventId: 7 (by name), Name = "N")] + +var model = LogAttributeData.FromAttributeData(attribute); +``` + +- References and usings come from the options: `AdditionalAssemblyTypes` for the assembly declaring the + attribute, `AdditionalNamespaces` for its namespace, or declare the attribute in `AdditionalSources`. +- A compilation error throws `InvalidOperationException` listing the errors, so a misspelt parameter name + fails the test rather than producing an application with a missing argument. +- `AttributeArgumentForm` tells `Positional`, `NamedParameter` (`message: "x"`), `Defaulted` (an optional + parameter that was not written) and `NamedArgument` (`Name = "x"`) apart, which `AttributeData` alone + cannot. +- `AttributeArgument.ValueEquals` compares numbers across types, enums by value or member name, `typeof` + by `Type`/`TypeIdentity`/name, and arrays element-wise. Names are case-sensitive, as generated models are. +- The TUnit assertions over these (`HasConstructorArgument`, `HasNamedArgument`, …) are in + `purview-sourcegenerator-tunit-tests`; what to cover for a model is in + `purview-sourcegenerator-attribute-data-models`. + ## Best practices +- **Test attribute parsing against real `AttributeData`** (`AttributeDataFactory`), not only by asserting + on emitted parsing code: emitted code can look right and still read the wrong name. - **Deterministic output**: `WriteAutoGeneratedHeader` is timestamp-free; assert with `ContainsGeneratedCode`/`GeneratesCode` (whitespace-flattened) or `CodeQuery`, never with timestamps. - **Generator references**: to use a generated type in the test project AND pass the generator type to a diff --git a/.agents/skills/purview-sourcegenerator-tunit-tests/SKILL.md b/.agents/skills/purview-sourcegenerator-tunit-tests/SKILL.md index bc711c59..2a3a0612 100644 --- a/.agents/skills/purview-sourcegenerator-tunit-tests/SKILL.md +++ b/.agents/skills/purview-sourcegenerator-tunit-tests/SKILL.md @@ -3,7 +3,8 @@ name: purview-sourcegenerator-tunit-tests description: >- Use when writing TUnit tests for source generators, diagnostic analyzers, code fixes or refactorings built on Purview.SourceGeneratorFramework - choosing the correct TUnit base class and - method, customising SourceGeneratorTestOptions, using the TUnit assertion extensions, and + method, customising SourceGeneratorTestOptions, using the TUnit assertion extensions (including + attribute-argument assertions for testing attribute-data models), and modernising existing tests. metadata: product: Purview.SourceGeneratorFramework.Testing.TUnit @@ -125,6 +126,21 @@ when awaited. ``` - **`DiagnosticAssertions`** — `HasDiagnostic(descriptor|id)`, `HasDiagnostics(count)`, `DoesNotHaveDiagnostic`, `HasNoDiagnostics`, `HasNoErrorDiagnostics` on generator/analyzer/code-fix results. +- **`AttributeDataAssertions`** — on an `AttributeData` (typically from `AttributeDataFactory`): + `HasConstructorArgument(ordinal, expected)`, `HasConstructorArgument(parameterName, expected[, form])`, + `DoesNotHaveConstructorArgument(parameterName)` (defaulted or absent), `HasNamedArgument(name, expected)`, + `DoesNotHaveNamedArgument(name)`. Each returns the matched `AttributeArgument`, and a failure lists every + argument of the application with how it was supplied. Pin the attribute side with these before asserting + on the model, so a failure says which half is wrong: + ```csharp + var attribute = AttributeDataFactory.FromUsage("[Info(\"custom {Value}\")]", options); + + await Assert.That(attribute).HasConstructorArgument("messageTemplate", "custom {Value}"); + await Assert.That(attribute).HasConstructorArgument("eventId", -1, AttributeArgumentForm.Defaulted); + await Assert.That(attribute).DoesNotHaveNamedArgument("Name"); + + await Assert.That(InfoAttributeData.FromAttributeData(attribute).Log.MessageTemplate).IsEqualTo("custom {Value}"); + ``` - **`TypeIdentityAssertions`** — `HasSymbol(TypeIdentity)` / `HasSymbol("Namespace.Type")`. - **`GeneratedCodeAssertionsExtensions`** — `GeneratesCode(expected)`, `ContainsGeneratedCode(expected)` (whitespace-flattened string comparison). diff --git a/AGENTS.md b/AGENTS.md index a3ea7400..7aa74d84 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -229,6 +229,7 @@ entry) are both in source, so a clone has them without running anything. | --- | --- | --- | | `purview-sourcegenerator-codewriter` | `purview-dev/sourcegenerator-framework` | writing or reviewing emitter code (`CodeWriter`, scopes, `Emitters/`) | | `purview-sourcegenerator-pipeline` | `purview-dev/sourcegenerator-framework` | changing the incremental pipeline or its value-equatable models | +| `purview-sourcegenerator-attribute-data-models` | `purview-dev/sourcegenerator-framework` | changing or testing a `[Generate]` attribute-data model (`Records/AttributeDataModels.cs`, `[NestedModel]`) | | `purview-sourcegenerator-diagnostics` | `purview-dev/sourcegenerator-framework` | adding or changing a `TSG` diagnostic or the analyzer | | `purview-sourcegenerator-packaging` | `purview-dev/sourcegenerator-framework` | changing how the generator is packed or merged | | `purview-sourcegenerator-testing` | `purview-dev/sourcegenerator-framework` | writing generator, analyzer or refactoring tests (`CodeQuery`) | @@ -239,8 +240,8 @@ entry) are both in source, so a clone has them without running anything. This repository's own skills in `skills/` are authored for **consumers** of its packages, not for use while working on this repository. They are never installed into this repository's `.agents/skills/`, which holds only skills installed from other repositories; `InstalledSkillTests` enforces this. Skills are installed into the universal `.agents/skills/` location **only**. Never install copies -into `.claude/` or any other per-agent folder; `/.claude/` is gitignored for that reason. Pass -`-a universal`, which writes `.agents/skills/` and nothing else: +into `.claude/` or any other per-agent folder. Pass `-a universal`, which writes `.agents/skills/` +and nothing else: ```bash # add or update every skill from a source @@ -260,9 +261,31 @@ CLI; `src/Directory.Build.props` sets `PurviewSkillsAgents=universal` so it is r way rather than defaulting to every agent (`*`). None of the restored packages ship skills, so `ListPurviewSkills` has nothing to recommend and the set above is chosen by hand. +Claude Code is the exception: it reads skills from `.claude/skills/`, not `.agents/skills/`. Give it +**links**, never copies, so there is still one installed copy and the lockfile stays its record. +`/.claude/` is gitignored, so the links are per-clone; create or refresh them after an install: + +```powershell +# Windows: junctions need no elevation +New-Item -ItemType Directory -Force .claude/skills | Out-Null +Get-ChildItem .agents/skills -Directory | ForEach-Object { + $link = Join-Path .claude/skills $_.Name + if (-not (Test-Path $link)) { New-Item -ItemType Junction -Path $link -Target $_.FullName | Out-Null } +} +``` + +```bash +# macOS/Linux +mkdir -p .claude/skills +for d in .agents/skills/*/; do n=$(basename "$d"); [ -e ".claude/skills/$n" ] || ln -s "../../.agents/skills/$n" ".claude/skills/$n"; done +``` + +To remove a link, delete the link itself (`[System.IO.Directory]::Delete($link)` or `rm`), never +`Remove-Item -Recurse`, which can follow a junction into `.agents/skills/`. + `src/tests/Repository.UnitTests/InstalledSkillTests.cs` keeps this honest: every `skills-lock.json` -entry has its `.agents/skills//SKILL.md`, every installed folder is in the lockfile, and no -`.claude/skills/` copy exists. +entry has its `.agents/skills//SKILL.md`, every installed folder is in the lockfile, and every +entry in `.claude/skills/`, when the folder exists, is a link to the same skill in `.agents/skills/`. ## Authoring skills diff --git a/CHANGELOG.md b/CHANGELOG.md index 071bc377..d2573d91 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,32 @@ The format follows [Keep a Changelog](https://keepachangelog.com/en/1.1.0/). Ver 5.0.7; earlier releases are described by their [GitHub releases](https://github.com/purview-dev/telemetry-sourcegenerator/releases) and, for breaking changes, by [Breaking Changes](docs/wiki/Breaking-Changes.md). +## [Unreleased] + +The level-specific logging attributes honour their arguments again. Generated log messages and event +ids change for every method that passed a template, event id, name or generation mode to one of them. + +### Fixed + +- **`[Trace]`, `[Debug]`, `[Info]`, `[Warning]`, `[Error]` and `[Critical]` ignored every argument + but the level** (since 5.0.0). `[Info("Order {OrderId} placed")]` emitted the default + `"Method: OrderId = {OrderId}"` template, every such method logged with `EventId` 0 rather than the + id derived from its name, and `Name` and `GenerationMode` were dropped. `[Log]` was unaffected. The + cause was Purview.SourceGeneratorFramework's nested-model parsing, fixed in 1.3.0, which this release + builds on. Output now matches 4.x. +- **V2 logging named the `EventId` with `nameof(...)`**, so a custom `Name` failed to compile (CS0103) + and a prefixed name such as `ITestLogger.Log` lost its prefix. The name is now emitted as a string, + as V1 does. +- `[Error]` now treats a whitespace-only `MessageTemplate` or `Name` as unset, like the other + level-specific attributes. + +### Changed + +- **`EventId` on `[Log]` and the level-specific attributes is `int`, not `int?`.** A nullable property + cannot be set as a named attribute argument, so `[Info(EventId = 42)]` failed with CS0655 and only + the constructor overloads worked. Omitting it, or passing a negative value, still uses the id derived + from the method name. + ## [5.0.7-prerelease.3] - 2026-10-28 The `TSG1011` message no longer offers an opt-out that never existed, and the define it named is diff --git a/Directory.Packages.props b/Directory.Packages.props index 83d80a53..a1026eb1 100644 --- a/Directory.Packages.props +++ b/Directory.Packages.props @@ -2,7 +2,7 @@ 5.9.0 1.76.0 - 1.2.2 + 1.3.0 10.0.14 10.11.0 1.20.1 diff --git a/docs/wiki/Contributing.md b/docs/wiki/Contributing.md index 90443087..b48452d8 100644 --- a/docs/wiki/Contributing.md +++ b/docs/wiki/Contributing.md @@ -71,7 +71,7 @@ The repo mirrors its GitHub CI/CD pipelines locally with the reusable `purview-b ## Source-generator skills -Before doing specialist work, load the relevant skill from `.agents/skills/` — see `AGENTS.md` ("Installed skills") for the full list, when to load each one, and how to add or update them. The installed skills and `skills-lock.json` are committed; install only into `.agents/` (`-a universal`), never into `.claude/` or another per-agent folder. +Before doing specialist work, load the relevant skill from `.agents/skills/` — see `AGENTS.md` ("Installed skills") for the full list, when to load each one, and how to add or update them. The installed skills and `skills-lock.json` are committed; install only into `.agents/` (`-a universal`), never into `.claude/` or another per-agent folder. Claude Code reads only `.claude/skills/`, so link it to `.agents/skills/` rather than copying (the commands are in `AGENTS.md`). ## Release process diff --git a/package.json b/package.json index 9ff58b9c..c1a61668 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "telemetry-sourcegenerator", - "version": "5.1.0", + "version": "5.2.0", "description": "Generates [`ActivitySource`](https://learn.microsoft.com/en-us/dotnet/api/system.diagnostics.activitysource), [`ILogger`](https://learn.microsoft.com/en-us/dotnet/api/microsoft.extensions.logging.ilogger), and [`Metrics`](https://learn.microsoft.com/en-us/dotnet/api/system.diagnostics.metrics) based on interface methods.", "license": "MIT", "readme": "README.md", diff --git a/skills-lock.json b/skills-lock.json index 3bd69121..fc537d0f 100644 --- a/skills-lock.json +++ b/skills-lock.json @@ -73,6 +73,12 @@ "skillPath": "skills/purview-persistence/SKILL.md", "computedHash": "0523ecc210bf3571e2fccb859128a499fd80694e9d4089b377a8d39d73c028f4" }, + "purview-sourcegenerator-attribute-data-models": { + "source": "purview-dev/sourcegenerator-framework", + "sourceType": "github", + "skillPath": "skills/purview-sourcegenerator-attribute-data-models/SKILL.md", + "computedHash": "84c309a0094a44a8ca76f0078c1fccee1f3c99b96b13a90b5b01260274c0cda6" + }, "purview-sourcegenerator-codewriter": { "source": "purview-dev/sourcegenerator-framework", "sourceType": "github", @@ -83,13 +89,13 @@ "source": "purview-dev/sourcegenerator-framework", "sourceType": "github", "skillPath": "skills/purview-sourcegenerator-diagnostics/SKILL.md", - "computedHash": "c3c7920701e0e688f34ddece9e448472e010e69767e8c08914638128d34a8f17" + "computedHash": "c3d035e053ebcde99de95399c46e9d822d01c9837154ecff5792f5f0dfafa0ca" }, "purview-sourcegenerator-packaging": { "source": "purview-dev/sourcegenerator-framework", "sourceType": "github", "skillPath": "skills/purview-sourcegenerator-packaging/SKILL.md", - "computedHash": "e0e6372e92aefc8df026a73010d9e7436b0a8c2378a96fdebd8b4de9e025d4a0" + "computedHash": "ee2440f1e0aa0cea731eb3eae2ea3c4e9ea77502867abc21e9f0a3c61f20c640" }, "purview-sourcegenerator-pipeline": { "source": "purview-dev/sourcegenerator-framework", @@ -101,13 +107,13 @@ "source": "purview-dev/sourcegenerator-framework", "sourceType": "github", "skillPath": "skills/purview-sourcegenerator-testing/SKILL.md", - "computedHash": "d88bf2353073013ff9ddf9d3fe1a2e9415eb60d319bbd053baa105f50fa3f889" + "computedHash": "8ea0b5a51f045721c730a3bc57e3a14e79195b9b99738430fbb056263785b75c" }, "purview-sourcegenerator-tunit-tests": { "source": "purview-dev/sourcegenerator-framework", "sourceType": "github", "skillPath": "skills/purview-sourcegenerator-tunit-tests/SKILL.md", - "computedHash": "14809d3727643e9a779d590b6627b3283af7b36c835e902cb80162552cb45b4f" + "computedHash": "5b801f9bf792f2ae0baeb2a3835680b9f95dbaed2e57ced43815449e64e19132" }, "purview-telemetry": { "source": "purview-dev/engineering-playbook", diff --git a/skills/purview-telemetry-diagnostics/SKILL.md b/skills/purview-telemetry-diagnostics/SKILL.md index b16d8735..57fd3429 100644 --- a/skills/purview-telemetry-diagnostics/SKILL.md +++ b/skills/purview-telemetry-diagnostics/SKILL.md @@ -178,6 +178,15 @@ generated only where the generator runs. The method name becomes a member on the generated implementation class, so two methods cannot share one. Rename. +## No diagnostic, but the log message is wrong + +The log reads `Method: Param = {Param}` despite `[Info("…")]`, and the event id is 0. Versions 5.0.0 +to 5.1.0 drop every argument of `[Trace]`, `[Debug]`, `[Info]`, `[Warning]`, `[Error]` and +`[Critical]` except the level: template, event id, `Name` and `GenerationMode`. Upgrade, or on those +versions use `[Log(LogLevel.Information, "…")]`, which is unaffected. Do not fall back to +`[LoggerMessage]` or `ILogger.Log*`. To confirm, set `EmitCompilerGeneratedFiles` and read the template +in the generated `*.Logging.g.cs` under `obj/*/generated/`. + ## Related skills `purview-telemetry-quickstart` to get an interface wired up, `purview-telemetry-design` for diff --git a/skills/purview-telemetry-migration/SKILL.md b/skills/purview-telemetry-migration/SKILL.md index b9a01fad..c88a7d1b 100644 --- a/skills/purview-telemetry-migration/SKILL.md +++ b/skills/purview-telemetry-migration/SKILL.md @@ -33,11 +33,14 @@ using Purview.Telemetry; [Logger] public interface IOrderServiceLogs { - [Info] + [Info("Order {OrderId} placed for {CustomerName}")] void OrderPlaced(int orderId, string customerName); } ``` +A literal message template and a literal event id carry over. Without a template the generator writes +`OrderPlaced: OrderId = {OrderId}, CustomerName = {CustomerName}`. + The original class is rewritten to use the interface: ```csharp @@ -78,4 +81,21 @@ If the refactoring does not cover a call pattern, convert it manually: | `Histogram.Record(...)` | `[Histogram]` | | `UpDownCounter.Add(...)` | `[UpDownCounter]` | +## Keeping the message, event id and name + +Each level attribute takes the original template, event id and log name, positionally, by parameter +name, or as named arguments: + +```csharp +[Info("Order {OrderId} placed")] // logger.LogInformation("Order {OrderId} placed", id) +[Warning(7, "Retrying {Attempt}")] // logger.LogWarning(new EventId(7), "Retrying {Attempt}", n) +[Error(MessageTemplate = "Failed {OrderId}", EventId = 9, Name = "OrderFailed")] +``` + +**Check the generated text after migrating.** Versions 5.0.0 to 5.1.0 ignore every argument on +`[Trace]`, `[Debug]`, `[Info]`, `[Warning]`, `[Error]` and `[Critical]` except the level, with no +diagnostic: the template falls back to the default form and the event id is 0. On those versions use +`[Log]`, which is unaffected: `[Log(LogLevel.Information, "Order {OrderId} placed")]`. Named +`EventId = n` needs the release after 5.1.0; earlier ones accept only the constructor form. + See [references/REFERENCE.md](references/REFERENCE.md) for detailed before/after examples and common conversions. diff --git a/src/src/SourceGenerator/Emitters/GeneratedTypesEmitter.cs b/src/src/SourceGenerator/Emitters/GeneratedTypesEmitter.cs index 58aa3399..b6363ea3 100644 --- a/src/src/SourceGenerator/Emitters/GeneratedTypesEmitter.cs +++ b/src/src/SourceGenerator/Emitters/GeneratedTypesEmitter.cs @@ -1193,8 +1193,8 @@ static void WriteLogAttribute(CodeWriter writer, TypeIdentity type) WritePublicProperty( body, "EventId", - TypeLibrary.System.Int32.MakeNullable(writer), - "Gets or sets the event identifier of the log entry." + TypeLibrary.System.Int32, + "Gets or sets the event identifier of the log entry. A negative value, or none, uses an identifier derived from the method name." ); WriteNullableStringProperty(body, "Name", "Gets or sets the name of the log entry."); WritePublicProperty( @@ -1281,8 +1281,8 @@ static void WriteSpecificLogAttribute(CodeWriter writer, TypeIdentity type, stri WritePublicProperty( body, "EventId", - TypeLibrary.System.Int32.MakeNullable(writer), - "Gets or sets the event identifier of the log entry." + TypeLibrary.System.Int32, + "Gets or sets the event identifier of the log entry. A negative value, or none, uses an identifier derived from the method name." ); WriteNullableStringProperty(body, "Name", "Gets or sets the name of the log entry."); WritePublicProperty( diff --git a/src/src/SourceGenerator/Emitters/LoggerGenTargetClassEmitter.Methods.cs b/src/src/SourceGenerator/Emitters/LoggerGenTargetClassEmitter.Methods.cs index cb6198dc..c6cb4a1a 100644 --- a/src/src/SourceGenerator/Emitters/LoggerGenTargetClassEmitter.Methods.cs +++ b/src/src/SourceGenerator/Emitters/LoggerGenTargetClassEmitter.Methods.cs @@ -306,9 +306,10 @@ writer.IsNullableContextEnabled is null or true : "new " + TypeLibrary.Microsoft.Extensions.Logging.EventId + "(" ) .Write(eventId.ToString(CultureInfo.InvariantCulture)) - .Write(", nameof(") - .Write(methodTarget.LogName) - .Line(")),") + .Write(", ") + // A literal, not nameof: the log name can be a custom Name or carry a prefix. + .Write(methodTarget.LogName.Wrap()) + .Line("),") // State .Write(stateVarName.WithComma(andSpace: false)) // Exception @@ -381,9 +382,9 @@ writer.IsNullableContextEnabled is null or true : "new " + TypeLibrary.Microsoft.Extensions.Logging.EventId + "(" ) .Write(eventId.ToString(CultureInfo.InvariantCulture)) - .Write(", nameof(") - .Write(methodTarget.LogName) - .Line(")),") + .Write(", ") + .Write(methodTarget.LogName.Wrap()) + .Line("),") .Write("new ") .Write(structName) .Write('('); diff --git a/src/src/SourceGenerator/Records/AttributeDataModels.cs b/src/src/SourceGenerator/Records/AttributeDataModels.cs index d8242081..2548f947 100644 --- a/src/src/SourceGenerator/Records/AttributeDataModels.cs +++ b/src/src/SourceGenerator/Records/AttributeDataModels.cs @@ -164,6 +164,8 @@ public LogAttributeData ToLogAttribute() => Log with { Level = TypeLibrary.Purview.Telemetry.LogLevelMap[TypeLibrary.Purview.Telemetry.ErrorAttribute].Value, + MessageTemplate = SharedHelpers.NullIfWhitespace(Log.MessageTemplate), + Name = SharedHelpers.NullIfWhitespace(Log.Name), }; } diff --git a/src/tests/Repository.UnitTests/InstalledSkillTests.cs b/src/tests/Repository.UnitTests/InstalledSkillTests.cs index 8c67fad6..aad03656 100644 --- a/src/tests/Repository.UnitTests/InstalledSkillTests.cs +++ b/src/tests/Repository.UnitTests/InstalledSkillTests.cs @@ -6,6 +6,8 @@ namespace Purview.Telemetry.Repository; /// Guards the skills installed from other repositories. They are installed by the skills CLI into the /// universal .agents/skills/ folder only, and that folder and the CLI's skills-lock.json /// are committed, so the two must agree and no per-agent copy or instruction file may appear beside them. +/// Claude Code only reads .claude/skills/, so that gitignored folder may hold links into +/// .agents/skills/, never copies. /// /// /// SkillTests validates this repository's own skills in skills/; these are someone else's, @@ -61,22 +63,29 @@ await Assert } [Test] - public async Task ClaudeFolder_GivenTheInstalledSkills_HoldsNoCopy(CancellationToken cancellationToken) + public async Task ClaudeFolder_GivenTheInstalledSkills_HoldsOnlyLinksToThem(CancellationToken cancellationToken) { - // Arrange + // Arrange: Claude Code reads .claude/skills/, not .agents/skills/, so a local (gitignored) folder of + // links is allowed. A real copy is not: it drifts from skills-lock.json the moment the CLI updates. cancellationToken.ThrowIfCancellationRequested(); var claudeSkills = Path.Combine(RepositoryRoot.Value, ".claude", "skills"); + if (!Directory.Exists(claudeSkills)) + return; // Act - var exists = Directory.Exists(claudeSkills); + var notLinks = Directory + .GetFileSystemEntries(claudeSkills) + .Where(static path => !IsLinkToInstalledSkill(path)) + .Select(static path => Path.GetFileName(path)) + .ToArray(); // Assert await Assert - .That(exists) - .IsFalse() + .That(notLinks) + .IsEmpty() .Because( - "skills are installed into .agents/skills/ only. A .claude/skills/ folder means the CLI ran without " - + "`-a universal`; delete it and reinstall with `-a universal`" + "every entry in .claude/skills/ must be a link to the same skill in .agents/skills/, not a copy. " + + $"Replace these with links (see AGENTS.md): {string.Join(", ", notLinks)}" ); } @@ -131,6 +140,18 @@ await Assert static string AgentsSkillsFolder() => Path.Combine(RepositoryRoot.Value, ".agents", "skills"); + static bool IsLinkToInstalledSkill(string path) + { + var linkTarget = new DirectoryInfo(path).LinkTarget; + if (linkTarget is null) + return false; + + var resolved = Path.GetFullPath(linkTarget, Path.GetDirectoryName(path)!); + var expected = Path.Combine(AgentsSkillsFolder(), Path.GetFileName(path)); + + return string.Equals(Path.TrimEndingDirectorySeparator(resolved), expected, StringComparison.OrdinalIgnoreCase); + } + static async Task ReadLockfileSkillsAsync(CancellationToken cancellationToken) { await using var stream = File.OpenRead(Path.Combine(RepositoryRoot.Value, "skills-lock.json")); diff --git a/src/tests/SourceGenerator.IntegrationTests/Generators/TelemetrySourceGeneratorLoggingTests.SpecificLogAttributeArguments.cs b/src/tests/SourceGenerator.IntegrationTests/Generators/TelemetrySourceGeneratorLoggingTests.SpecificLogAttributeArguments.cs new file mode 100644 index 00000000..a8ffdcd3 --- /dev/null +++ b/src/tests/SourceGenerator.IntegrationTests/Generators/TelemetrySourceGeneratorLoggingTests.SpecificLogAttributeArguments.cs @@ -0,0 +1,162 @@ +using System.Globalization; +using Purview.Telemetry.SourceGenerator.Helpers; + +namespace Purview.Telemetry.SourceGenerator.Logging; + +/// +/// The level-specific attributes ([Trace], [Debug], [Info], [Warning], [Error], [Critical]) share the +/// LogAttributeData model with [Log]. LogAttributeDataModelTests proves the models read every +/// argument; these prove the V1 and V2 emitters write what was read, not just that the method exists. +/// +partial class TelemetrySourceGeneratorLoggingTests +{ + [Test] + [MethodDataSource(nameof(SpecificLogAttributeTypesAndGenerationModes))] + public async Task Generate_GivenSpecificLogAttributeWithPositionalTemplate_EmitsTemplate( + string attribute, + string generationMode, + CancellationToken cancellationToken + ) + { + // Arrange + var basicLogger = $$""" + +namespace Testing; + +[Logger(GenerationMode = LoggerGenerationMode.{{generationMode}})] +public interface ITestLogger +{ + [{{attribute}}("Custom {Value} message")] + void Log(int value); +} + +"""; + + // Act + using var generationResult = await GenerateAsync(basicLogger, cancellationToken: cancellationToken); + + // Assert + var loggerClass = GetLoggerClassSource(generationResult); + await Assert.That(loggerClass).Contains("\"Custom {Value} message\""); + await Assert.That(loggerClass).DoesNotContain("Log: Value = {Value}"); + await Assert + .That(loggerClass) + .Contains(SharedHelpers.GetNonRandomizedHashCode("Log").ToString(CultureInfo.InvariantCulture)) + .Because("an omitted event id falls back to the hashed method name, not 0"); + } + + [Test] + [MethodDataSource(nameof(SpecificLogAttributeTypesAndGenerationModes))] + public async Task Generate_GivenSpecificLogAttributeWithEventIdConstructor_EmitsEventIdTemplateAndName( + string attribute, + string generationMode, + CancellationToken cancellationToken + ) + { + // Arrange + var basicLogger = $$""" + +namespace Testing; + +[Logger(GenerationMode = LoggerGenerationMode.{{generationMode}})] +public interface ITestLogger +{ + [{{attribute}}(77, "EventId {Value} message", "RenamedEntry")] + void Log(int value); +} + +"""; + + // Act + using var generationResult = await GenerateAsync(basicLogger, cancellationToken: cancellationToken); + + // Assert + var loggerClass = GetLoggerClassSource(generationResult); + await Assert.That(loggerClass).Contains("\"EventId {Value} message\""); + await Assert.That(loggerClass).Contains("(77, "); + await Assert.That(loggerClass).Contains("RenamedEntry"); + } + + [Test] + [MethodDataSource(nameof(SpecificLogAttributeTypes))] + public async Task Generate_GivenSpecificLogAttributeForcingV1OnV2Interface_EmitsLoggerMessageDefine( + string attribute, + CancellationToken cancellationToken + ) + { + // Arrange + var basicLogger = $$""" + +namespace Testing; + +[Logger(GenerationMode = LoggerGenerationMode.V2)] +public interface ITestLogger +{ + [{{attribute}}(GenerationMode = LoggerGenerationMode.V1)] + void Log(int value); +} + +"""; + + // Act + using var generationResult = await GenerateAsync(basicLogger, cancellationToken: cancellationToken); + + // Assert + var loggerClass = GetLoggerClassSource(generationResult); + await Assert + .That(loggerClass) + .Contains("LoggerMessage.Define") + .Because("GenerationMode on the attribute overrides the interface's V2 mode"); + } + + [Test] + [MethodDataSource(nameof(SpecificLogAttributeTypes))] + public async Task Generate_GivenV2SpecificLogAttributeWithInterfacePrefix_EmitsPrefixedEventName( + string attribute, + CancellationToken cancellationToken + ) + { + // Arrange + var basicLogger = $$""" + +namespace Testing; + +[Logger(GenerationMode = LoggerGenerationMode.V2, PrefixType = LogPrefixType.Interface)] +public interface ITestLogger +{ + [{{attribute}}] + void Log(int value); +} + +"""; + + // Act + using var generationResult = await GenerateAsync(basicLogger, cancellationToken: cancellationToken); + + // Assert + var loggerClass = GetLoggerClassSource(generationResult); + await Assert + .That(loggerClass) + .Contains("\"ITestLogger.Log\"") + .Because("V2 must name the event like V1 does, not collapse the prefix through nameof"); + } + + static string GetLoggerClassSource(DriverRunResult generationResult) => + generationResult.Generated().GetClass("TestLoggerCore", "Testing").Node.ToFullString(); + + public static IEnumerable<(string, string)> SpecificLogAttributeTypesAndGenerationModes + { + get + { + List<(string, string)> data = []; + + foreach (var attribute in SpecificLogAttributeTypes) + { + data.Add((attribute, "V1")); + data.Add((attribute, "V2")); + } + + return data; + } + } +} diff --git a/src/tests/SourceGenerator.IntegrationTests/Records/LogAttributeDataModelTests.cs b/src/tests/SourceGenerator.IntegrationTests/Records/LogAttributeDataModelTests.cs new file mode 100644 index 00000000..5e1e1976 --- /dev/null +++ b/src/tests/SourceGenerator.IntegrationTests/Records/LogAttributeDataModelTests.cs @@ -0,0 +1,229 @@ +using Microsoft.CodeAnalysis.CSharp; +using Purview.Telemetry.SourceGenerator.Infra; +using Purview.Telemetry.SourceGenerator.Logging; + +namespace Purview.Telemetry.SourceGenerator.Records; + +/// +/// Reads real applications of the generated logging attributes through the attribute-data models. The +/// level-specific models nest , and 5.0.0 to 5.1.0 read every one of them as +/// , so these pin each argument the models must carry. +/// +/// +/// The attribute declarations are the generator's own post-initialization output, not a hand-written mirror, +/// so a declaration that cannot be applied (such as a nullable EventId) fails here. +/// +public class LogAttributeDataModelTests +{ + static readonly Lazy Options = new(CreateOptions); + + [Test] + [MethodDataSource(nameof(SpecificLogAttributes))] + public async Task FromAttributeData_GivenPositionalTemplate_ReadsTemplate(string attribute, int level) + { + // Arrange + var application = Apply($"[{attribute}(\"custom {{Value}}\")]"); + + // Act + var model = ReadSpecific(attribute, application); + + // Assert + await Assert.That(application).HasConstructorArgument("messageTemplate", "custom {Value}"); + await Assert.That(model.MessageTemplate).IsEqualTo("custom {Value}"); + await Assert.That(model.Level).IsEqualTo(level); + await Assert.That(model.EventIdOrNull).IsNull(); + await Assert.That(model.Name).IsNull(); + } + + [Test] + [MethodDataSource(nameof(SpecificLogAttributes))] + public async Task FromAttributeData_GivenConstructorArgumentsByName_ReadsEach(string attribute, int level) + { + // Arrange + var application = Apply($"[{attribute}(eventId: 7, messageTemplate: \"by name {{Value}}\", name: \"Named\")]"); + + // Act + var model = ReadSpecific(attribute, application); + + // Assert + await Assert.That(application).HasConstructorArgument("eventId", 7, AttributeArgumentForm.NamedParameter); + await Assert.That(model.EventIdOrNull).IsEqualTo(7); + await Assert.That(model.MessageTemplate).IsEqualTo("by name {Value}"); + await Assert.That(model.Name).IsEqualTo("Named"); + await Assert.That(model.Level).IsEqualTo(level); + } + + [Test] + [MethodDataSource(nameof(SpecificLogAttributes))] + public async Task FromAttributeData_GivenNamedArguments_ReadsEach(string attribute, int level) + { + // Arrange + var application = Apply( + $"[{attribute}(MessageTemplate = \"named {{Value}}\", EventId = 42, Name = \"Renamed\", GenerationMode = LoggerGenerationMode.V1)]" + ); + + // Act + var model = ReadSpecific(attribute, application); + + // Assert + await Assert.That(application).HasNamedArgument("EventId", 42); + await Assert.That(model.MessageTemplate).IsEqualTo("named {Value}"); + await Assert.That(model.EventIdOrNull).IsEqualTo(42); + await Assert.That(model.Name).IsEqualTo("Renamed"); + await Assert.That(model.GenerationMode).IsEqualTo(1); + await Assert.That(model.Level).IsEqualTo(level); + } + + [Test] + [MethodDataSource(nameof(SpecificLogAttributes))] + public async Task FromAttributeData_GivenNoArguments_TakesDefaults(string attribute, int level) + { + // Arrange + var application = Apply($"[{attribute}]"); + + // Act + var specific = ReadSpecific(attribute, application); + + // Assert + await Assert.That(application).HasConstructorArgument("messageTemplate", null, AttributeArgumentForm.Defaulted); + await Assert.That(specific.MessageTemplate).IsNull(); + await Assert.That(specific.EventId).IsEqualTo(-1).Because("an omitted event id falls back to the hashed name"); + await Assert.That(specific.Name).IsNull(); + await Assert.That(specific.GenerationMode).IsEqualTo(0); + await Assert.That(specific.Level).IsEqualTo(level); + } + + [Test] + [MethodDataSource(nameof(SpecificLogAttributes))] + public async Task FromAttributeData_GivenWhitespaceTemplateAndName_TreatsThemAsUnset(string attribute, int level) + { + // Arrange + var application = Apply($"[{attribute}(\" \", \" \")]"); + + // Act + var model = ReadSpecific(attribute, application); + + // Assert + await Assert.That(model.MessageTemplate).IsNull(); + await Assert.That(model.Name).IsNull(); + await Assert.That(model.Level).IsEqualTo(level); + } + + [Test] + [MethodDataSource( + nameof(TelemetrySourceGeneratorLoggingTests.SpecificLogAttributeTypes) + )] + public async Task FromAttributeData_GivenSpecificAttribute_NestedLevelTakesItsDefault(string attribute) + { + // Arrange + var application = Apply($"[{attribute}(\"template\")]"); + + // Act + var nested = ReadNested(attribute, application); + + // Assert + await Assert.That(application).DoesNotHaveConstructorArgument("level"); + await Assert + .That(nested.Level) + .IsEqualTo(99) + .Because($"[{attribute}] has no level argument; ToLogAttribute supplies the level instead"); + await Assert.That(nested.MessageTemplate).IsEqualTo("template"); + } + + [Test] + [MethodDataSource( + nameof(TelemetrySourceGeneratorLoggingTests.SpecificLogAttributeTypes) + )] + public async Task FromAttributeData_GivenSpecificAttribute_NestedTargetRejectsIt(string attribute) + { + // Arrange + var application = Apply($"[{attribute}(\"template\")]"); + + // Act + var log = LogAttributeData.FromAttributeData(application); + + // Assert + await Assert + .That(log.Exists) + .IsFalse() + .Because($"[{attribute}] is not [Log]; only the nested read skips the check"); + } + + [Test] + public async Task FromAttributeData_GivenLogWithNamedEventId_ReadsIt() + { + // Arrange + var application = Apply("[Log(MessageTemplate = \"log {Value}\", EventId = 9)]"); + + // Act + var model = LogAttributeData.FromAttributeData(application); + + // Assert + await Assert.That(model.Exists).IsTrue(); + await Assert.That(model.MessageTemplate).IsEqualTo("log {Value}"); + await Assert.That(model.EventIdOrNull).IsEqualTo(9); + } + + static AttributeData Apply(string usage) => AttributeDataFactory.FromUsage(usage, Options.Value); + + static LogAttributeData ReadSpecific(string attribute, AttributeData application) => + attribute switch + { + "Trace" => TraceAttributeData.FromAttributeData(application).ToLogAttribute(), + "Debug" => DebugAttributeData.FromAttributeData(application).ToLogAttribute(), + "Info" => InfoAttributeData.FromAttributeData(application).ToLogAttribute(), + "Warning" => WarningAttributeData.FromAttributeData(application).ToLogAttribute(), + "Error" => ErrorAttributeData.FromAttributeData(application).ToLogAttribute(), + "Critical" => CriticalAttributeData.FromAttributeData(application).ToLogAttribute(), + _ => throw new ArgumentOutOfRangeException(nameof(attribute), attribute, null), + }; + + // The nested model as read, before ToLogAttribute applies the level and whitespace rules. + static LogAttributeData ReadNested(string attribute, AttributeData application) => + attribute switch + { + "Trace" => TraceAttributeData.FromAttributeData(application).Log, + "Debug" => DebugAttributeData.FromAttributeData(application).Log, + "Info" => InfoAttributeData.FromAttributeData(application).Log, + "Warning" => WarningAttributeData.FromAttributeData(application).Log, + "Error" => ErrorAttributeData.FromAttributeData(application).Log, + "Critical" => CriticalAttributeData.FromAttributeData(application).Log, + _ => throw new ArgumentOutOfRangeException(nameof(attribute), attribute, null), + }; + + static TelemetrySourceGeneratorTestOptions CreateOptions() + { + var compilation = CSharpCompilation.Create( + nameof(LogAttributeDataModelTests), + [], + [MetadataReference.CreateFromFile(typeof(object).Assembly.Location)], + new CSharpCompilationOptions(OutputKind.DynamicallyLinkedLibrary) + ); + + var attributeSources = CSharpGeneratorDriver + .Create(new TelemetrySourceGenerator()) + .RunGenerators(compilation) + .GetRunResult() + .Results.SelectMany(static result => result.GeneratedSources) + .Select(static source => source.SourceText.ToString()); + + return new() { AdditionalSources = [.. attributeSources] }; + } + + public static IEnumerable<(string, int)> SpecificLogAttributes + { + get + { + List<(string, int)> data = []; + + data.Add(("Trace", 0)); + data.Add(("Debug", 1)); + data.Add(("Info", 2)); + data.Add(("Warning", 3)); + data.Add(("Error", 4)); + data.Add(("Critical", 5)); + + return data; + } + } +}