File-level directives: allow quoting and additional properties - #55592
File-level directives: allow quoting and additional properties#55592jjonescz wants to merge 19 commits into
Conversation
|
Azure Pipelines: Successfully started running 2 pipeline(s). 1 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
There was a problem hiding this comment.
Pull request overview
This PR updates file-based program #: directive parsing/emission to support double-quoted values (for whitespace) and additional MSBuild item metadata on #:package, #:project, and #:ref, and adds a new analyzer (CA2267) to help migrate legacy unquoted-whitespace directives.
Changes:
- Extend file-level directive parsing to support quoted values and trailing
Name=Valuemetadata, and emit metadata into generated MSBuild items. - Add/expand CLI and conversion tests for quoting and metadata round-tripping.
- Introduce CA2267 (“Prefer quoted file-based program directive values”) with code fix, plus documentation/resource updates.
Reviewed changes
Copilot reviewed 46 out of 46 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| test/dotnet.Tests/CommandTests/Run/RunFileTests_Directives.cs | Adds E2E coverage for #:ref metadata emitted into <ProjectReference>. |
| test/dotnet.Tests/CommandTests/Run/FileBasedAppSourceEditorTests.cs | Tests directive editing/round-tripping with metadata and quoting. |
| test/dotnet.Tests/CommandTests/Project/Convert/DotnetProjectConvertTests.cs | Adds conversion tests for quoting/metadata and updates escaping/whitespace expectations. |
| src/Microsoft.CodeAnalysis.NetAnalyzers/tests/Microsoft.CodeAnalysis.NetAnalyzers.UnitTests/Microsoft.NetCore.Analyzers/Usage/PreferQuotedFileBasedProgramDirectiveTests.cs | New unit tests for CA2267 analyzer + fixer. |
| src/Microsoft.CodeAnalysis.NetAnalyzers/src/Utilities/Compiler/DiagnosticCategoryAndIdRanges.txt | Extends Usage ID range to include CA2267. |
| src/Microsoft.CodeAnalysis.NetAnalyzers/src/Microsoft.CodeAnalysis.NetAnalyzers/Microsoft.NetCore.Analyzers/xlf/MicrosoftNetCoreAnalyzersResources.zh-Hant.xlf | Adds localized entries for new CA2267 strings. |
| src/Microsoft.CodeAnalysis.NetAnalyzers/src/Microsoft.CodeAnalysis.NetAnalyzers/Microsoft.NetCore.Analyzers/xlf/MicrosoftNetCoreAnalyzersResources.zh-Hans.xlf | Adds localized entries for new CA2267 strings. |
| src/Microsoft.CodeAnalysis.NetAnalyzers/src/Microsoft.CodeAnalysis.NetAnalyzers/Microsoft.NetCore.Analyzers/xlf/MicrosoftNetCoreAnalyzersResources.tr.xlf | Adds localized entries for new CA2267 strings. |
| src/Microsoft.CodeAnalysis.NetAnalyzers/src/Microsoft.CodeAnalysis.NetAnalyzers/Microsoft.NetCore.Analyzers/xlf/MicrosoftNetCoreAnalyzersResources.ru.xlf | Adds localized entries for new CA2267 strings. |
| src/Microsoft.CodeAnalysis.NetAnalyzers/src/Microsoft.CodeAnalysis.NetAnalyzers/Microsoft.NetCore.Analyzers/xlf/MicrosoftNetCoreAnalyzersResources.pt-BR.xlf | Adds localized entries for new CA2267 strings. |
| src/Microsoft.CodeAnalysis.NetAnalyzers/src/Microsoft.CodeAnalysis.NetAnalyzers/Microsoft.NetCore.Analyzers/xlf/MicrosoftNetCoreAnalyzersResources.pl.xlf | Adds localized entries for new CA2267 strings. |
| src/Microsoft.CodeAnalysis.NetAnalyzers/src/Microsoft.CodeAnalysis.NetAnalyzers/Microsoft.NetCore.Analyzers/xlf/MicrosoftNetCoreAnalyzersResources.ko.xlf | Adds localized entries for new CA2267 strings. |
| src/Microsoft.CodeAnalysis.NetAnalyzers/src/Microsoft.CodeAnalysis.NetAnalyzers/Microsoft.NetCore.Analyzers/xlf/MicrosoftNetCoreAnalyzersResources.ja.xlf | Adds localized entries for new CA2267 strings. |
| src/Microsoft.CodeAnalysis.NetAnalyzers/src/Microsoft.CodeAnalysis.NetAnalyzers/Microsoft.NetCore.Analyzers/xlf/MicrosoftNetCoreAnalyzersResources.it.xlf | Adds localized entries for new CA2267 strings. |
| src/Microsoft.CodeAnalysis.NetAnalyzers/src/Microsoft.CodeAnalysis.NetAnalyzers/Microsoft.NetCore.Analyzers/xlf/MicrosoftNetCoreAnalyzersResources.fr.xlf | Adds localized entries for new CA2267 strings. |
| src/Microsoft.CodeAnalysis.NetAnalyzers/src/Microsoft.CodeAnalysis.NetAnalyzers/Microsoft.NetCore.Analyzers/xlf/MicrosoftNetCoreAnalyzersResources.es.xlf | Adds localized entries for new CA2267 strings. |
| src/Microsoft.CodeAnalysis.NetAnalyzers/src/Microsoft.CodeAnalysis.NetAnalyzers/Microsoft.NetCore.Analyzers/xlf/MicrosoftNetCoreAnalyzersResources.de.xlf | Adds localized entries for new CA2267 strings. |
| src/Microsoft.CodeAnalysis.NetAnalyzers/src/Microsoft.CodeAnalysis.NetAnalyzers/Microsoft.NetCore.Analyzers/xlf/MicrosoftNetCoreAnalyzersResources.cs.xlf | Adds localized entries for new CA2267 strings. |
| src/Microsoft.CodeAnalysis.NetAnalyzers/src/Microsoft.CodeAnalysis.NetAnalyzers/Microsoft.NetCore.Analyzers/Usage/PreferQuotedFileBasedProgramDirective.Fixer.cs | Adds shared fixer base for CA2267. |
| src/Microsoft.CodeAnalysis.NetAnalyzers/src/Microsoft.CodeAnalysis.NetAnalyzers/Microsoft.NetCore.Analyzers/Usage/PreferQuotedFileBasedProgramDirective.cs | Adds shared diagnostic descriptor for CA2267. |
| src/Microsoft.CodeAnalysis.NetAnalyzers/src/Microsoft.CodeAnalysis.NetAnalyzers/Microsoft.NetCore.Analyzers/MicrosoftNetCoreAnalyzersResources.resx | Adds CA2267 resource strings. |
| src/Microsoft.CodeAnalysis.NetAnalyzers/src/Microsoft.CodeAnalysis.NetAnalyzers/AnalyzerReleases.Unshipped.md | Registers CA2267 in unshipped analyzer list. |
| src/Microsoft.CodeAnalysis.NetAnalyzers/src/Microsoft.CodeAnalysis.NetAnalyzers.sarif.template | Adds CA2267 to SARIF template metadata. |
| src/Microsoft.CodeAnalysis.NetAnalyzers/src/Microsoft.CodeAnalysis.NetAnalyzers.md | Adds generated documentation entry for CA2267. |
| src/Microsoft.CodeAnalysis.NetAnalyzers/src/Microsoft.CodeAnalysis.CSharp.NetAnalyzers/Microsoft.NetCore.Analyzers/Usage/FileBasedProgramDirectiveQuoting.cs | Adds shared quoting detection/rewrite logic for analyzer/fixer. |
| src/Microsoft.CodeAnalysis.NetAnalyzers/src/Microsoft.CodeAnalysis.CSharp.NetAnalyzers/Microsoft.NetCore.Analyzers/Usage/CSharpPreferQuotedFileBasedProgramDirective.Fixer.cs | Implements C# code fix for CA2267. |
| src/Microsoft.CodeAnalysis.NetAnalyzers/src/Microsoft.CodeAnalysis.CSharp.NetAnalyzers/Microsoft.NetCore.Analyzers/Usage/CSharpPreferQuotedFileBasedProgramDirective.cs | Implements C# analyzer for CA2267. |
| src/Cli/Microsoft.DotNet.FileBasedPrograms/xlf/FileBasedProgramsResources.zh-Hant.xlf | Adds localized CLI strings for new directive parsing errors. |
| src/Cli/Microsoft.DotNet.FileBasedPrograms/xlf/FileBasedProgramsResources.zh-Hans.xlf | Adds localized CLI strings for new directive parsing errors. |
| src/Cli/Microsoft.DotNet.FileBasedPrograms/xlf/FileBasedProgramsResources.tr.xlf | Adds localized CLI strings for new directive parsing errors. |
| src/Cli/Microsoft.DotNet.FileBasedPrograms/xlf/FileBasedProgramsResources.ru.xlf | Adds localized CLI strings for new directive parsing errors. |
| src/Cli/Microsoft.DotNet.FileBasedPrograms/xlf/FileBasedProgramsResources.pt-BR.xlf | Adds localized CLI strings for new directive parsing errors. |
| src/Cli/Microsoft.DotNet.FileBasedPrograms/xlf/FileBasedProgramsResources.pl.xlf | Adds localized CLI strings for new directive parsing errors. |
| src/Cli/Microsoft.DotNet.FileBasedPrograms/xlf/FileBasedProgramsResources.ko.xlf | Adds localized CLI strings for new directive parsing errors. |
| src/Cli/Microsoft.DotNet.FileBasedPrograms/xlf/FileBasedProgramsResources.ja.xlf | Adds localized CLI strings for new directive parsing errors. |
| src/Cli/Microsoft.DotNet.FileBasedPrograms/xlf/FileBasedProgramsResources.it.xlf | Adds localized CLI strings for new directive parsing errors. |
| src/Cli/Microsoft.DotNet.FileBasedPrograms/xlf/FileBasedProgramsResources.fr.xlf | Adds localized CLI strings for new directive parsing errors. |
| src/Cli/Microsoft.DotNet.FileBasedPrograms/xlf/FileBasedProgramsResources.es.xlf | Adds localized CLI strings for new directive parsing errors. |
| src/Cli/Microsoft.DotNet.FileBasedPrograms/xlf/FileBasedProgramsResources.de.xlf | Adds localized CLI strings for new directive parsing errors. |
| src/Cli/Microsoft.DotNet.FileBasedPrograms/xlf/FileBasedProgramsResources.cs.xlf | Adds localized CLI strings for new directive parsing errors. |
| src/Cli/Microsoft.DotNet.FileBasedPrograms/VirtualProjectBuilder.cs | Emits directive metadata as child elements on generated MSBuild items. |
| src/Cli/Microsoft.DotNet.FileBasedPrograms/InternalAPI.Unshipped.txt | Tracks new internal API surface for directive metadata properties. |
| src/Cli/Microsoft.DotNet.FileBasedPrograms/FileLevelDirectiveHelpers.cs | Implements tokenization, quoted values, and metadata parsing for directives. |
| src/Cli/Microsoft.DotNet.FileBasedPrograms/FileBasedProgramsResources.resx | Adds/updates directive parsing error messages. |
| src/Cli/dotnet/Commands/Project/Convert/ProjectConvertCommand.cs | Preserves #:ref metadata when converting to a project. |
| documentation/general/dotnet-run-file.md | Documents quoting rules, legacy behavior, and new metadata support. |
Suppressed comments (1)
src/Cli/Microsoft.DotNet.FileBasedPrograms/FileLevelDirectiveHelpers.cs:565
- Metadata element names are emitted directly into the generated project file. XmlConvert.VerifyName allows ':' (QName) which can yield invalid MSBuild XML without an xmlns declaration. Use VerifyNCName here to ensure generated elements are always safe.
try
{
name = XmlConvert.VerifyName(name);
}
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 46 out of 46 changed files in this pull request and generated 1 comment.
Suppressed comments (5)
src/Cli/Microsoft.DotNet.FileBasedPrograms/FileLevelDirectiveHelpers.cs:366
- Token.ContainsDiagnostics is not specific to unterminated quotes; it will also be true for invalid escape sequences (e.g. "\q"). Reporting UnterminatedQuoteInDirective for all diagnostics can mislead users. Distinguish the unterminated-quote case (missing closing ") from other invalid string literal diagnostics and use InvalidQuoteInDirective for the latter.
var token = SyntaxFactory.ParseToken(text, offset: i);
if (token.ContainsDiagnostics)
{
context.ReportError(FileBasedProgramsResources.UnterminatedQuoteInDirective);
return null;
src/Cli/Microsoft.DotNet.FileBasedPrograms/FileLevelDirectiveHelpers.cs:572
- Metadata names are emitted as XML element names. XmlConvert.VerifyName permits ':' (QName), which can yield invalid MSBuild XML when written as <a:b> without a namespace binding. Prefer XmlConvert.VerifyNCName here to prevent generating malformed project files.
try
{
name = XmlConvert.VerifyName(name);
}
catch (XmlException ex)
src/Cli/Microsoft.DotNet.FileBasedPrograms/FileLevelDirectiveHelpers.cs:563
- ParseMetadata treats tokens like "=value" as valid because it only checks separatorIndex < 0. XmlConvert.VerifyName("") will throw, which can surface as an unhandled exception instead of a directive parse error. Treat separatorIndex <= 0 as invalid metadata and report InvalidDirectiveMetadata.
var separatorIndex = token.IndexOf('=');
if (separatorIndex < 0)
{
context.ReportError(string.Format(FileBasedProgramsResources.InvalidDirectiveMetadata, token));
return null;
src/Cli/Microsoft.DotNet.FileBasedPrograms/FileLevelDirectiveHelpers.cs:984
- Ref.Metadata is left uninitialized (default ImmutableArray), unlike Package/Project which default to Empty. Default ImmutableArray can throw on common operations and forces callers to handle IsDefault. Initialize to ImmutableArray.Empty for consistency and safer consumption.
public ImmutableArray<(string Name, string Value)> Metadata { get; init; }
test/dotnet.Tests/CommandTests/Run/RunFileTests_Directives.cs:350
- The new RefDirective_Metadata test summary claims it validates that trailing #:ref metadata is emitted into the generated , but the test only asserts the app output. If metadata emission regresses (metadata dropped), the build/run would likely still succeed and this test would not fail. Add an assertion that inspects the generated virtual project and verifies test and a b c are present under the ProjectReference.
|
Azure Pipelines: Successfully started running 2 pipeline(s). 1 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
|
|
||
| namespace Microsoft.NetCore.Analyzers.Usage | ||
| { | ||
| public abstract class PreferQuotedFileBasedProgramDirectiveFixer : CodeFixProvider |
There was a problem hiding this comment.
nit: Is this correct?
We changed nearly every other analyzer over to the SyntaxEditorBasedCodeFixProvider to ensure that all the hookups and handling are correct and done consistently, as well as ensuring that none of the issues with BatchFixer surface up.
The fixes that are being done for file-based program directives are a bit simpler, likely with no concern over nesting; but the lack of other features like supported fix-all scopes. ordering, etc should generally still be impactful
| /// <summary> | ||
| /// Validates that <paramref name="name"/> is a valid XML NCName, the constraint MSBuild applies to | ||
| /// property and item-metadata names (an NCName additionally disallows the ':' that a plain XML name | ||
| /// permits). Returns <see langword="true"/> when valid; otherwise returns <see langword="false"/> and | ||
| /// sets <paramref name="errorMessage"/> to the underlying validation-failure message. | ||
| /// </summary> | ||
| public static bool IsValidMSBuildName(string name, out string? errorMessage) |
There was a problem hiding this comment.
There are some reserved MSBuild names like Identity that will cause the build to fail with
"Identity" is a reserved item metadata, and cannot be redefined as a custom metadata on the item.
The build failed.
Is that something we have the capability of testing/filtering for?
There was a problem hiding this comment.
In a similar vein, we're allowing cases like #:package Name@1.0 Version=2.0 which will have the latter override what we emit for the former. Feels like a likely bug if a user writes something like that; so perhaps such names should also be warned on.
There was a problem hiding this comment.
Also duplicates where last one wins, so a user can write Prop=a Prop=b and then not understand that Prop=a is "overwritten"
| continue; | ||
| } | ||
|
|
||
| if (!FileBasedProgramDirectiveQuoting.TryGetQuotedForm(kind, value, out _)) |
There was a problem hiding this comment.
This is causing it to bail and not report a diagnostic and seems to mismatch with how the SDK parsing works, right?
consider #:package Name@1.0 Property which hits the legacy path, so we won't get a diagnostic (because we cannot give a fix) even though the user should ideally be made aware they have something on the legacy pattern
| |-|-| | ||
| |Category|Usage| | ||
| |Enabled|True| | ||
| |Severity|Warning| |
There was a problem hiding this comment.
Is warning the right severity here? The old form can be used correctly, its just harder and more error prone.
Is the justification that the old form is essentially deprecated or similar?
| { | ||
| // Characters that are not allowed in a directive or metadata name because they would be confused | ||
| // with a separator: whitespace, '@', '=', '/'. | ||
| private static readonly Regex s_disallowedNameCharacters = new("""[\s@=/]""", RegexOptions.Compiled); |
There was a problem hiding this comment.
Do we expect this to be frequent enough that Compiled is worthwhile? I'd expect compilations to last a very small amount of time and be thrown away, so we'd either want a source generated version or just the standard interpreted; otherwise we may be paying more for the regex compilation than the work we're actually doing (which is a handful of single lines, most of which are likely small)
| [TestMethod] | ||
| public async Task MultipleDirectives_AllFixedAsync() | ||
| { | ||
| await new VerifyCS.Test |
There was a problem hiding this comment.
nit: We should have NumberOfFixAllIterations = 1 here. Nothing about this change should require 2 passes and so that ensures we're actually invoking FixAll and having it done as expected perf wise
| namespace Microsoft.NetCore.Analyzers.Usage.UnitTests | ||
| { | ||
| [TestClass] | ||
| public class PreferQuotedFileBasedProgramDirectiveTests |
There was a problem hiding this comment.
We don't have any tests covering trivia preservation
| async ct => | ||
| { | ||
| var text = await context.Document.GetTextAsync(ct).ConfigureAwait(false); | ||
| return context.Document.WithText(text.Replace(triviaSpan, newDirectiveText)); |
There was a problem hiding this comment.
This is going to reload the document for each individual fix in a multi-fix scenario, right?
That's why we have SyntaxEditorBasedCodeFixProvider instead to help force down the path that allows actually batching it all in a single pass?
Resolves #52399.
Resolves #49367.
Docs: dotnet/docs#55352