-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Warn for missing shebang #53614
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
Warn for missing shebang #53614
Changes from all commits
0fe80a0
a3d03da
3e41a04
75365b8
f924d63
e44e29d
e9f6254
39f4c95
162a08a
e5e3765
df349de
4ae6b42
8f30601
481d4ed
54e9b4f
1657410
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,42 @@ | ||
| // Copyright (c) Microsoft. All Rights Reserved. Licensed under the MIT license. See License.txt in the project root for license information. | ||
|
|
||
| using System.Composition; | ||
| using Microsoft.CodeAnalysis; | ||
| using Microsoft.CodeAnalysis.CodeActions; | ||
| using Microsoft.CodeAnalysis.CodeFixes; | ||
| using Microsoft.CodeAnalysis.CSharp; | ||
| using Microsoft.CodeAnalysis.Formatting; | ||
| using Microsoft.NetCore.Analyzers; | ||
| using Microsoft.NetCore.Analyzers.Usage; | ||
|
|
||
| namespace Microsoft.NetCore.CSharp.Analyzers.Usage | ||
| { | ||
| [ExportCodeFixProvider(LanguageNames.CSharp), Shared] | ||
| public sealed class CSharpMissingShebangInFileBasedProgramFixer : MissingShebangInFileBasedProgramFixer | ||
| { | ||
| public override async Task RegisterCodeFixesAsync(CodeFixContext context) | ||
| { | ||
| var root = await context.Document.GetSyntaxRootAsync(context.CancellationToken).ConfigureAwait(false); | ||
| if (root is null) | ||
| { | ||
| return; | ||
| } | ||
|
|
||
| var codeAction = CodeAction.Create( | ||
| MicrosoftNetCoreAnalyzersResources.MissingShebangInFileBasedProgramCodeFixTitle, | ||
| async ct => | ||
| { | ||
| var options = await context.Document.GetOptionsAsync(ct).ConfigureAwait(false); | ||
| var eol = options.GetOption(FormattingOptions.NewLine, LanguageNames.CSharp); | ||
| var shebangTrivia = SyntaxFactory.ParseLeadingTrivia("#!/usr/bin/env dotnet" + eol); | ||
| var firstToken = root.GetFirstToken(includeZeroWidth: true); | ||
| var newFirstToken = firstToken.WithLeadingTrivia(shebangTrivia.AddRange(firstToken.LeadingTrivia)); | ||
|
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. Where should this go relative to copyright headers?
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.
|
||
| var newRoot = root.ReplaceToken(firstToken, newFirstToken) | ||
| .WithAdditionalAnnotations(Formatter.Annotation); | ||
| return context.Document.WithSyntaxRoot(newRoot); | ||
| }, | ||
| MicrosoftNetCoreAnalyzersResources.MissingShebangInFileBasedProgramCodeFixTitle); | ||
| context.RegisterCodeFix(codeAction, context.Diagnostics); | ||
| } | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,131 @@ | ||
| // Copyright (c) Microsoft. All Rights Reserved. Licensed under the MIT license. See License.txt in the project root for license information. | ||
|
|
||
| using Analyzer.Utilities; | ||
| using Analyzer.Utilities.Extensions; | ||
| using Microsoft.CodeAnalysis; | ||
| using Microsoft.CodeAnalysis.CSharp; | ||
| using Microsoft.CodeAnalysis.Diagnostics; | ||
| using Microsoft.NetCore.Analyzers.Usage; | ||
|
|
||
| namespace Microsoft.NetCore.CSharp.Analyzers.Usage | ||
| { | ||
| [DiagnosticAnalyzer(LanguageNames.CSharp)] | ||
| public sealed class CSharpMissingShebangInFileBasedProgram : MissingShebangInFileBasedProgram | ||
| { | ||
| public override void Initialize(AnalysisContext context) | ||
| { | ||
| context.EnableConcurrentExecution(); | ||
| context.ConfigureGeneratedCodeAnalysis(GeneratedCodeAnalysisFlags.None); | ||
|
|
||
| context.RegisterCompilationStartAction(context => | ||
| { | ||
| var entryPointFilePath = context.Options.GetMSBuildPropertyValue( | ||
| MSBuildPropertyOptionNames.EntryPointFilePath, context.Compilation); | ||
| if (string.IsNullOrEmpty(entryPointFilePath)) | ||
| { | ||
| return; | ||
| } | ||
|
|
||
| // Count non-generated trees upfront so we can report directly | ||
| // from a SyntaxTreeAction without needing CompilationEnd. | ||
| // We avoid CompilationEnd so diagnostics appear as live IDE diagnostics. | ||
| // We replicate Roslyn's generated code detection here because | ||
| // Compilation.SyntaxTrees is the raw set (unlike RegisterSyntaxTreeAction | ||
| // which gets automatic filtering via ConfigureGeneratedCodeAnalysis). | ||
| int nonGeneratedTreeCount = 0; | ||
| foreach (var tree in context.Compilation.SyntaxTrees) | ||
| { | ||
| if (IsGeneratedCode(tree, context.Options.AnalyzerConfigOptionsProvider)) | ||
| { | ||
| continue; | ||
| } | ||
|
|
||
| nonGeneratedTreeCount++; | ||
| } | ||
|
|
||
| // Only report when there are multiple non-generated files | ||
| // (i.e., #:include directives are used). | ||
|
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. I guess this has the effect of also detecting when a Directory.Build.props adds a compile item? Is that right?
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. I think if it is a goal to handle that case, then, it should have a test |
||
| // Single-file programs don't need a shebang to distinguish the entry point. | ||
| if (nonGeneratedTreeCount <= 1) | ||
| { | ||
| return; | ||
| } | ||
|
|
||
| context.RegisterSyntaxTreeAction(context => | ||
| { | ||
| if (!context.Tree.FilePath.Equals(entryPointFilePath, StringComparison.Ordinal)) | ||
| { | ||
| return; | ||
| } | ||
|
|
||
| var root = context.Tree.GetRoot(context.CancellationToken); | ||
| if (root.GetLeadingTrivia().Any(SyntaxKind.ShebangDirectiveTrivia)) | ||
|
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. Are we counting on the fact that if the
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'll add a test to demonstrate. |
||
| { | ||
| return; | ||
| } | ||
|
|
||
| var location = root.GetFirstToken(includeZeroWidth: true).GetLocation(); | ||
| context.ReportDiagnostic(location.CreateDiagnostic(Rule)); | ||
| }); | ||
| }); | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// Replicates Roslyn's generated code detection which checks: | ||
| /// the <c>generated_code</c> analyzer config option, | ||
| /// common file name patterns, and <c><auto-generated></c> comment headers. | ||
| /// Based on <see href="https://github.com/dotnet/roslyn/blob/0504782ef845507260874f2efc253b36d1775685/src/Compilers/Core/Portable/SourceGeneration/GeneratedCodeUtilities.cs">GeneratedCodeUtilities</see>. | ||
| /// </summary> | ||
| private static bool IsGeneratedCode(SyntaxTree tree, AnalyzerConfigOptionsProvider optionsProvider) | ||
|
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. I don't like hardcoding this knowledge here, but don't know how to better do this.
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. At first I thought: just indicate that the analyzer shouldn't run on generated code, and see if you get called back for a given syntax tree or not. But I think that doesn't work, since it would need the "compilation end" callback to run. I think copying the implementation like you did is the best we can do for now.
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. I would recommend adding a permalink here to the location in Roslyn this was based on though. |
||
| { | ||
| if (optionsProvider.GetOptions(tree) | ||
| .TryGetValue("generated_code", out var generatedValue) && | ||
| generatedValue.Equals("true", StringComparison.OrdinalIgnoreCase)) | ||
| { | ||
| return true; | ||
| } | ||
|
|
||
| var filePath = tree.FilePath; | ||
| if (!string.IsNullOrEmpty(filePath)) | ||
| { | ||
| var fileName = Path.GetFileName(filePath); | ||
| if (fileName.StartsWith("TemporaryGeneratedFile_", StringComparison.OrdinalIgnoreCase)) | ||
| { | ||
| return true; | ||
| } | ||
|
|
||
| var nameWithoutExtension = Path.GetFileNameWithoutExtension(fileName); | ||
| if (nameWithoutExtension.EndsWith(".designer", StringComparison.OrdinalIgnoreCase) || | ||
| nameWithoutExtension.EndsWith(".generated", StringComparison.OrdinalIgnoreCase) || | ||
| nameWithoutExtension.EndsWith(".g", StringComparison.OrdinalIgnoreCase) || | ||
| nameWithoutExtension.EndsWith(".g.i", StringComparison.OrdinalIgnoreCase)) | ||
| { | ||
| return true; | ||
| } | ||
| } | ||
|
|
||
| // Check for <auto-generated> or <autogenerated> comment at the top of the file. | ||
| foreach (var trivia in tree.GetRoot().GetLeadingTrivia()) | ||
| { | ||
| switch (trivia.Kind()) | ||
| { | ||
| case SyntaxKind.SingleLineCommentTrivia: | ||
| case SyntaxKind.MultiLineCommentTrivia: | ||
| var text = trivia.ToString(); | ||
| if (text.Contains("<auto-generated") || text.Contains("<autogenerated")) | ||
| { | ||
| return true; | ||
| } | ||
| break; | ||
| case SyntaxKind.WhitespaceTrivia: | ||
| case SyntaxKind.EndOfLineTrivia: | ||
| continue; | ||
| default: | ||
| return false; | ||
| } | ||
| } | ||
|
|
||
| return false; | ||
| } | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2742,6 +2742,18 @@ Comparing a span to 'null' or 'default' might not do what you intended. 'default | |
| |CodeFix|True| | ||
| --- | ||
|
|
||
| ## [CA2266](https://learn.microsoft.com/dotnet/fundamentals/code-analysis/quality-rules/ca2266): File-based program entry point should start with '#!' | ||
|
|
||
| When a file-based program consists of multiple files, the entry point file should start with a shebang ('#!') line to clearly distinguish it from other included files. | ||
|
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. It occurs to me that we may also want this when However, I don't wish to block on that.
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. Good idea, but I think that can be implemented as a follow up (filed #53749), since this PR should unblock us removing the experimental opt-in from |
||
|
|
||
| |Item|Value| | ||
| |-|-| | ||
| |Category|Usage| | ||
| |Enabled|True| | ||
| |Severity|Warning| | ||
| |CodeFix|True| | ||
| --- | ||
|
|
||
| ## [CA2300](https://learn.microsoft.com/dotnet/fundamentals/code-analysis/quality-rules/ca2300): Do not use insecure deserializer BinaryFormatter | ||
|
|
||
| The method '{0}' is insecure when deserializing untrusted data. If you need to instead detect BinaryFormatter deserialization without a SerializationBinder set, then disable rule CA2300, and enable rules CA2301 and CA2302. | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,14 @@ | ||
| // Copyright (c) Microsoft. All Rights Reserved. Licensed under the MIT license. See License.txt in the project root for license information. | ||
|
|
||
| using System.Collections.Immutable; | ||
| using Microsoft.CodeAnalysis.CodeFixes; | ||
|
|
||
| namespace Microsoft.NetCore.Analyzers.Usage | ||
| { | ||
| public abstract class MissingShebangInFileBasedProgramFixer : CodeFixProvider | ||
| { | ||
| public override ImmutableArray<string> FixableDiagnosticIds { get; } = ImmutableArray.Create(MissingShebangInFileBasedProgram.RuleId); | ||
|
|
||
| public override FixAllProvider GetFixAllProvider() => WellKnownFixAllProviders.BatchFixer; | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,29 @@ | ||
| // Copyright (c) Microsoft. All Rights Reserved. Licensed under the MIT license. See License.txt in the project root for license information. | ||
|
|
||
| using System.Collections.Immutable; | ||
| using Analyzer.Utilities; | ||
| using Microsoft.CodeAnalysis; | ||
| using Microsoft.CodeAnalysis.Diagnostics; | ||
|
|
||
| namespace Microsoft.NetCore.Analyzers.Usage | ||
| { | ||
| using static MicrosoftNetCoreAnalyzersResources; | ||
|
|
||
| public abstract class MissingShebangInFileBasedProgram : DiagnosticAnalyzer | ||
| { | ||
| internal const string RuleId = "CA2266"; | ||
|
|
||
| internal static readonly DiagnosticDescriptor Rule = DiagnosticDescriptorHelper.Create( | ||
| RuleId, | ||
| CreateLocalizableResourceString(nameof(MissingShebangInFileBasedProgramTitle)), | ||
| CreateLocalizableResourceString(nameof(MissingShebangInFileBasedProgramMessage)), | ||
| DiagnosticCategory.Usage, | ||
| RuleLevel.BuildWarning, | ||
| CreateLocalizableResourceString(nameof(MissingShebangInFileBasedProgramDescription)), | ||
| isPortedFxCopRule: false, | ||
| isDataflowRule: false, | ||
| isReportedAtCompilationEnd: false); | ||
|
|
||
| public sealed override ImmutableArray<DiagnosticDescriptor> SupportedDiagnostics { get; } = ImmutableArray.Create(Rule); | ||
| } | ||
| } |
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
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.
There's a part of me that really feels like this should be something more like a CompilationOption rather than just being passed along as an MSBuild property but I'm not really sure what we'd actually gain with that.
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.
Well, we have the feature flag FileBasedProgram (which causes roslyn to allow
#:and#!directives). We could pass the entry point file path as its value I guess. Feature flags are part of parse options. But I also don't know what we'd gain with that.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.
csc has the
-mainswitch today, but, it takes a type name, not a file path..I think that using a compiler-visible msbuild property is the right notch on the dial for the time being.