-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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 1 commit
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 |
|---|---|---|
|
|
@@ -686,7 +686,7 @@ private DirectiveTriviaSyntax ParsePragmaDirective(SyntaxToken hash, SyntaxToken | |
|
|
||
| private DirectiveTriviaSyntax ParseShebangDirective(SyntaxToken hash, SyntaxToken exclamation, bool isActive) | ||
| { | ||
| if (lexer.Options.Kind != SourceCodeKind.Script && !lexer.Options.FileBasedProgram) | ||
| if (lexer.Options.Kind != SourceCodeKind.Script && !lexer.Options.AllowIgnoredDirectives) | ||
| { | ||
| exclamation = this.AddError(exclamation, ErrorCode.ERR_PPShebangInProjectBasedProgram); | ||
| } | ||
|
|
@@ -698,7 +698,7 @@ private DirectiveTriviaSyntax ParseIgnoredDirective(SyntaxToken hash, SyntaxToke | |
| { | ||
| if (isActive) | ||
| { | ||
| if (!lexer.Options.FileBasedProgram) | ||
| if (!lexer.Options.AllowIgnoredDirectives) | ||
|
Contributor
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. 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
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. AFAIK, this only controls whether an error is reported on usage.
Contributor
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. Not blocking this PR: should we update the spec or remove the condition? In other words, did we intentionally fork the language?
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. The spec says:
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.
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. Yes, reporting an error on
Contributor
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.
That behavior makes sense. I'll start an email thread. |
||
| { | ||
| colon = this.AddError(colon, ErrorCode.ERR_PPIgnoredNeedsFileBasedProgram); | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -14,6 +14,7 @@ | |
| using Microsoft.CodeAnalysis.Options; | ||
| using Microsoft.CodeAnalysis.Test.Utilities; | ||
| using Microsoft.CodeAnalysis.Text; | ||
| using Roslyn.Utilities; | ||
| using Xunit; | ||
|
|
||
| namespace Microsoft.CodeAnalysis.CSharp.UnitTests; | ||
|
|
@@ -100,6 +101,27 @@ public async Task PathRecommendation_02() | |
| await VerifyItemExistsAsync(markup, expectedItem: "Project.csproj"); | ||
| } | ||
|
|
||
| [Fact] | ||
| public async Task PathRecommendation_03() | ||
| { | ||
| // Test a virtual file scenario (e.g. ctrl+N in VS Code or other cases where there is not an actual file on disk.) | ||
| var code = """ | ||
| #:project $$ | ||
| """; | ||
|
|
||
| var markup = $""" | ||
| <Workspace> | ||
| <Project Language="C#" CommonReferences="true" AssemblyName="Test1" Features="FileBasedProgram=true"> | ||
|
Contributor
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.
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. 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.
Contributor
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. 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?
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. Resolving by deleting MiscellaneousFile flag from the compiler layer. (i.e. reverting all compiler changes in this PR.) |
||
| <Document FilePath="Untitled-1" ResolveFilePath="false"><![CDATA[{code}]]></Document> | ||
| </Project> | ||
| </Workspace> | ||
| """; | ||
|
|
||
| // In this case, only stuff like drive roots would be recommended. | ||
| var expectedRoot = PlatformInformation.IsWindows ? @"C:" : "/"; | ||
|
RikkiGibson marked this conversation as resolved.
Outdated
RikkiGibson marked this conversation as resolved.
Outdated
|
||
| await VerifyItemExistsAsync(markup, expectedRoot); | ||
| } | ||
|
|
||
| // Note: The editor uses a shared mechanism to filter out completion items which don't match the prefix of what the user is typing. | ||
| // Therefore we do not have "negative tests" here for file names. | ||
| } | ||
|
|
||
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.
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?
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.
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.