-
Notifications
You must be signed in to change notification settings - Fork 4.2k
File-based programs live directive diagnostics #80575
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 22 commits
e774d7a
eadcdb1
7c91f63
956bd15
92b7303
41db780
231a977
ba0f21d
d918140
494668e
8c11417
a6a7009
37f351e
e23b8d0
47268e0
c341c47
0298b82
90ffbdf
b43a560
4a036e9
334478c
98a1041
9800e7b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,22 @@ | ||
| // Licensed to the .NET Foundation under one or more agreements. | ||
| // The .NET Foundation licenses this file to you under the MIT license. | ||
| // See the LICENSE file in the project root for more information. | ||
|
|
||
| #if !NET | ||
| using Roslyn.Utilities; | ||
|
|
||
| namespace Microsoft.DotNet.FileBasedPrograms; | ||
|
|
||
| /// <summary>Provides implementations of certain methods to the FileBasedPrograms source package.</summary> | ||
| internal partial class ExternalHelpers | ||
| { | ||
| public static partial int CombineHashCodes(int value1, int value2) | ||
RikkiGibson marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| => Hash.Combine(value1, value2); | ||
|
|
||
| public static partial string GetRelativePath(string relativeTo, string path) | ||
| => PathUtilities.GetRelativePath(relativeTo, path); | ||
|
|
||
| public static partial bool IsPathFullyQualified(string path) | ||
| => PathUtilities.IsAbsolute(path); | ||
| } | ||
| #endif | ||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Note that the analyzer is included under this path, because we only expect/want it to run in the editor. Running it on the command line would just be redundant with what It wouldn't cause a problem if it did somehow run on the command line, though, because in general, in a scenario where this analyzer would report an error, |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,68 @@ | ||
| // Licensed to the .NET Foundation under one or more agreements. | ||
| // The .NET Foundation licenses this file to you under the MIT license. | ||
| // See the LICENSE file in the project root for more information. | ||
|
|
||
| using Microsoft.CodeAnalysis.CodeQuality; | ||
| using Microsoft.CodeAnalysis.CodeStyle; | ||
| using Microsoft.CodeAnalysis.Diagnostics; | ||
| using Microsoft.DotNet.FileBasedPrograms; | ||
|
|
||
| namespace Microsoft.CodeAnalysis.FileBasedPrograms; | ||
|
|
||
| [DiagnosticAnalyzer(LanguageNames.CSharp)] | ||
| internal sealed class FileLevelDirectiveDiagnosticAnalyzer() | ||
| : AbstractCodeQualityDiagnosticAnalyzer( | ||
| descriptors: [s_descriptor], | ||
| generatedCodeAnalysisFlags: GeneratedCodeAnalysisFlags.Analyze | GeneratedCodeAnalysisFlags.ReportDiagnostics) | ||
| { | ||
| public const string DiagnosticId = "FileBasedPrograms"; | ||
|
|
||
| private static readonly DiagnosticDescriptor s_descriptor = CreateDescriptor( | ||
| id: DiagnosticId, | ||
| enforceOnBuild: EnforceOnBuild.Never, | ||
| title: DiagnosticId, | ||
| messageFormat: "{0}", | ||
| hasAnyCodeStyleOption: false, | ||
| isUnnecessary: false, | ||
| isEnabledByDefault: true, | ||
| isConfigurable: false, | ||
| defaultSeverity: DiagnosticSeverity.Error, | ||
| helpLinkUri: "https://learn.microsoft.com/dotnet/csharp/language-reference/preprocessor-directives#file-based-apps"); | ||
|
|
||
| public override DiagnosticAnalyzerCategory GetAnalyzerCategory() | ||
| => DiagnosticAnalyzerCategory.SyntaxTreeWithoutSemanticsAnalysis; | ||
|
|
||
| protected override void InitializeWorker(AnalysisContext context) | ||
| { | ||
| context.RegisterSyntaxTreeAction(context => | ||
| { | ||
| var cancellationToken = context.CancellationToken; | ||
| var tree = context.Tree; | ||
| if (!tree.Options.Features.ContainsKey("FileBasedProgram")) | ||
| return; | ||
|
|
||
| var root = tree.GetRoot(cancellationToken); | ||
| if (!root.ContainsDirectives) | ||
| return; | ||
|
|
||
| // The compiler already reports an error on all the directives past the first token in the file. | ||
| // Therefore, the analyzer only deals with the directives on the first token. | ||
| // Console.WriteLine("Hello World!"); | ||
| // #:property foo=bar // error CS9297: '#:' directives cannot be after first token in file | ||
| var diagnosticBag = DiagnosticBag.Collect(out var diagnosticsBuilder); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this type is unfamiliar to me. need to figure out what that is about.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
| FileLevelDirectiveHelpers.FindLeadingDirectives( | ||
| new SourceFile(tree.FilePath, tree.GetText(cancellationToken)), | ||
| root.GetLeadingTrivia(), | ||
| diagnosticBag, | ||
| builder: null); | ||
|
|
||
| foreach (var simpleDiagnostic in diagnosticsBuilder) | ||
| { | ||
| context.ReportDiagnostic(Diagnostic.Create( | ||
| s_descriptor, | ||
| location: Location.Create(tree, simpleDiagnostic.Location.TextSpan), | ||
| simpleDiagnostic.Message)); | ||
| } | ||
| }); | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,3 @@ | ||
| [*.cs] | ||
| dotnet_diagnostic.RS0051.severity = none # Symbol is not part of the declared API | ||
| dotnet_diagnostic.RS0056.severity = none # InternalAPI.txt is missing '#nullable enable', so the nullability annotations of API isn't recorded. It is recommended to enable this tracking. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -67,7 +67,15 @@ | |
| </ItemGroup> | ||
| <ItemGroup> | ||
| <PackageReference Include="Humanizer.Core" PrivateAssets="compile" /> | ||
| <PackageReference Include="Microsoft.DotNet.FileBasedPrograms" GeneratePathProperty="true" /> | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Does this need to be
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
| <EmbeddedResource | ||
| Include="$(PkgMicrosoft_DotNet_FileBasedPrograms)\contentFiles\cs\any\FileBasedProgramsResources.resx" | ||
| GenerateSource="true" | ||
| Namespace="Microsoft.DotNet.FileBasedPrograms" /> | ||
| </ItemGroup> | ||
| <PropertyGroup> | ||
| <DefineConstants>$(DefineConstants);FILE_BASED_PROGRAMS_SOURCE_PACKAGE_GRACEFUL_EXCEPTION</DefineConstants> | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. what is this for?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
This symbol causes the source package to declare type |
||
| </PropertyGroup> | ||
| <Import Project="..\..\..\Analyzers\CSharp\Analyzers\CSharpAnalyzers.projitems" Label="Shared" /> | ||
| <Import Project="..\..\..\Analyzers\CSharp\CodeFixes\CSharpCodeFixes.projitems" Label="Shared" /> | ||
| <Import Project="..\..\..\Compilers\CSharp\CSharpAnalyzerDriver\CSharpAnalyzerDriver.projitems" Label="Shared" /> | ||
|
|
||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ripping out all the bits that were reporting "build only diagnostics" for |
This file was deleted.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -22,63 +22,9 @@ namespace Microsoft.CodeAnalysis.LanguageServer.FileBasedPrograms; | |
| [Export(typeof(VirtualProjectXmlProvider)), Shared] | ||
| [method: ImportingConstructor] | ||
| [method: Obsolete(MefConstruction.ImportingConstructorMessage, error: true)] | ||
| internal class VirtualProjectXmlProvider(IDiagnosticsRefresher diagnosticRefresher, DotnetCliHelper dotnetCliHelper) | ||
| internal class VirtualProjectXmlProvider(DotnetCliHelper dotnetCliHelper) | ||
| { | ||
| private readonly SemaphoreSlim _gate = new(initialCount: 1); | ||
| private readonly Dictionary<string, ImmutableArray<SimpleDiagnostic>> _diagnosticsByFilePath = []; | ||
|
|
||
| internal async ValueTask<ImmutableArray<SimpleDiagnostic>> GetCachedDiagnosticsAsync(string path, CancellationToken cancellationToken) | ||
| { | ||
| using (await _gate.DisposableWaitAsync(cancellationToken)) | ||
| { | ||
| _diagnosticsByFilePath.TryGetValue(path, out var diagnostics); | ||
| return diagnostics; | ||
| } | ||
| } | ||
|
|
||
| internal async ValueTask UnloadCachedDiagnosticsAsync(string path) | ||
| { | ||
| using (await _gate.DisposableWaitAsync(CancellationToken.None)) | ||
| { | ||
| _diagnosticsByFilePath.Remove(path); | ||
| } | ||
| } | ||
|
|
||
| internal async Task<(string VirtualProjectXml, ImmutableArray<SimpleDiagnostic> Diagnostics)?> GetVirtualProjectContentAsync(string documentFilePath, ILogger logger, CancellationToken cancellationToken) | ||
| { | ||
| var result = await GetVirtualProjectContentImplAsync(documentFilePath, logger, cancellationToken); | ||
| if (result is { } project) | ||
| { | ||
| using (await _gate.DisposableWaitAsync(cancellationToken)) | ||
| { | ||
| _diagnosticsByFilePath.TryGetValue(documentFilePath, out var previousCachedDiagnostics); | ||
| _diagnosticsByFilePath[documentFilePath] = project.Diagnostics; | ||
|
|
||
| // check for difference, and signal to host to update if so. | ||
| if (previousCachedDiagnostics.IsDefault || !project.Diagnostics.SequenceEqual(previousCachedDiagnostics)) | ||
| diagnosticRefresher.RequestWorkspaceRefresh(); | ||
| } | ||
| } | ||
| else | ||
| { | ||
| using (await _gate.DisposableWaitAsync(CancellationToken.None)) | ||
| { | ||
| if (_diagnosticsByFilePath.TryGetValue(documentFilePath, out var diagnostics)) | ||
| { | ||
| _diagnosticsByFilePath.Remove(documentFilePath); | ||
| if (!diagnostics.IsDefaultOrEmpty) | ||
| { | ||
| // diagnostics have changed from "non-empty" to "unloaded". refresh. | ||
| diagnosticRefresher.RequestWorkspaceRefresh(); | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| return result; | ||
| } | ||
|
|
||
| private async Task<(string VirtualProjectXml, ImmutableArray<SimpleDiagnostic> Diagnostics)?> GetVirtualProjectContentImplAsync(string documentFilePath, ILogger logger, CancellationToken cancellationToken) | ||
| { | ||
| var workingDirectory = Path.GetDirectoryName(documentFilePath); | ||
| var process = dotnetCliHelper.Run(["run-api"], workingDirectory, shouldLocalizeOutput: true, keepStandardInputOpen: true); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why do we still need to call run-api?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We would want to add a method to source package for getting a virtual project and use it here before getting rid of that.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Right, so we only wanted the source package for live diagnostics to be visible even without saving the file (which would not be easily possible with run-api). For other things, run-api still works fine. Is that correct?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes. I think it's a good idea to expand the source package, and use it to replace the task being done by this method, though.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. +1 to using source package instead of calling the run-api. |
||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can probably setup a subscription so that we get pushed updates for this package automatically, but, want to start by just taking the version that was pushed to the feed yesterday.