Files
romanett 24399f1808 Fix source generator MODELGEN003 diagnostic dropping message placeholders (#4310)
## Problem

`LoggerAdapter.Log<TState>` in
`Tools/Opc.Ua.SourceGeneration/SourceGeneratorTelemetry.cs` reported an
**already fully-formatted** logger `message` against a looked-up
`DiagnosticDescriptor` whose `messageFormat` may declare more positional
placeholders than the single formatted string carries.

For `EventId == 3` the descriptor is `SourceGenerator.Exception`
(`MODELGEN003`) with:

```
messageFormat = "Exception during model generation '{0}': {1}"
```

That format has **two** placeholders but only **one** argument
(`message`) was supplied. Roslyn's `Diagnostic.GetMessage()` runs
`string.Format(format, args)`, which throws `FormatException` because
`{1}` has no argument; Roslyn's internal catch then returns the **raw,
unsubstituted template**. The user therefore saw the literal text
`Exception during model generation '{0}': {1}` with the placeholders
unreplaced, masking the real error.

## Fix

Route the pre-formatted message through a new
`CreateFormattedDiagnostic` helper that reports it via a **passthrough
descriptor whose `messageFormat` is exactly `"{0}"`**, while preserving
the original descriptor's `Id`, `Title`, `Category`, `DefaultSeverity`,
`IsEnabledByDefault`, `HelpLinkUri` and custom tags:

```csharp
public static Diagnostic CreateFormattedDiagnostic(
    DiagnosticDescriptor descriptor,
    string message)
{
    var passthrough = new DiagnosticDescriptor(
        descriptor.Id,
        descriptor.Title,
        "{0}",
        descriptor.Category,
        descriptor.DefaultSeverity,
        descriptor.IsEnabledByDefault,
        helpLinkUri: descriptor.HelpLinkUri,
        customTags: descriptor.CustomTags.ToArray());
    return Diagnostic.Create(passthrough, Location.None, message);
}
```

`LoggerAdapter.Log` now calls this helper instead of
`Diagnostic.Create(descriptor, Location.None, message)`. This renders
the message verbatim under the correct diagnostic id regardless of the
descriptor's placeholder arity, and is safe even when the message itself
contains `{`/`}` characters (the format string is only `"{0}"`).

> Note: the public `descriptor.CustomTags` (`IEnumerable<string>`) +
`.ToArray()` is used because Roslyn's `ImmutableCustomTags` property is
`internal`.

### Why the helper lives in `SourceGeneratorHelpers.cs` (not
`SourceGenerator.cs`)

`SourceGeneratorTelemetry.cs` and `SourceGeneratorHelpers.cs` are shared
(glob-linked) into **both** the model generator
(`Opc.Ua.SourceGeneration`) and the stack generator
(`Opc.Ua.SourceGeneration.Stack`). The stack generator has its **own**
`SourceGenerator` partial class and does **not** compile the model
generator's `SourceGenerator.cs`. Placing the helper in
`SourceGenerator.cs` broke the stack build with `CS0117:
'SourceGenerator' does not contain a definition for
'CreateFormattedDiagnostic'`. `SourceGeneratorHelpers.cs` is the shared
partial (where `Guard` also lives), so the helper is available to both
generators.

### Guard path untouched

The other emission path, `SourceGenerator.Guard(...)` in
`SourceGeneratorHelpers.cs`, uses the same `Exception` descriptor but
correctly passes **two** args (`ex.Message`, `ex.ToString()`), so that
path was already fine and is **not** changed.

## Tests

New
`tests/Opc.Ua.SourceGeneration.Tests/SourceGeneratorTelemetryTests.cs`
(4 tests):

1. **`DirectReportAgainstMultiPlaceholderDescriptorLosesMessage`** —
documents the pre-fix behaviour: the raw `Diagnostic.Create(Exception,
..., "boom")` render drops `"boom"` and leaks `{1}`.
2. **`CreateFormattedDiagnosticRendersMessageVerbatim`** — the fix
renders `"boom"` exactly, with no `{0}`/`{1}`.
3. **`CreateFormattedDiagnosticPreservesDescriptorMetadata`** — id
(`MODELGEN003`), severity, category, help link, IsEnabledByDefault and
custom tags are preserved.
4. **`CreateFormattedDiagnosticIsSafeForBracesInMessage`** — a message
containing `{`/`}` is emitted verbatim.

## Validation

- `tools/Opc.Ua.SourceGeneration` (and the stack variant) build with **0
warnings / 0 errors**.
- Full `Opc.Ua.SourceGeneration.Tests` suite on net10.0: **132/132
passing**, including the 4 new tests.

## Files changed

- `tools/Opc.Ua.SourceGeneration/SourceGeneratorHelpers.cs` — added
`using System.Linq;` + `CreateFormattedDiagnostic` helper.
- `tools/Opc.Ua.SourceGeneration/SourceGeneratorTelemetry.cs` —
`LoggerAdapter.Log` calls the helper.
- `tests/Opc.Ua.SourceGeneration.Tests/SourceGeneratorTelemetryTests.cs`
— new regression tests.

---------

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
2026-08-24 15:19:26 +00:00

125 lines
5.3 KiB
C#

/* ========================================================================
* Copyright (c) 2005-2026 The OPC Foundation, Inc. All rights reserved.
*
* OPC Foundation MIT License 1.00
*
* Permission is hereby granted, free of charge, to any person
* obtaining a copy of this software and associated documentation
* files (the "Software"), to deal in the Software without
* restriction, including without limitation the rights to use,
* copy, modify, merge, publish, distribute, sublicense, and/or sell
* copies of the Software, and to permit persons to whom the
* Software is furnished to do so, subject to the following
* conditions:
*
* The above copyright notice and this permission notice shall be
* included in all copies or substantial portions of the Software.
* THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,
* EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES
* OF MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND
* NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT
* HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY,
* WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
* FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR
* OTHER DEALINGS IN THE SOFTWARE.
*
* The complete license agreement can be found here:
* http://opcfoundation.org/License/MIT/1.00/
* ======================================================================*/
using System.Globalization;
using System.Linq;
using Microsoft.CodeAnalysis;
using NUnit.Framework;
namespace Opc.Ua.SourceGeneration
{
/// <summary>
/// Tests for the logger-to-diagnostic bridge in
/// <see cref="SourceGeneratorTelemetry"/> / <see cref="SourceGenerator"/>.
/// </summary>
[TestFixture]
[Category("SourceGeneration")]
[SetCulture("en-us")]
[SetUICulture("en-us")]
public class SourceGeneratorTelemetryTests
{
/// <summary>
/// Documents the underlying bug: reporting an already-formatted message
/// directly against a descriptor whose format declares more placeholders
/// than arguments supplied yields the raw, unsubstituted template.
/// </summary>
[Test]
public void DirectReportAgainstMultiPlaceholderDescriptorLosesMessage()
{
Diagnostic diagnostic = Diagnostic.Create(
SourceGenerator.Exception, Location.None, "boom");
string rendered = diagnostic.GetMessage(CultureInfo.InvariantCulture);
// The single argument cannot satisfy the '{0}': '{1}' format, so the
// real message is dropped and the placeholders leak through verbatim.
Assert.That(rendered, Does.Not.Contain("boom"));
Assert.That(rendered, Does.Contain("{1}"));
}
/// <summary>
/// Verifies the fix: an already-formatted message is rendered verbatim
/// under the correct diagnostic identity regardless of the looked-up
/// descriptor's placeholder arity.
/// </summary>
[Test]
public void CreateFormattedDiagnosticRendersMessageVerbatim()
{
Diagnostic diagnostic = SourceGenerator.CreateFormattedDiagnostic(
SourceGenerator.Exception, "boom");
string rendered = diagnostic.GetMessage(CultureInfo.InvariantCulture);
Assert.That(rendered, Is.EqualTo("boom"));
Assert.That(rendered, Does.Not.Contain("{0}"));
Assert.That(rendered, Does.Not.Contain("{1}"));
}
/// <summary>
/// The passthrough descriptor preserves the looked-up descriptor's
/// identity and metadata (id, severity, category, help link, tags).
/// </summary>
[Test]
public void CreateFormattedDiagnosticPreservesDescriptorMetadata()
{
DiagnosticDescriptor source = SourceGenerator.Exception;
Diagnostic diagnostic = SourceGenerator.CreateFormattedDiagnostic(
source, "boom");
DiagnosticDescriptor result = diagnostic.Descriptor;
Assert.That(result.Id, Is.EqualTo(source.Id));
Assert.That(result.Id, Is.EqualTo("MODELGEN003"));
Assert.That(diagnostic.Severity, Is.EqualTo(source.DefaultSeverity));
Assert.That(result.Category, Is.EqualTo(source.Category));
Assert.That(result.Description, Is.EqualTo(source.Description));
Assert.That(result.HelpLinkUri, Is.EqualTo(source.HelpLinkUri));
Assert.That(result.IsEnabledByDefault, Is.EqualTo(source.IsEnabledByDefault));
Assert.That(result.CustomTags, Is.EquivalentTo(source.CustomTags.ToArray()));
}
/// <summary>
/// The formatted message is emitted safely even when it contains brace
/// characters that would otherwise be interpreted as format placeholders.
/// </summary>
[Test]
public void CreateFormattedDiagnosticIsSafeForBracesInMessage()
{
const string message = "value {0} was not {closed";
Diagnostic diagnostic = SourceGenerator.CreateFormattedDiagnostic(
SourceGenerator.GenericError, message);
string rendered = diagnostic.GetMessage(CultureInfo.InvariantCulture);
Assert.That(rendered, Is.EqualTo(message));
}
}
}