-
Notifications
You must be signed in to change notification settings - Fork 4.2k
Do not report parse errors for '#:' in miscellaneous files #81385
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 all commits
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 |
|---|---|---|
|
|
@@ -45,19 +45,20 @@ internal static ProjectInfo CreateMiscellaneousProjectInfoForDocument( | |
| // https://devdiv.visualstudio.com/DevDiv/_workitems/edit/575761 | ||
| var parseOptions = languageServices.GetService<ISyntaxTreeFactoryService>()?.GetDefaultParseOptionsWithLatestLanguageVersion(); | ||
|
|
||
| if (parseOptions != null && | ||
| compilationOptions != null && | ||
| languageInformation.ScriptExtension is not null && | ||
| fileExtension == languageInformation.ScriptExtension) | ||
| if (parseOptions != null) | ||
| { | ||
| parseOptions = parseOptions.WithKind(SourceCodeKind.Script); | ||
| compilationOptions = GetCompilationOptionsWithScriptReferenceResolvers(services, compilationOptions, filePath); | ||
| } | ||
|
|
||
| if (parseOptions != null && languageInformation.ScriptExtension is not null && fileExtension != languageInformation.ScriptExtension) | ||
| { | ||
| // Any non-script misc file should not complain about usage of '#:' ignored directives. | ||
| parseOptions = parseOptions.WithFeatures([.. parseOptions.Features, new("FileBasedProgram", "true")]); | ||
| if (compilationOptions != null && | ||
| languageInformation.ScriptExtension is not null && | ||
| fileExtension == languageInformation.ScriptExtension) | ||
| { | ||
| parseOptions = parseOptions.WithKind(SourceCodeKind.Script); | ||
| compilationOptions = GetCompilationOptionsWithScriptReferenceResolvers(services, compilationOptions, filePath); | ||
| } | ||
| else | ||
| { | ||
| // Any non-script misc file should not complain about usage of '#:' ignored directives. | ||
| parseOptions = parseOptions.WithFeatures([.. parseOptions.Features, new("FileBasedProgram", "true")]); | ||
|
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 change makes it so if there are parse options, then we are either going to treat this as a script, or we'll pass the FileBasedProgram flag (to avoid reporting possibly unwanted errors for |
||
| } | ||
| } | ||
|
|
||
| var projectId = ProjectId.CreateNewId(debugName: $"{workspace.GetType().Name} Files Project for {filePath}"); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -4,7 +4,6 @@ | |
|
|
||
| using Microsoft.CodeAnalysis.LanguageServer.HostWorkspace; | ||
| using Microsoft.CodeAnalysis.LanguageServer.UnitTests.Miscellaneous; | ||
| using Microsoft.CodeAnalysis.Options; | ||
| using Microsoft.CodeAnalysis.Shared.Extensions; | ||
| using Microsoft.CodeAnalysis.Shared.TestHooks; | ||
| using Microsoft.CodeAnalysis.Test.Utilities; | ||
|
|
@@ -146,13 +145,9 @@ await testLspServer.OpenDocumentAsync(nonFileUri, """ | |
| Assert.Equal(2, primordialDocument.Project.Documents.Count()); | ||
| Assert.Empty(primordialDocument.Project.MetadataReferences); | ||
|
|
||
| // No errors for '#:' are expected. | ||
| var primordialSyntaxTree = await primordialDocument.GetRequiredSyntaxTreeAsync(CancellationToken.None); | ||
| // TODO: we probably don't want to report syntax errors for '#:' in the primordial non-file document. | ||
| // The logic which decides whether to add '-features:FileBasedProgram' probably needs to be adjusted. | ||
| primordialSyntaxTree.GetDiagnostics(CancellationToken.None).Verify( | ||
| // vscode-notebook-cell://dev-container/test.cs(1,2): error CS9298: '#:' directives can be only used in file-based programs ('-features:FileBasedProgram')" | ||
| // #:sdk Microsoft.Net.Sdk | ||
| TestHelpers.Diagnostic(code: 9298, squiggledText: ":").WithLocation(1, 2)); | ||
| Assert.Empty(primordialSyntaxTree.GetDiagnostics(CancellationToken.None)); | ||
|
|
||
| // Wait for the canonical project to finish loading. | ||
| await testLspServer.TestWorkspace.GetService<AsynchronousOperationListenerProvider>().GetWaiter(FeatureAttribute.Workspace).ExpeditedWaitAsync(); | ||
|
|
@@ -165,13 +160,47 @@ await testLspServer.OpenDocumentAsync(nonFileUri, """ | |
| // Should have the appropriate generated files now that we ran a design time build | ||
| Assert.Contains(canonicalDocument.Project.Documents, d => d.Name == "Canonical.AssemblyInfo.cs"); | ||
|
|
||
| // No errors for '#:' are expected. | ||
| var canonicalSyntaxTree = await canonicalDocument.GetRequiredSyntaxTreeAsync(CancellationToken.None); | ||
| // TODO: we probably don't want to report syntax errors for '#:' in the canonical non-file document. | ||
| // The logic which decides whether to add '-features:FileBasedProgram' probably needs to be adjusted. | ||
| canonicalSyntaxTree.GetDiagnostics(CancellationToken.None).Verify( | ||
| // vscode-notebook-cell://dev-container/test.cs(1,2): error CS9298: '#:' directives can be only used in file-based programs ('-features:FileBasedProgram')" | ||
| Assert.Empty(canonicalSyntaxTree.GetDiagnostics(CancellationToken.None)); | ||
| } | ||
|
|
||
| [Theory, CombinatorialData] | ||
| public async Task TestScriptsWithIgnoredDirectives(bool mutatingLspWorkspace) | ||
| { | ||
| // https://github.com/dotnet/roslyn/issues/81644: A csx script with '#:' directives should not be loaded as a file-based program | ||
| await using var testLspServer = await CreateTestLspServerAsync(string.Empty, mutatingLspWorkspace, new InitializationOptions { ServerKind = WellKnownLspServerKinds.CSharpVisualBasicLspServer }); | ||
| Assert.Null(await GetMiscellaneousDocumentAsync(testLspServer)); | ||
|
|
||
| var nonFileUri = ProtocolConversions.CreateAbsoluteDocumentUri(@"C:\script.csx"); | ||
| await testLspServer.OpenDocumentAsync(nonFileUri, """ | ||
| #:sdk Microsoft.Net.Sdk | ||
| Console.WriteLine("Hello World"); | ||
| """).ConfigureAwait(false); | ||
|
|
||
| // File is added to a miscellaneous project containing only the script. | ||
| var (_, primordialDocument) = await GetRequiredLspWorkspaceAndDocumentAsync(nonFileUri, testLspServer).ConfigureAwait(false); | ||
| Assert.Equal(1, primordialDocument.Project.Documents.Count()); | ||
| Assert.Empty(primordialDocument.Project.MetadataReferences); | ||
|
|
||
| // FileBasedProgram feature flag is not passed, so an error is expected on '#:'. | ||
| var primordialSyntaxTree = await primordialDocument.GetRequiredSyntaxTreeAsync(CancellationToken.None); | ||
| primordialSyntaxTree.GetDiagnostics(CancellationToken.None).Verify( | ||
| // script.csx(1,2): error CS9298: '#:' directives can be only used in file-based programs ('-features:FileBasedProgram')" | ||
|
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 wouldn't want to report this error for a misc file like |
||
| // #:sdk Microsoft.Net.Sdk | ||
| TestHelpers.Diagnostic(code: 9298, squiggledText: ":").WithLocation(1, 2)); | ||
|
|
||
| // Wait for project load to finish | ||
| await testLspServer.TestWorkspace.GetService<AsynchronousOperationListenerProvider>().GetWaiter(FeatureAttribute.Workspace).ExpeditedWaitAsync(); | ||
|
|
||
| var (miscWorkspace, canonicalDocument) = await GetRequiredLspWorkspaceAndDocumentAsync(nonFileUri, testLspServer).ConfigureAwait(false); | ||
| Assert.Equal(WorkspaceKind.Host, miscWorkspace.Kind); | ||
| Assert.NotNull(canonicalDocument); | ||
| Assert.NotEqual(primordialDocument, canonicalDocument); | ||
| Assert.Contains(canonicalDocument.Project.Documents, d => d.Name == "script.AssemblyInfo.cs"); | ||
|
|
||
| var canonicalSyntaxTree = await canonicalDocument.GetRequiredSyntaxTreeAsync(CancellationToken.None); | ||
| Assert.Empty(canonicalSyntaxTree.GetDiagnostics(CancellationToken.None)); | ||
| } | ||
|
|
||
| [Theory, CombinatorialData] | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.
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.
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. #Closed
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.
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.
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.
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?
Uh oh!
There was an error while loading. Please reload this page.
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.
Resolving by deleting MiscellaneousFile flag from the compiler layer. (i.e. reverting all compiler changes in this PR.)