Skip to content

Recognize C# record as a type in CreateCSharpManifestResourceName#13870

Draft
Copilot wants to merge 3 commits into
mainfrom
copilot/fix-record-type-recognition
Draft

Recognize C# record as a type in CreateCSharpManifestResourceName#13870
Copilot wants to merge 3 commits into
mainfrom
copilot/fix-record-type-recognition

Conversation

Copy link
Copy Markdown
Contributor

Copilot AI commented May 26, 2026

Context

CreateCSharpManifestResourceName derives the manifest resource name from the first type declared in the adjacent .cs file. Its parser only triggered on the class keyword, and record (a C# 9 contextual keyword) was tokenized as a plain identifier — so files whose first top-level type is a record produced wrong or missing resource names, breaking embedded resource lookup and satellite-assembly localization.

// MyNs/Person.cs — sibling Person.resx was misnamed
namespace MyNs;
public record Person(string Name);

Changes Made

  • src/Shared/LanguageParser/CSharptokenEnumerator.cs — add "record" to s_keywordList so it tokenizes as a KeywordToken.
  • src/Tasks/CSharpParserUtilities.cs — in Extract, treat record the same as class to start resolving the type name. record class Foo also works because the subsequent class keyword resets and re-triggers resolution.
  • src/Tasks.UnitTests/CSharpParserUtilitites_Tests.cs — new RecordTypeResolution theory covering positional records, file-scoped namespaces, record class, sealed record, and record-before-class ordering.

Testing

Existing CreateCSharpManifestResourceName_Tests (33) still pass. New theory cases plus a reflection-based driver confirm the parser now returns the expected qualified name (e.g. MyNs.Person) for the scenarios above while leaving plain class behavior untouched.

Notes

record is contextual in C#, so promoting it to a reserved keyword in this lexer could mis-tokenize it when used as an identifier (e.g. var record = ...;). In practice this is benign here: Extract returns on the first detected type, so anything where record appears as an identifier is either inside a body the parser never reaches, or an edge case (top-level statements) that already has no class name to report.

@dotnet-policy-service
Copy link
Copy Markdown
Contributor

Hello @copilot, I noticed that you’re changing an .swr file or any file under src/Package/MSBuild.VSSetup.. Please make sure to validate this change by an experimental VS insertion. This is accomplished by pushing to an exp/* branch, which requires write permissions to this repo.

Copilot AI changed the title [WIP] Fix CreateCSharpManifestResourceName to recognize record types Recognize C# record as a type in CreateCSharpManifestResourceName May 26, 2026
Copilot AI requested a review from OvesN May 26, 2026 13:31
Copilot finished work on behalf of OvesN May 26, 2026 13:32
}
}
else if (t.InnerText == "class")
else if (t.InnerText == "class" || t.InnerText == "record")
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

hm I am a bit concerned what happens when people use record struct and record class, how is this tokenized in roslyn? 🤔 . But this PR improves on the current state where we don't recognize records at all...

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

At the end, also "enum", "structs" and interfaces are not recognized when it comes to resource name generation. Not sure if this method is the right place to find the type name regarding all possible types or if it will break other code.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

CreateCSharpManifestResourceName does not recognize record as a type, miscomputes manifest resource names

4 participants