Do not report parse errors for '#:' in miscellaneous files #81385

RikkiGibson · 2025-11-21T01:42:28Z

Follow up to #81252

Removes unexpected errors for #: in miscellaneous files.
Fixes a completion crash for #:project $$ in the ctrl+N case. The file has a non-absolute path and we shouldn't try to use its containing directory as a base for path lookup.

Usually just the combination of: document is not part of an ordinary project, and document contains this directive, would make it be treated as an ordinary file. But there is also the case of virtual documents (not present on disk), containing these directives, e.g. when you hit ctrl+N in VS Code, set language mode to C# and start typing. Virtual documents are never treated as file-based programs, they are essentially only treated as miscellaneous files. But the user might be intending for this to be a file-based program, once they have saved it to disk. So reporting an error in this case is much more a nuisance than a benefit.

I also added a separate Features value to indicate that a file is a MiscellaneousFile, which I thought might be useful in contrast to something being labeled a FileBasedProgram. I'm a bit uncertain about this. As-is, directive errors are not going to be reported in such virtual documents, and it's not clear to me whether that's good or bad. At the least I think testing would be required before we go ahead with such directives since the code which reports the diagnostics may expect the file to have an on-disk path.

dibarbet

ide side lgtm

jcouv · 2025-11-21T07:09:43Z

Did we forget to create a test plan and label for file-based apps feature? #Closed

jcouv · 2025-11-21T07:28:52Z

src/Compilers/CSharp/Portable/CSharpParseOptions.cs

        /// In this mode, ignored directives <c>#:</c> are allowed.
        /// </remarks>
-        internal bool FileBasedProgram => Features.ContainsKey("FileBasedProgram");
+        internal bool AllowIgnoredDirectives => Features.ContainsKey("FileBasedProgram") || Features.ContainsKey("MiscellaneousFile");


Should those feature flags documented anywhere?
Not directly related to the PR: Aren't feature flags typically used for temporary or not fully supported scenarios? Should we consider exposing a proper API on parse options for file-based apps?

Perhaps, but, I will note that explicitly including these feature names in ordinary projects and similar to result in unspecified behavior. These are generally supposed to be only included by the tooling.

It’s possible there is a more preferred way of “tagging” compilations in the desired way here than just using special feature names. Happy to have that discussion in more depth.

jcouv · 2025-11-21T07:35:03Z

src/Compilers/CSharp/Portable/Parser/DirectiveParser.cs

            if (isActive)
            {
-                if (!lexer.Options.FileBasedProgram)
+                if (!lexer.Options.AllowIgnoredDirectives)


Sorry for my lack of context, but I'm surprised that the parsing of ignored directives is conditional in the first place. The spec only says we're adding directives to the language and that they are ignored: https://github.com/dotnet/csharplang/blob/main/proposals/csharp-14.0/ignored-directives.md #Closed

AFAIK, this only controls whether an error is reported on usage.

Not blocking this PR: should we update the spec or remove the condition? In other words, did we intentionally fork the language?

The spec says:

Compilers are also free to report errors if these directives are used in unsupported scenarios

It's possible we want to push some of those rules into the language, but it's not clear to me. I think the mechanism that decides whether the scenario is supported would also have to be defined in the language spec.

Yes, reporting an error on #: directives in normal project-based apps is intentional. That is because those directives are ignored for project-based apps and users might be confused if they were ignored silently. This is only a tooling feature though, i.e., roslyn implements it; sdk tells it the kind of the program (file-based vs project-based) via passing the feature flag. It is not a language feature since language has no way to interpret those directives.

That is because those directives are ignored for project-based apps and users might be confused if they were ignored silently.

That behavior makes sense.
Still, the net result is that we have forked the language, but I don't recall that was discussed as part of the language design. The spec only says we add these to the grammar and they are ignored, but our implementation diverges (errors in some cases). We're using a feature flag for a long-living feature, which smells.

I'll start an email thread.

jcouv · 2025-11-21T07:49:41Z

...torFeatures/CSharpTest/Completion/CompletionProviders/AppDirectiveCompletionProviderTests.cs

+
+        var markup = $"""
+            <Workspace>
+                <Project Language="C#" CommonReferences="true" AssemblyName="Test1" Features="FileBasedProgram=true">


Features="FileBasedProgram=true"

Did this test fail before the changes in this PR?
The reason I'm asking is because I see logic elsewhere that adds the "MiscellaneousFile" feature and therefore it's not clear whether this test should have the "FileBasedProgram" feature flag.

No, it passed. It’s reasonable for some of these tests to use FileBasedProgram feature flag, because we want to see how the editor features behave on file-based programs.

However I do think the actual applications of FileBasedProgram versus MiscellaneousFile feature names requires some discussion.

From the description of the bug being fixed, I expected a test like this one but without FileBasedProgram and with MiscellaneousFile. It used to fail, but now it would pass. Did I understand right?

jcouv · 2025-11-21T07:57:09Z

...torFeatures/CSharpTest/Completion/CompletionProviders/AppDirectiveCompletionProviderTests.cs

+            """;
+
+        // In this case, only stuff like drive roots would be recommended.
+        var expectedRoot = PlatformInformation.IsWindows ? @"C:" : "/";


@

nit: unnecessary @?

jcouv · 2025-11-21T07:59:22Z

...torFeatures/CSharpTest/Completion/CompletionProviders/AppDirectiveCompletionProviderTests.cs

+            """;
+
+        // In this case, only stuff like drive roots would be recommended.
+        var expectedRoot = PlatformInformation.IsWindows ? @"C:" : "/";


"C:"

nit: where does this value come from? Technically, it's possible not to have a "C:" drive, so I'm wondering whether this test depends on how the machine is set up #WontFix

If test machine lacks a C drive then this will fail.

jcouv

Done with review pass (commit 1) for compiler. Only glanced at IDE code

jjonescz · 2025-11-24T10:18:16Z

Did we forget to create a test plan and label for file-based apps feature?

We have a label: Feature - Run File #: and #! directives and file-based C# programs
I don't think we have a test plan; IIRC that might have been intentional since the relevant language feature "ignored directives" was merged in a single PR.

Do not report parse errors for '#:' in miscellaneous files

ffc8135

RikkiGibson requested review from a team as code owners November 21, 2025 01:42

github-actions bot added the Area-Compilers label Nov 21, 2025

dotnet-policy-service bot added the VSCode label Nov 21, 2025

dibarbet approved these changes Nov 21, 2025

View reviewed changes

jcouv reviewed Nov 21, 2025

View reviewed changes

jcouv self-assigned this Nov 21, 2025

jjonescz added the Feature - Run File #: and #! directives and file-based C# programs label Nov 21, 2025

Do not report parse errors for '#:' in miscellaneous files #81385

Are you sure you want to change the base?

Do not report parse errors for '#:' in miscellaneous files #81385

Uh oh!

Conversation

RikkiGibson commented Nov 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dibarbet left a comment

Choose a reason for hiding this comment

Uh oh!

jcouv commented Nov 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jcouv Nov 21, 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

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jcouv Nov 21, 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!

jcouv left a comment

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

RikkiGibson commented Nov 21, 2025 •

edited

Loading

jcouv commented Nov 21, 2025 •

edited

Loading

jcouv Nov 21, 2025 •

edited

Loading

jcouv Nov 21, 2025 •

edited

Loading

jjonescz commented Nov 24, 2025 •

edited

Loading