Fbp live directive diagnostics #80575

RikkiGibson · Oct 7, 2025

note: this is currently in a majorly hacky state. Working out the API surface that we want from an SDK source package, by coping the impl and modifying.

Also includes changes from #80410 until we merge that

RikkiGibson · Oct 7, 2025

src/Analyzers/CSharp/Analyzers/FileBasedPrograms/AppDirectiveHelpers.cs

+        return builder.ToImmutable();
+    }
+
+    public static void FindDirectives(


@jjonescz wanted you to see the helper I extracted. This is what I have so far, for what the analyzer would call in order to get the diagnostics from "SDK layer".

RikkiGibson · Oct 7, 2025

src/Analyzers/CSharp/Analyzers/FileBasedPrograms/AppDirectiveHelpers.cs

+                    // TODO2: no Path.GetRelativePath on netstandard
+                    directiveText = PathUtilities.GetRelativePath(sourceDirectory, fullFilePath);
+                }
+                else if (!File.Exists(resolvedProjectPath))


In an ordinary analyzer, checking if files exist on disk is not ok, since nothing will trigger the analyzer to re-run if the disk state changes. I am unsure if this is also a significant problem for an analyzer which we only want to run in the editor. @jasonmalinowski for a spot check.

But, I don't know what could substitute for this. I don't think it's realistic to pass all the '*.csproj's' paths which could be used as additional files or something like that.

Note that I'm extracting this IO access into a separate "evaluation" phase in dotnet/sdk#51108. So the IDE could in theory choose to skip this, but that doesn't sound very nice, as then the IDE wouldn't get the relevant #:project diagnostics.

Once I get through some other feedback I will try this out in some scenarios where we also mess with the file system.

e.g. if we have a bad #:project ./path/to/project.csproj, and then we go create the project at that location on disk, what is the experience. Does just tabbing over to the editor make the error go away? Do you have to make edits? close/reopen the file? etc. As well as the opposite, deleting a csproj which is already present.

The thing is, creating and deleting csproj's is an infrequent enough event that, I feel like this analyzer can have a little file I/O, as a treat. Especially because the behavior doesn't depend on file contents

RikkiGibson · Oct 7, 2025

src/Analyzers/CSharp/Analyzers/FileBasedPrograms/AppDirectiveDiagnosticAnalyzer.cs

+#pragma warning disable IDE0002 // Name can be simplified
+            var diagnosticBag = FileBasedPrograms.DiagnosticBag.Collect(out var diagnosticsBuilder);
+#pragma warning restore IDE0002
+            AppDirectiveHelpers.FindDirectives(


@jjonescz call site for the newly extracted helper.

jjonescz · Oct 8, 2025

src/Analyzers/CSharp/Analyzers/FileBasedPrograms/AppDirectiveDiagnosticAnalyzer.cs

+{
+    public const string DiagnosticId = "FileBasedPrograms";
+
+    private static readonly DiagnosticDescriptor Rule = new DiagnosticDescriptor(


I'm getting this warning when I open this in VSCode (and didn't see it reported by CI, so just FYI):

The symbol 'DiagnosticDescriptor.DiagnosticDescriptor(string, string, string, string, DiagnosticSeverity, bool, string?, string?, params string[])' is banned in this project: Analyzers should extend 'AbstractBuiltInCodeStyleDiagnosticAnalyzer' or 'AbstractCodeQualityDiagnosticAnalyzer' instead

jjonescz · Oct 8, 2025

src/Analyzers/CSharp/Analyzers/CSharpAnalyzersResources.resx

    <value>Property accessor can be simplified</value>
  </data>
+
+  <!-- TODO2: should this live here? Or sdk source package gives us some other resource file? -->


I guess source packages can contain any sources, including resx and xlf, and if these strings need to be used inside SDK too, I don't see other option than having these in the package.

I'll try creating a new resource file next to AppDirectiveHelpers for short term, and hopefully it is something we can just move to the source package when ready.

There is no real overhead to creating an additional resource file vs reusing existing one, I think?

jjonescz · Oct 8, 2025

src/Analyzers/CSharp/Analyzers/FileBasedPrograms/AppDirectiveDiagnosticAnalyzer.cs

+
+        // TODO: should SimpleDiagnostics have IDs? message args? TextSpan?
+        // It feels unreasonable for users to suppress these.
+        // When these are present, the user cannot build/run, period.


Yeah, currently we only have errors. We might want to add warnings in the future, but don't know about any candidates right now.

Future candidate is #r support. We've talked about enabling it for ease of conversion/pasting from CSX but marking it as a warning.

jjonescz · Oct 8, 2025

src/Analyzers/CSharp/Analyzers/FileBasedPrograms/AppDirectiveDiagnosticAnalyzer.cs

+        // When these are present, the user cannot build/run, period.
+        Diagnostic createDiagnostic(SyntaxTree syntaxTree, SimpleDiagnostic simpleDiagnostic)
+        {
+            const string FileBasedPrograms = nameof(FileBasedPrograms);


This looks unused.

jjonescz · Oct 8, 2025

src/Analyzers/CSharp/Analyzers/FileBasedPrograms/AppDirectiveHelpers.cs

+internal static class AppDirectiveHelpers
+{
+
+#pragma warning disable RSEXPERIMENTAL003 // 'SyntaxTokenParser' is experimental


I think SyntaxTokenParser is not experimental anymore (but in the sdk we don't have that new version of roslyn yet)

jjonescz · Oct 8, 2025

src/Analyzers/CSharp/Analyzers/FileBasedPrograms/AppDirectiveHelpers.cs

+                var message = trivia.GetStructure() is IgnoredDirectiveTriviaSyntax { Content: { RawKind: (int)SyntaxKind.StringLiteralToken } content }
+                    ? content.Text.AsSpan().Trim()
+                    : "";
+                // TODO: original impl was using more recent span-oriented APIs. Optimal sharing may be tricky.


Hm, that's sad that we need to target netstandard2.0, but it's probably fine, I don't expect the directive parsing to be perf critical (I just like to use modern APIs).

We also likely should ensure the source package is built against netstandard2.0 inside the sdk repo to verify the compatibility early.

jjonescz · Oct 8, 2025

src/Analyzers/CSharp/Analyzers/FileBasedPrograms/AppDirectiveHelpers.cs

+                    // TODO2: no Path.GetRelativePath on netstandard
+                    directiveText = PathUtilities.GetRelativePath(sourceDirectory, fullFilePath);
+                }
+                else if (!File.Exists(resolvedProjectPath))


Note that I'm extracting this IO access into a separate "evaluation" phase in dotnet/sdk#51108. So the IDE could in theory choose to skip this, but that doesn't sound very nice, as then the IDE wouldn't get the relevant #:project diagnostics.

jjonescz · Oct 8, 2025

src/Analyzers/CSharp/Analyzers/FileBasedPrograms/AppDirectiveHelpers.cs

+// See the LICENSE file in the project root for more information.
+
+// Largely copied from https://github.com/dotnet/sdk/blob/ce691a53dad7f2b6057c85c1e7fd7800dfe0788f/src/Cli/dotnet/Commands/Run/VirtualProjectBuildingCommand.cs#L1410
+// TODO: we'd like to extract a source package which makes this copy of the source unnecessary.


This looks good to me overall. Do you intend to create the source package yourself in the sdk, or should I go do and prioritize that?

Btw, I think this source package will be useful also if we move some of the virtual project building logic to msbuild as we discussed. And I plan to look at that next.

I'll try creating a source package this week, once next round of feedback here is dealt with, and if I get stumped by something I will reach out for help.

I think it is not too difficult to build a source package locally, and adjust my local Roslyn enlistment to consume the package, which would allow determining that the package is working for the intended purpose.

RikkiGibson

Submitting comments

RikkiGibson · Oct 8, 2025

src/Analyzers/CSharp/Analyzers/CSharpAnalyzersResources.resx

    <value>Property accessor can be simplified</value>
  </data>
+
+  <!-- TODO2: should this live here? Or sdk source package gives us some other resource file? -->


I'll try creating a new resource file next to AppDirectiveHelpers for short term, and hopefully it is something we can just move to the source package when ready.

There is no real overhead to creating an additional resource file vs reusing existing one, I think?

RikkiGibson · Oct 8, 2025

src/Analyzers/CSharp/Analyzers/FileBasedPrograms/AppDirectiveHelpers.cs

+        SourceFile sourceFile,
+        SyntaxTriviaList triviaList,
+        DiagnosticBag diagnostics,
+        ImmutableArray<CSharpDirective>.Builder builder)


note that analyzer has no need for this part. Possibly I will try to adjust factoring so that we don't need to create this, maybe make the builder nullable or something.

RikkiGibson · Oct 8, 2025

src/Analyzers/CSharp/Analyzers/FileBasedPrograms/AppDirectiveHelpers.cs

+                    // TODO2: no Path.GetRelativePath on netstandard
+                    directiveText = PathUtilities.GetRelativePath(sourceDirectory, fullFilePath);
+                }
+                else if (!File.Exists(resolvedProjectPath))


Once I get through some other feedback I will try this out in some scenarios where we also mess with the file system.

e.g. if we have a bad #:project ./path/to/project.csproj, and then we go create the project at that location on disk, what is the experience. Does just tabbing over to the editor make the error go away? Do you have to make edits? close/reopen the file? etc. As well as the opposite, deleting a csproj which is already present.

The thing is, creating and deleting csproj's is an infrequent enough event that, I feel like this analyzer can have a little file I/O, as a treat. Especially because the behavior doesn't depend on file contents

…diagnostics

github-actions bot added the Area-IDE label Oct 7, 2025

dotnet-policy-service bot added the VSCode label Oct 7, 2025

RikkiGibson commented Oct 7, 2025

View reviewed changes

jjonescz reviewed Oct 8, 2025

View reviewed changes

RikkiGibson commented Oct 8, 2025

View reviewed changes

File-based programs live directive diagnostics

e774d7a

RikkiGibson force-pushed the fbp-live-directive-diagnostics branch from 6233d9b to e774d7a Compare October 10, 2025 22:25

RikkiGibson added 2 commits October 16, 2025 10:52

Merge remote-tracking branch 'upstream/main' into fbp-live-directive-…

eadcdb1

…diagnostics

WIP

7c91f63

Search code, repositories, users, issues, pull requests...

Fbp live directive diagnostics #80575

Are you sure you want to change the base?

Fbp live directive diagnostics #80575

Conversation

RikkiGibson commented Oct 7, 2025

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!

jjonescz Oct 8, 2025 • edited by RikkiGibson 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!

jjonescz Oct 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!

RikkiGibson left a comment

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

jjonescz Oct 8, 2025 •

edited by RikkiGibson

Loading

jjonescz Oct 8, 2025 •

edited

Loading