Use Roslyn to parse directives in file-based programs #47978

jjonescz · 2025-03-28T12:50:28Z

Copilot

Pull Request Overview

This PR integrates Roslyn APIs for parsing file-based directives and updates test expectations accordingly.

Updated tests to expect dynamic target frameworks using ToolsetInfo.CurrentTargetFramework and refined error messages.
Introduced new tests for directives handling (after tokens, after #if blocks, and in comments) and refactored the virtual project building logic to use Roslyn parsing.

Reviewed Changes

Copilot reviewed 2 out of 16 changed files in this pull request and generated no comments.

File	Description
test/dotnet-project-convert.Tests/DotnetProjectConvertTests.cs	Updates test expectations for target framework and error messages, and adds new tests for directive handling.
src/Cli/dotnet/Commands/Run/VirtualProjectBuildingCommand.cs	Removes directive-removal logic, updates project file generation, and incorporates Roslyn API usage for directive parsing.

Files not reviewed (14)

src/Cli/dotnet/Commands/Run/LocalizableStrings.resx: Language not supported
src/Cli/dotnet/Commands/Run/xlf/LocalizableStrings.cs.xlf: Language not supported
src/Cli/dotnet/Commands/Run/xlf/LocalizableStrings.de.xlf: Language not supported
src/Cli/dotnet/Commands/Run/xlf/LocalizableStrings.es.xlf: Language not supported
src/Cli/dotnet/Commands/Run/xlf/LocalizableStrings.fr.xlf: Language not supported
src/Cli/dotnet/Commands/Run/xlf/LocalizableStrings.it.xlf: Language not supported
src/Cli/dotnet/Commands/Run/xlf/LocalizableStrings.ja.xlf: Language not supported
src/Cli/dotnet/Commands/Run/xlf/LocalizableStrings.ko.xlf: Language not supported
src/Cli/dotnet/Commands/Run/xlf/LocalizableStrings.pl.xlf: Language not supported
src/Cli/dotnet/Commands/Run/xlf/LocalizableStrings.pt-BR.xlf: Language not supported
src/Cli/dotnet/Commands/Run/xlf/LocalizableStrings.ru.xlf: Language not supported
src/Cli/dotnet/Commands/Run/xlf/LocalizableStrings.tr.xlf: Language not supported
src/Cli/dotnet/Commands/Run/xlf/LocalizableStrings.zh-Hans.xlf: Language not supported
src/Cli/dotnet/Commands/Run/xlf/LocalizableStrings.zh-Hant.xlf: Language not supported

Comments suppressed due to low confidence (2)

test/dotnet-project-convert.Tests/DotnetProjectConvertTests.cs:478

Ensure that the updated error message wording in property directives matches the intended format and is reflected consistently in user documentation.

expectedWildcardPattern: "The property directive needs to have two parts separated by a space like 'PropertyName PropertyValue': /app/Program.cs:1");

src/Cli/dotnet/Commands/Run/VirtualProjectBuildingCommand.cs:139

Review the removal of the _targetFilePath handling to ensure that the lack of directive removal from the source file does not lead to unintended side effects in downstream processing.

Debug.Assert(_directives.IsDefault, "{nameof(PrepareProjectInstance)} should not be called multiple times.");

jjonescz · 2025-04-01T07:58:45Z

@RikkiGibson @jaredpar @MiYanni for reviews, thanks #Resolved

jjonescz · 2025-04-01T07:59:51Z

src/Cli/dotnet/Commands/Run/LocalizableStrings.resx

  <data name="PropertyDirectiveMissingParts" xml:space="preserve">
-    <value>The property directive needs to have two parts separated by '=' like 'PropertyName=PropertyValue': {0}</value>
-    <comment>{0} is the file path and line number. {Locked="="}</comment>
+    <value>The property directive needs to have two parts separated by a space like 'PropertyName PropertyValue': {0}</value>


📝 In the previous PR, I've changed the separator to a space but forgot to change this error message. #Resolved

jjonescz · 2025-04-01T08:00:56Z

test/dotnet-project-convert.Tests/DotnetProjectConvertTests.cs

                #:property Name   Value   
                #:property NugetPackageDescription "My package with spaces"
+                 #  !  /test
+                  #!  /program   x   


📝 Needed to reorder the directives in this test slightly because using Roslyn APIs means we now ignore all directives that appear after invalid directives. #Resolved

jaredpar · 2025-04-02T22:26:55Z

test/dotnet-project-convert.Tests/DotnetProjectConvertTests.cs

+    {
+        VerifyConversion(
+            inputCSharp: """
+                #:property Prop 1


Do we have tests for when the property value has characters that could impact XML processing? For example:

#:property Evil </Property> ``` #Resolved

Yes, Directives_Escaping

MiYanni · 2025-04-03T00:56:20Z

src/Cli/dotnet/Commands/Run/VirtualProjectBuildingCommand.cs

+                // Include the preceding whitespace in the span, i.e., span will be the whole line.
+                var span = previousWhiteSpaceSpan.IsEmpty ? trivia.FullSpan : TextSpan.FromBounds(previousWhiteSpaceSpan.Start, trivia.FullSpan.End);
+
+                var message = trivia.GetStructure() is IgnoredDirectiveTriviaSyntax { EndOfDirectiveToken.LeadingTrivia: [{ RawKind: (int)SyntaxKind.PreprocessingMessageTrivia } messageTrivia] }


Man, I need to get better at reading/writing using pattern matching syntax. ☠️ #Resolved

It's very powerful, we use it in roslyn a lot, but I guess it can seem also quite opaque at first and maybe I'm overusing it :D

Naw, it just has a learning curve. I like simplifying imperative logic, so it is definitely something I want to get used to using!

The API review decision will cause this to change to use a more straightforward Message prop access, right? We won't ship with the message accessible in this way on this particular trivia?

The API review decision will cause this to change to use a more straightforward Message prop access, right?

Yes, once dotnet/roslyn#77968 is merged and flows here, this will be simplified as you say.

RikkiGibson · 2025-04-07T16:28:53Z

Taking a look #Resolved

RikkiGibson · 2025-04-07T16:45:55Z

test/dotnet-project-convert.Tests/DotnetProjectConvertTests.cs

+            expectedCSharp: """
+                #define X
+                Console.WriteLine();
+                #:property Prop 3


Would it be reasonable for the convert invocation to fail in the presence of this directive? Essentially make the user get their directives in shape before running 'convert' so that we don't leave them in an in-between state, with a directive that is never going to work anyway, and which they now have to figure out how to move over to their project cleanly. #Resolved

That's a good idea, I'm going to implement that, thanks.

RikkiGibson · 2025-04-07T16:48:44Z

src/Cli/dotnet/Commands/Run/VirtualProjectBuildingCommand.cs

        _directives = FindDirectives(sourceFile);

-        // If there were any `#:` directives, remove them from the file.
-        // (This is temporary until Roslyn is updated to ignore them.)


Pretty much the same logic is sticking around for convert right? #Resolved

RikkiGibson · 2025-04-07T16:49:36Z

src/Cli/dotnet/Commands/Run/VirtualProjectBuildingCommand.cs

+        foreach (var trivia in result.Token.LeadingTrivia)
        {
-            var lineText = sourceFile.Text.ToString(line.Span);
+            // Stop when the trivia contains an error (e.g., because it's after #if).


I wonder if we should be more specific about what kinds of errors make us stop. Essentially if something happens to be malformed is doing that likely to prevent a cascade or produce a result which is more actionable. Perhaps we can simply adjust if we find that to be the case. #Resolved

During 'dotnet run file.cs', compiler reports errors that I think are actionable. For 'dotnet project convert', addressing your other comment should start producing more actionable errors there as well.

RikkiGibson · 2025-04-07T16:51:34Z

test/dotnet-project-convert.Tests/DotnetProjectConvertTests.cs

                  <PropertyGroup>
                    <OutputType>Exe</OutputType>
-                    <TargetFramework>net10.0</TargetFramework>
+                    <TargetFramework>{ToolsetInfo.CurrentTargetFramework}</TargetFramework>


I am curious what happens when user writes #:property TargetFramework net9.0 or #:property TargetFramework net472 or what have you. Or, heck, #:property OutputType Library. No need to test it here and now but perhaps eventually to demonstrate the behavior. #Resolved

There is a test for this - it's named Directives, see below. The user values will be added to a PropertyGroup below this one, so MSBuild will deal with them and overwrite the built-ins, see also #47702 (comment).

sdk/test/dotnet-project-convert.Tests/DotnetProjectConvertTests.cs

Lines 319 to 359 in 5086618

[Fact]

public void Directives()

{

VerifyConversion(

inputCSharp: """

#!/program

#:sdk Microsoft.NET.Sdk

#:sdk Aspire.Hosting.Sdk 9.1.0

#:property TargetFramework net11.0

#:package System.CommandLine 2.0.0-beta4.22272.1

#:property LangVersion preview

Console.WriteLine();

""",

expectedProject: $"""

<Project Sdk="Microsoft.NET.Sdk">

<Sdk Name="Aspire.Hosting.Sdk" Version="9.1.0" />

<PropertyGroup>

<OutputType>Exe</OutputType>

<TargetFramework>{ToolsetInfo.CurrentTargetFramework}</TargetFramework>

<ImplicitUsings>enable</ImplicitUsings>

<Nullable>enable</Nullable>

</PropertyGroup>

<PropertyGroup>

<TargetFramework>net11.0</TargetFramework>

<LangVersion>preview</LangVersion>

</PropertyGroup>

<ItemGroup>

<PackageReference Include="System.CommandLine" Version="2.0.0-beta4.22272.1" />

</ItemGroup>

</Project>

""",

expectedCSharp: """

Console.WriteLine();

""");

}

333fred · 2025-04-07T23:59:46Z

src/Cli/dotnet/Commands/Run/VirtualProjectBuildingCommand.cs

+
+            if (trivia.IsKind(SyntaxKind.ShebangDirectiveTrivia))
+            {
+                Debug.Assert(previousWhiteSpaceSpan.IsEmpty, "#! should be at the first character");


Don't think this is something we can assert; just skip it or otherwise produce an error. Asserts should be for invariants, not potential bad syntax from a user. Please make sure we have tests that would trigger this assert. #Resolved

I don't think we can trigger this assert. Roslyn doesn't produce ShebangDirectiveTrivia if it's not at the first character, see the hashPosition == 0 condition in https://github.com/dotnet/roslyn/blob/256273c38fd5efffa8a18b921f66b281176b02c1/src/Compilers/CSharp/Portable/Parser/DirectiveParser.cs#L111-L114.

That being said, we have tests here in sdk that have a shebang directive with some whitespace before, so if parsing of that changes in Roslyn, this assert would be hit.

So, I would consider this to be an invariant and if it changes, the assert will catch it, and we might need to react somehow.

I don't think we can trigger this assert. Roslyn doesn't produce ShebangDirectiveTrivia if it's not at the first character, see the hashPosition == 0 condition in https://github.com/dotnet/roslyn/blob/256273c38fd5efffa8a18b921f66b281176b02c1/src/Compilers/CSharp/Portable/Parser/DirectiveParser.cs#L111-L114

This seems like a bug, not a feature. I expect that to be fixed at some point.

Makes sense, I will remove the assert, thanks.

Filed dotnet/roslyn#78054 to follow up on this.

Use Roslyn to parse directives in file-based programs

5086618

jjonescz added the Area-run-file Items related to the "dotnet run <file>" effort label Mar 28, 2025

ghost added the untriaged Request triage from a team member label Mar 28, 2025

jjonescz marked this pull request as ready for review March 28, 2025 14:45

Copilot AI review requested due to automatic review settings March 28, 2025 14:45

Copilot AI reviewed Mar 28, 2025

View reviewed changes

jjonescz requested review from DamianEdwards, Forgind, MiYanni, RikkiGibson, baronfel, chsienki and jaredpar March 28, 2025 14:46

jjonescz commented Apr 1, 2025

View reviewed changes

jaredpar assigned 333fred, RikkiGibson and chsienki Apr 2, 2025

jaredpar reviewed Apr 2, 2025

View reviewed changes

MiYanni approved these changes Apr 3, 2025

View reviewed changes

RikkiGibson reviewed Apr 7, 2025

View reviewed changes

RikkiGibson approved these changes Apr 7, 2025

View reviewed changes

333fred reviewed Apr 8, 2025

View reviewed changes

jjonescz added 2 commits April 8, 2025 12:40

Fail conversion on invalid directives

0edb91c

Merge branch 'main' into sprint-directives-with-roslyn

4cdfc15

jjonescz requested a review from 333fred April 8, 2025 11:51

jjonescz added 2 commits April 8, 2025 18:02

Handle shebang leading whitespace

9c368ce

Merge branch 'main' into sprint-directives-with-roslyn

45006e2

333fred approved these changes Apr 8, 2025

View reviewed changes

Merge branch 'main' into sprint-directives-with-roslyn

5c1af13

jaredpar mentioned this pull request Apr 9, 2025

dotnet run app.cs is creating obj if there are shebang directives #48278

Closed

jjonescz merged commit f05154a into dotnet:main Apr 9, 2025
39 checks passed

jjonescz deleted the sprint-directives-with-roslyn branch April 9, 2025 08:16

	[Fact]
	public void Directives()
	{
	VerifyConversion(
	inputCSharp: """
	#!/program
	#:sdk Microsoft.NET.Sdk
	#:sdk Aspire.Hosting.Sdk 9.1.0
	#:property TargetFramework net11.0
	#:package System.CommandLine 2.0.0-beta4.22272.1
	#:property LangVersion preview
	Console.WriteLine();
	""",
	expectedProject: $"""
	<Project Sdk="Microsoft.NET.Sdk">

	<Sdk Name="Aspire.Hosting.Sdk" Version="9.1.0" />

	<PropertyGroup>
	<OutputType>Exe</OutputType>
	<TargetFramework>{ToolsetInfo.CurrentTargetFramework}</TargetFramework>
	<ImplicitUsings>enable</ImplicitUsings>
	<Nullable>enable</Nullable>
	</PropertyGroup>

	<PropertyGroup>
	<TargetFramework>net11.0</TargetFramework>
	<LangVersion>preview</LangVersion>
	</PropertyGroup>

	<ItemGroup>
	<PackageReference Include="System.CommandLine" Version="2.0.0-beta4.22272.1" />
	</ItemGroup>

	</Project>

	""",
	expectedCSharp: """
	Console.WriteLine();
	""");
	}

Use Roslyn to parse directives in file-based programs #47978

Use Roslyn to parse directives in file-based programs #47978

Uh oh!

Conversation

jjonescz commented Mar 28, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

jjonescz commented Apr 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jjonescz Apr 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jjonescz Apr 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jaredpar Apr 2, 2025 • edited by jjonescz Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MiYanni Apr 3, 2025 • edited by jjonescz Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RikkiGibson commented Apr 7, 2025 • edited by jjonescz Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

RikkiGibson Apr 7, 2025 • edited by jjonescz Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RikkiGibson Apr 7, 2025 • edited by jjonescz Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RikkiGibson Apr 7, 2025 • edited by jjonescz Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RikkiGibson Apr 7, 2025 • edited by jjonescz Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jjonescz Apr 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

333fred Apr 7, 2025 • edited by jjonescz Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jjonescz Apr 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

jjonescz commented Apr 1, 2025 •

edited

Loading

jjonescz Apr 1, 2025 •

edited

Loading

jjonescz Apr 1, 2025 •

edited

Loading

jaredpar Apr 2, 2025 •

edited by jjonescz

Loading

MiYanni Apr 3, 2025 •

edited by jjonescz

Loading

RikkiGibson commented Apr 7, 2025 •

edited by jjonescz

Loading

RikkiGibson Apr 7, 2025 •

edited by jjonescz

Loading

RikkiGibson Apr 7, 2025 •

edited by jjonescz

Loading

RikkiGibson Apr 7, 2025 •

edited by jjonescz

Loading

RikkiGibson Apr 7, 2025 •

edited by jjonescz

Loading

jjonescz Apr 7, 2025 •

edited

Loading

333fred Apr 7, 2025 •

edited by jjonescz

Loading

jjonescz Apr 8, 2025 •

edited

Loading