Log warnings for skipped STR resource keys instead of failing the build by OvesN · Pull Request #13291 · dotnet/msbuild

OvesN · 2026-02-25T07:21:25Z

Context

The GenerateResource task with MSBuild:Compile failed the entire build when a resource key (e.g. containing = or `) could not be converted to a valid C# identifier. In contrast, ResXFileCodeGenerator silently skipped such entries. This aligns the behavior by skipping invalid keys with warnings instead of failing.

Changes Made

Added warning messages MSB3827–MSB3830 for resource keys that are skipped during STR property generation.
STR files are now still generated even when some keys cannot produce valid identifiers.

Testing

Added unit tests verifying that invalid resource keys emit the expected warnings and valid keys still generate STR properties.

Notes

ResXFileCodeGenerator in VS silently skips invalid keys with no diagnostic. A separate VS issue has been created to add warnings there as well.

…stead of failing the build, ensuring STR files are still generated when possible

OvesN · 2026-02-25T07:23:50Z

@dotnet-policy-service agree company="Microsoft"

Copilot

Pull request overview

This PR changes the behavior of the GenerateResource task when processing resource keys that cannot be converted to valid C# identifiers in strongly typed resource (STR) files. Instead of failing the entire build with errors, the task now logs warnings and skips problematic keys while still generating the STR file with valid properties. This aligns MSBuild's MSBuild:Compile generator behavior with Visual Studio's ResXFileCodeGenerator, which silently skips invalid keys.

Changes:

Modified StronglyTypedResourceBuilder to return structured warning information (key, reason, extraArg) instead of error strings
Added four new warning codes: MSB3827 (reserved name), MSB3828 (invalid identifier), MSB3829 (name collision), MSB3830 (void type)
Updated GenerateResource.cs to log warnings instead of errors and mark STR generation as successful even when some properties are skipped
Added comprehensive unit tests covering reserved names, invalid identifiers, name collisions, and the no-warnings case

Reviewed changes

Copilot reviewed 17 out of 17 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
src/Tasks/system.design/stronglytypedresourcebuilder.cs	Changed error tracking from string array to tuple array with structured warning data (key, reason, extraArg); updated all error-adding sites to include reason codes
src/Tasks/GenerateResource.cs	Changed from error logging to warning logging for STR property generation issues; task now succeeds even with warnings
src/Tasks/Resources/Strings.resx	Added four new warning message strings: MSB3827, MSB3828, MSB3829, MSB3830
src/Tasks/Resources/xlf/*.xlf	Added localization placeholders (marked as "new") for the four new warning messages across all language files
src/Tasks.UnitTests/ResourceHandling/GenerateResource_Tests.cs	Added four comprehensive tests covering reserved names, invalid identifiers, name collisions, and the no-warnings case

src/Tasks.UnitTests/ResourceHandling/GenerateResource_Tests.cs

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

The input dictionary uses StringComparer.InvariantCultureIgnoreCase, so pure case-insensitive collisions (e.g. "MyKey" vs "mykey") cannot exist in the input.

JanProvaznik

I would suggest partitioning the logic so that the builder tries to create the file and notes down what it had to skip to do that and for what reason (with enum for reasons), and then the Task decides how to log that information.

src/Tasks/GenerateResource.cs

src/Tasks/Resources/Strings.resx

src/Tasks/system.design/stronglytypedresourcebuilder.cs

src/Tasks/GenerateResource.cs

src/Tasks/system.design/stronglytypedresourcebuilder.cs

Architecture fixes - StronglyTypedResourceBuilder now produces list of records that represents warning, logic to choose resource key for message loggind moved to GenFixes after code review. StronglyTypedResourceBuilder now produces a list of records representing warnings; the logic to choose the resource key for message logging has been moved to GenerateResource. Added a HashSet to optimize searching for already-warned keys.erateResource. Added hashset to optimize sraching of already warned keys

JanProvaznik

looking better, still some code quality comments and a logical oversight, your extra else branch was actually correct

JanProvaznik · 2026-02-25T14:01:57Z

src/Tasks/GenerateResource.cs

+                        StronglyTypedResourceBuilder.ResourceWarningReason.NameCollision => "GenerateResource.STRPropertySkippedNameCollision",
+                        _ => throw new InvalidOperationException(),
+                    };
+                    _logger.LogWarningWithCodeFromResources(messageKey, warning.Key, warning.ExtraArg);


don't pass the second argument to those resources that don't take it

I suppose that requires switching what logging method is called instead of switching to retrive key

JanProvaznik · 2026-02-25T14:04:06Z

src/Tasks/system.design/stronglytypedresourcebuilder.cs

        private const int DocCommentLengthThreshold = 512;

+        // Describes a resource key that could not be converted to a valid STR property.
+        internal enum ResourceWarningReason


I'm not a fan of the "Warning" naming in general, that we warn about this is concern of the presentation layer (the Task), can it be something like KeySkippedReason/unmatchable something? because that's what happens, no?

JanProvaznik · 2026-02-25T14:07:37Z

src/Tasks/system.design/stronglytypedresourcebuilder.cs

        private const String ResMgrPropertyName = "ResourceManager";
        private const String CultureInfoFieldName = "resourceCulture";
        private const String CultureInfoPropertyName = "Culture";
+        private static readonly String reservedNames = String.Join(", ", ResMgrPropertyName, CultureInfoPropertyName);


why not const?
nit: i'd write just $"{ResMgrPropertyName}, {CultureInfoPropertyName}"

JanProvaznik · 2026-02-25T14:11:27Z

src/Tasks/system.design/stronglytypedresourcebuilder.cs

+            NameCollision,
+        }
+
+        internal sealed record ResourceWarning(string Key, ResourceWarningReason Reason, string ExtraArg);


ExtraArg is not clear, name it more clearly what is stored there

feel free to have multiple properties and fill the one that's relevant,
use string? to express the intent that null is valid

JanProvaznik · 2026-02-25T14:20:23Z

src/Tasks/system.design/stronglytypedresourcebuilder.cs

-                    errors.Add(entry.Key);
+
+                    // Warn about the current entry.
+                    warnings.Add(new ResourceWarning(entry.Key, ResourceWarningReason.NameCollision, alreadyAddedOriginalName));


alreadyAddedOriginalName is null when two valid identifiers
collide only by case (e.g. "MyRes" vs "myRes") — neither goes through fixup,
so reverseFixupTable has no entry and TryGetValue on line 824 doesn't set it.
The warning renders as a name collision exists with resource "". Restore the
IndexOfKey lookup from the earlier revision to retrieve the actual stored key
name

Add warning messages MSB3827–MSB3830 for skipped STR resource keys in…

0748bba

…stead of failing the build, ensuring STR files are still generated when possible

Copilot AI review requested due to automatic review settings February 25, 2026 07:21

Merge branch 'main' into dev/veronikao/dev/str-skip-property-warnings

1801da3

Copilot started reviewing on behalf of OvesN February 25, 2026 07:22 View session

Copilot AI reviewed Feb 25, 2026

View reviewed changes

src/Tasks.UnitTests/ResourceHandling/GenerateResource_Tests.cs Outdated Show resolved Hide resolved

OvesN self-assigned this Feb 25, 2026

OvesN requested a review from JanProvaznik February 25, 2026 07:27

OvesN and others added 2 commits February 25, 2026 08:30

Update src/Tasks.UnitTests/ResourceHandling/GenerateResource_Tests.cs

46f1ff7

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

Remove unreachable else branch in VerifyResourceNames

bc00ba7

The input dictionary uses StringComparer.InvariantCultureIgnoreCase, so pure case-insensitive collisions (e.g. "MyKey" vs "mykey") cannot exist in the input.

JanProvaznik requested changes Feb 25, 2026

View reviewed changes

JanProvaznik reviewed Feb 25, 2026

View reviewed changes

Conversation

OvesN commented Feb 25, 2026

Context

Changes Made

Testing

Notes

Uh oh!

OvesN commented Feb 25, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

JanProvaznik left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

JanProvaznik left a comment

Choose a reason for hiding this comment

Uh oh!

JanProvaznik Feb 25, 2026

Choose a reason for hiding this comment

Uh oh!

JanProvaznik Feb 25, 2026

Choose a reason for hiding this comment

Uh oh!

JanProvaznik Feb 25, 2026

Choose a reason for hiding this comment

Uh oh!

JanProvaznik Feb 25, 2026

Choose a reason for hiding this comment

Uh oh!

JanProvaznik Feb 25, 2026

Choose a reason for hiding this comment

Uh oh!

JanProvaznik Feb 25, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants