File-based apps: ensure shebang analyzer only applies to #:include - #54397
Conversation
There was a problem hiding this comment.
Pull request overview
This PR narrows the CA2266 (“missing shebang in file-based program”) behavior so it only triggers when additional C# source files are brought in via #:include, avoiding warnings for multi-file compilations caused by other MSBuild mechanisms (e.g., Directory.Build.props, project references, etc.).
Changes:
- Emit compiler-visible item metadata in the generated virtual project so analyzers can distinguish
#:include-addedCompileitems. - Update the CA2266 analyzer logic to only warn when a non-entrypoint syntax tree is marked as coming from
#:include. - Adjust/extend CLI and analyzer unit tests to validate the new behavior and the generated project content.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| test/dotnet.Tests/CommandTests/Run/RunFileTests_Directives.cs | Updates API-based virtual project assertions to require the new compiler-visible metadata declaration. |
| test/dotnet.Tests/CommandTests/Run/RunFileTests_CscOnlyAndApi.cs | Updates expected virtual project XML in CscOnly/API tests to include the compiler-visible metadata item. |
| test/dotnet.Tests/CommandTests/Run/RunFileTests_BuildOptions.cs | Updates CA2266 expectations to ensure no warning from extra Compile items added via Directory.Build.props, while still warning for real #:include. |
| src/Microsoft.DotNet.ProjectTools/VirtualProjectBuilder.cs | Adds <CompilerVisibleItemMetadata ...> so FileBasedProgramsFromIncludeDirective flows into analyzer config as build_metadata.Compile.*. |
| src/Microsoft.CodeAnalysis.NetAnalyzers/tests/.../MissingShebangInFileBasedProgramTests.cs | Updates/extends analyzer tests to cover “include vs non-include extra file” behavior. |
| src/Microsoft.CodeAnalysis.NetAnalyzers/src/.../CSharpMissingShebangInFileBasedProgram.cs | Changes CA2266 triggering condition to require presence of build_metadata.Compile.FileBasedProgramsFromIncludeDirective=true on at least one non-entrypoint tree. |
|
@RikkiGibson @333fred for reviews, thanks |
|
@RikkiGibson for another review, thanks |
| }); | ||
| } | ||
|
|
||
| private static bool IsFromIncludeDirective(SyntaxTree tree, AnalyzerConfigOptionsProvider optionsProvider) |
There was a problem hiding this comment.
Since we now only want to detect when an #:include happens to be the mechanism including the other file, did you consider making this a syntactic check instead? i.e. just look for an ignored directive whose content starts with include ?
There was a problem hiding this comment.
Perhaps we want to preserve the ability to #:include Config/* and similar, which won't expand into anything that includes .cs, and not tell user to add #! in that case? If so, is there a test for that scenario? I didn't see one from a brief search.
There was a problem hiding this comment.
did you consider making this a syntactic check instead? i.e. just look for an ignored directive whose content starts with
include?
That's an interesting idea, I did not consider that.
Perhaps we want to preserve the ability to
#:include Config/*and similar, which won't expand into anything that includes.cs, and not tell user to add#!in that case?
That really depends on IDE, but yes, I think y'all don't care about non-.cs included files?
If that's indeed the case, I think this cannot be a syntactic check. Even though the #:include directives must have an extension, it can be dynamically mapped (or even need expansion from msbuild properties), so it couldn't be detected from syntax only.
But perhaps we don't need to be that precise (and complex) and we can warn for any #:include really, even if IDE would work in cases where only non-.cs files are included.
If so, is there a test for that scenario? I didn't see one from a brief search.
I will add one, thanks.
There was a problem hiding this comment.
But perhaps we don't need to be that precise (and complex) and we can warn for any
#:includereally
I like the simplicity of this, I'm going to implement that.
There was a problem hiding this comment.
That really depends on IDE, but yes, I think y'all don't care about non-.cs included files?
Not 100% sure whether *.cs is the only extension IDE cares about.
Are people going to do #:include Views/*.razor or similar, and expect to get semantic info in the razor files which relates to the rest of the project?
I think erring on the side of making people add #!, even though we strictly could handle the #:include without it, sounds good.
| var filePath = tree.FilePath; | ||
| if (!string.IsNullOrEmpty(filePath)) | ||
| var text = trivia.ToString().AsSpan().TrimStart(); | ||
| if (!text.StartsWith(hashColon, StringComparison.Ordinal)) |
There was a problem hiding this comment.
It seemed slightly surprising to me, to not dig out the IgnoredDirectiveTrivia here and look at the items on that. Maybe those APIs aren't guaranteed to be available here?
There was a problem hiding this comment.
Good point, will fix, thanks.
|
/backport to release/10.0.3xx |
|
Started backporting to |
|
@jjonescz backporting to git am output$ git am --3way --empty=keep --ignore-whitespace --keep-non-patch changes.patch
Applying: File-based apps: ensure shebang analyzer only applies to `#:include`
Using index info to reconstruct a base tree...
M src/Microsoft.CodeAnalysis.NetAnalyzers/tests/Microsoft.CodeAnalysis.NetAnalyzers.UnitTests/Microsoft.NetCore.Analyzers/Usage/MissingShebangInFileBasedProgramTests.cs
M src/Microsoft.DotNet.ProjectTools/VirtualProjectBuilder.cs
A test/dotnet.Tests/CommandTests/Run/RunFileTests_BuildOptions.cs
A test/dotnet.Tests/CommandTests/Run/RunFileTests_CscOnlyAndApi.cs
A test/dotnet.Tests/CommandTests/Run/RunFileTests_Directives.cs
Falling back to patching base and 3-way merge...
Auto-merging src/Microsoft.CodeAnalysis.NetAnalyzers/tests/Microsoft.CodeAnalysis.NetAnalyzers.UnitTests/Microsoft.NetCore.Analyzers/Usage/MissingShebangInFileBasedProgramTests.cs
Auto-merging src/Microsoft.DotNet.ProjectTools/VirtualProjectBuilder.cs
CONFLICT (modify/delete): test/dotnet.Tests/CommandTests/Run/RunFileTests_BuildOptions.cs deleted in HEAD and modified in File-based apps: ensure shebang analyzer only applies to `#:include`. Version File-based apps: ensure shebang analyzer only applies to `#:include` of test/dotnet.Tests/CommandTests/Run/RunFileTests_BuildOptions.cs left in tree.
CONFLICT (modify/delete): test/dotnet.Tests/CommandTests/Run/RunFileTests_CscOnlyAndApi.cs deleted in HEAD and modified in File-based apps: ensure shebang analyzer only applies to `#:include`. Version File-based apps: ensure shebang analyzer only applies to `#:include` of test/dotnet.Tests/CommandTests/Run/RunFileTests_CscOnlyAndApi.cs left in tree.
CONFLICT (modify/delete): test/dotnet.Tests/CommandTests/Run/RunFileTests_Directives.cs deleted in HEAD and modified in File-based apps: ensure shebang analyzer only applies to `#:include`. Version File-based apps: ensure shebang analyzer only applies to `#:include` of test/dotnet.Tests/CommandTests/Run/RunFileTests_Directives.cs left in tree.
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
hint: When you have resolved this problem, run "git am --continue".
hint: If you prefer to skip this patch, run "git am --skip" instead.
hint: To restore the original branch and stop patching, run "git am --abort".
hint: Disable this message with "git config set advice.mergeConflict false"
Patch failed at 0001 File-based apps: ensure shebang analyzer only applies to `#:include`
Error: The process '/usr/bin/git' failed with exit code 128 |
To avoid breaking people that include
.csfiles via other means. See #53749 (comment).