From 0f320996cf57f46cabce0b6bb555954de1491795 Mon Sep 17 00:00:00 2001 From: "Michael B. Gale" Date: Wed, 22 Feb 2023 12:29:10 +0000 Subject: [PATCH] Make improvements based on PR feedback --- .../BuildScripts.cs | 6 ++--- .../Semmle.Autobuild.Cpp/CppAutobuilder.cs | 6 ++--- .../BuildScripts.cs | 6 ++--- .../CSharpAutobuilder.cs | 14 +++++----- .../Semmle.Autobuild.CSharp/DotNetRule.cs | 16 +++-------- .../Semmle.Autobuild.Shared/Autobuilder.cs | 21 ++++++++------- .../BuildCommandAutoRule.cs | 26 +++++++----------- .../DiagnosticClassifier.cs | 5 ++-- .../extractor/Semmle.Util/Semmle.Util.csproj | 2 +- .../extractor/Semmle.Util/ToolStatusPage.cs | 27 ++++++++++++++----- 10 files changed, 60 insertions(+), 69 deletions(-) diff --git a/cpp/autobuilder/Semmle.Autobuild.Cpp.Tests/BuildScripts.cs b/cpp/autobuilder/Semmle.Autobuild.Cpp.Tests/BuildScripts.cs index 160287c049a..76ec2ccf1e9 100644 --- a/cpp/autobuilder/Semmle.Autobuild.Cpp.Tests/BuildScripts.cs +++ b/cpp/autobuilder/Semmle.Autobuild.Cpp.Tests/BuildScripts.cs @@ -1,5 +1,6 @@ using Xunit; using Semmle.Autobuild.Shared; +using Semmle.Util; using System.Collections.Generic; using System; using System.Linq; @@ -79,10 +80,7 @@ namespace Semmle.Autobuild.Cpp.Tests { var ret = (this as IBuildActions).RunProcess(cmd, args, workingDirectory, env, out var stdout); - foreach (var line in stdout) - { - onOutput(line); - } + stdout.ForEach(line => onOutput(line)); return ret; } diff --git a/cpp/autobuilder/Semmle.Autobuild.Cpp/CppAutobuilder.cs b/cpp/autobuilder/Semmle.Autobuild.Cpp/CppAutobuilder.cs index 5ebb3acca1c..223425a8b18 100644 --- a/cpp/autobuilder/Semmle.Autobuild.Cpp/CppAutobuilder.cs +++ b/cpp/autobuilder/Semmle.Autobuild.Cpp/CppAutobuilder.cs @@ -21,12 +21,10 @@ namespace Semmle.Autobuild.Cpp public class CppAutobuilder : Autobuilder { - private DiagnosticClassifier classifier; + private readonly DiagnosticClassifier classifier; - public CppAutobuilder(IBuildActions actions, CppAutobuildOptions options) : base(actions, options) - { + public CppAutobuilder(IBuildActions actions, CppAutobuildOptions options) : base(actions, options) => classifier = new DiagnosticClassifier(); - } protected override DiagnosticClassifier DiagnosticClassifier => classifier; diff --git a/csharp/autobuilder/Semmle.Autobuild.CSharp.Tests/BuildScripts.cs b/csharp/autobuilder/Semmle.Autobuild.CSharp.Tests/BuildScripts.cs index a44c7c7918f..0243dc927d1 100644 --- a/csharp/autobuilder/Semmle.Autobuild.CSharp.Tests/BuildScripts.cs +++ b/csharp/autobuilder/Semmle.Autobuild.CSharp.Tests/BuildScripts.cs @@ -1,5 +1,6 @@ using Xunit; using Semmle.Autobuild.Shared; +using Semmle.Util; using System.Collections.Generic; using System; using System.Linq; @@ -89,10 +90,7 @@ namespace Semmle.Autobuild.CSharp.Tests { var ret = (this as IBuildActions).RunProcess(cmd, args, workingDirectory, env, out var stdout); - foreach (var line in stdout) - { - onOutput(line); - } + stdout.ForEach(line => onOutput(line)); return ret; } diff --git a/csharp/autobuilder/Semmle.Autobuild.CSharp/CSharpAutobuilder.cs b/csharp/autobuilder/Semmle.Autobuild.CSharp/CSharpAutobuilder.cs index 1b06fe073f1..5714ace6c45 100644 --- a/csharp/autobuilder/Semmle.Autobuild.CSharp/CSharpAutobuilder.cs +++ b/csharp/autobuilder/Semmle.Autobuild.CSharp/CSharpAutobuilder.cs @@ -41,10 +41,8 @@ namespace Semmle.Autobuild.CSharp protected override DiagnosticClassifier DiagnosticClassifier => diagnosticClassifier; - public CSharpAutobuilder(IBuildActions actions, CSharpAutobuildOptions options) : base(actions, options) - { + public CSharpAutobuilder(IBuildActions actions, CSharpAutobuildOptions options) : base(actions, options) => diagnosticClassifier = new CSharpDiagnosticClassifier(); - } public override BuildScript GetBuildScript() { @@ -164,7 +162,7 @@ namespace Semmle.Autobuild.CSharp } message.Severity = DiagnosticMessage.TspSeverity.Error; - Diagnostic(message); + AddDiagnostic(message); } // both dotnet and msbuild builds require project or solution files; if we haven't found any @@ -177,7 +175,7 @@ namespace Semmle.Autobuild.CSharp "You can manually specify a suitable build command for your project."; message.Severity = DiagnosticMessage.TspSeverity.Error; - Diagnostic(message); + AddDiagnostic(message); } else if (dotNetRule is not null && dotNetRule.NotDotNetProjects.Any()) { @@ -187,7 +185,7 @@ namespace Semmle.Autobuild.CSharp string.Join('\n', dotNetRule.NotDotNetProjects.Select(p => $"- `{p.FullPath}`")); message.Severity = DiagnosticMessage.TspSeverity.Warning; - Diagnostic(message); + AddDiagnostic(message); } // report any projects that failed to build with .NET Core @@ -201,7 +199,7 @@ namespace Semmle.Autobuild.CSharp "or to ensure that they can be built successfully."; message.Severity = DiagnosticMessage.TspSeverity.Error; - Diagnostic(message); + AddDiagnostic(message); } // report any projects that failed to build with MSBuild @@ -215,7 +213,7 @@ namespace Semmle.Autobuild.CSharp "or to ensure that they can be built successfully.";; message.Severity = DiagnosticMessage.TspSeverity.Error; - Diagnostic(message); + AddDiagnostic(message); } } diff --git a/csharp/autobuilder/Semmle.Autobuild.CSharp/DotNetRule.cs b/csharp/autobuilder/Semmle.Autobuild.CSharp/DotNetRule.cs index 4a337bd76ac..e18058339f5 100644 --- a/csharp/autobuilder/Semmle.Autobuild.CSharp/DotNetRule.cs +++ b/csharp/autobuilder/Semmle.Autobuild.CSharp/DotNetRule.cs @@ -15,22 +15,14 @@ namespace Semmle.Autobuild.CSharp /// internal class DotNetRule : IBuildRule { - private IEnumerable> notDotNetProjects; - public readonly List FailedProjectsOrSolutions = new(); /// /// A list of projects which are incompatible with DotNet. /// - public IEnumerable> NotDotNetProjects - { - get { return this.notDotNetProjects; } - } + public IEnumerable> NotDotNetProjects { get; private set; } - public DotNetRule() - { - this.notDotNetProjects = new List>(); - } + public DotNetRule() => NotDotNetProjects = new List>(); public BuildScript Analyse(IAutobuilder builder, bool auto) { @@ -39,11 +31,11 @@ namespace Semmle.Autobuild.CSharp if (auto) { - notDotNetProjects = builder.ProjectsOrSolutionsToBuild + NotDotNetProjects = builder.ProjectsOrSolutionsToBuild .SelectMany(p => Enumerators.Singleton(p).Concat(p.IncludedProjects)) .OfType>() .Where(p => !p.DotNetProject); - var notDotNetProject = notDotNetProjects.FirstOrDefault(); + var notDotNetProject = NotDotNetProjects.FirstOrDefault(); if (notDotNetProject is not null) { diff --git a/csharp/autobuilder/Semmle.Autobuild.Shared/Autobuilder.cs b/csharp/autobuilder/Semmle.Autobuild.Shared/Autobuilder.cs index 94dcb5c987d..48c362053ce 100644 --- a/csharp/autobuilder/Semmle.Autobuild.Shared/Autobuilder.cs +++ b/csharp/autobuilder/Semmle.Autobuild.Shared/Autobuilder.cs @@ -284,7 +284,7 @@ namespace Semmle.Autobuild.Shared /// Write to the diagnostics file. /// /// The diagnostics entry to write. - public void Diagnostic(DiagnosticMessage diagnostic) + public void AddDiagnostic(DiagnosticMessage diagnostic) { diagnostics.AddEntry(diagnostic); } @@ -320,13 +320,11 @@ namespace Semmle.Autobuild.Shared // if the build succeeded, all diagnostics we captured from the build output should be warnings; // otherwise they should all be errors var diagSeverity = buildResult == 0 ? DiagnosticMessage.TspSeverity.Warning : DiagnosticMessage.TspSeverity.Error; - foreach (var diagResult in this.DiagnosticClassifier.Results) + this.DiagnosticClassifier.Results.Select(result => result.ToDiagnosticMessage(this)).ForEach(result => { - var diag = diagResult.ToDiagnosticMessage(this); - diag.Severity = diagSeverity; - - Diagnostic(diag); - } + result.Severity = diagSeverity; + AddDiagnostic(result); + }); return buildResult; } @@ -345,8 +343,11 @@ namespace Semmle.Autobuild.Shared /// The resulting . public DiagnosticMessage MakeDiagnostic(string id, string name) { - DiagnosticMessage diag = new(new($"{this.Options.Language.UpperCaseName.ToLower()}/autobuilder/{id}", name)); - diag.Source.ExtractorName = Options.Language.UpperCaseName.ToLower(); + DiagnosticMessage diag = new(new( + $"{this.Options.Language.UpperCaseName.ToLower()}/autobuilder/{id}", + name, + Options.Language.UpperCaseName.ToLower() + )); diag.Visibility.StatusPage = true; return diag; @@ -367,7 +368,7 @@ namespace Semmle.Autobuild.Shared "You can manually specify a suitable build command for your project."; message.Severity = DiagnosticMessage.TspSeverity.Error; - Diagnostic(message); + AddDiagnostic(message); } /// diff --git a/csharp/autobuilder/Semmle.Autobuild.Shared/BuildCommandAutoRule.cs b/csharp/autobuilder/Semmle.Autobuild.Shared/BuildCommandAutoRule.cs index 16f153a3c2a..c0990a75ae1 100644 --- a/csharp/autobuilder/Semmle.Autobuild.Shared/BuildCommandAutoRule.cs +++ b/csharp/autobuilder/Semmle.Autobuild.Shared/BuildCommandAutoRule.cs @@ -10,29 +10,21 @@ namespace Semmle.Autobuild.Shared public class BuildCommandAutoRule : IBuildRule { private readonly WithDotNet withDotNet; - private IEnumerable candidatePaths; - private string? scriptPath; /// /// A list of paths to files in the project directory which we classified as scripts. /// - public IEnumerable CandidatePaths - { - get { return this.candidatePaths; } - } + public IEnumerable CandidatePaths { get; private set; } /// /// The path of the script we decided to run, if any. /// - public string? ScriptPath - { - get { return this.scriptPath; } - } + public string? ScriptPath { get; private set; } public BuildCommandAutoRule(WithDotNet withDotNet) { this.withDotNet = withDotNet; - this.candidatePaths = new List(); + this.CandidatePaths = new List(); } /// @@ -70,18 +62,18 @@ namespace Semmle.Autobuild.Shared var scripts = buildScripts.SelectMany(s => extensions.Select(e => s + e)); // search through the files in the project directory for paths which end in one of // the names given by `scripts`, then order them by their distance from the root - this.candidatePaths = builder.Paths.Where(p => scripts.Any(p.Item1.ToLower().EndsWith)).OrderBy(p => p.Item2).Select(p => p.Item1); + this.CandidatePaths = builder.Paths.Where(p => scripts.Any(p.Item1.ToLower().EndsWith)).OrderBy(p => p.Item2).Select(p => p.Item1); // pick the first matching path, if there is one - this.scriptPath = candidatePaths.FirstOrDefault(); + this.ScriptPath = this.CandidatePaths.FirstOrDefault(); - if (scriptPath is null) + if (this.ScriptPath is null) return BuildScript.Failure; var chmod = new CommandBuilder(builder.Actions); - chmod.RunCommand("/bin/chmod", $"u+x {scriptPath}"); + chmod.RunCommand("/bin/chmod", $"u+x {this.ScriptPath}"); var chmodScript = builder.Actions.IsWindows() ? BuildScript.Success : BuildScript.Try(chmod.Script); - var dir = builder.Actions.GetDirectoryName(scriptPath); + var dir = builder.Actions.GetDirectoryName(this.ScriptPath); // A specific .NET Core version may be required return chmodScript & withDotNet(builder, environment => @@ -93,7 +85,7 @@ namespace Semmle.Autobuild.Shared if (vsTools is not null) command.CallBatFile(vsTools.Path); - command.RunCommand(scriptPath); + command.RunCommand(this.ScriptPath); return command.Script; }); } diff --git a/csharp/autobuilder/Semmle.Autobuild.Shared/DiagnosticClassifier.cs b/csharp/autobuilder/Semmle.Autobuild.Shared/DiagnosticClassifier.cs index a29129f6ded..29f44e4e8f2 100644 --- a/csharp/autobuilder/Semmle.Autobuild.Shared/DiagnosticClassifier.cs +++ b/csharp/autobuilder/Semmle.Autobuild.Shared/DiagnosticClassifier.cs @@ -80,15 +80,14 @@ namespace Semmle.Autobuild.Shared /// The line to which the rules should be applied to. public void ClassifyLine(string line) { - foreach (var rule in this.rules) + this.rules.ForEach(rule => { var match = rule.Pattern.Match(line); if (match.Success) { rule.Fire(this, match); } - } - + }); } } } diff --git a/csharp/extractor/Semmle.Util/Semmle.Util.csproj b/csharp/extractor/Semmle.Util/Semmle.Util.csproj index aff07714957..894488f9f84 100644 --- a/csharp/extractor/Semmle.Util/Semmle.Util.csproj +++ b/csharp/extractor/Semmle.Util/Semmle.Util.csproj @@ -15,7 +15,7 @@ - + diff --git a/csharp/extractor/Semmle.Util/ToolStatusPage.cs b/csharp/extractor/Semmle.Util/ToolStatusPage.cs index d9b89903e01..8c650659595 100644 --- a/csharp/extractor/Semmle.Util/ToolStatusPage.cs +++ b/csharp/extractor/Semmle.Util/ToolStatusPage.cs @@ -31,12 +31,13 @@ namespace Semmle.Util /// /// Name of the CodeQL extractor. This is used to identify which tool component the reporting descriptor object should be nested under in SARIF. /// - public string? ExtractorName { get; set; } + public string? ExtractorName { get; } - public TspSource(string id, string name) + public TspSource(string id, string name, string? extractorName = null) { Id = id; Name = name; + ExtractorName = extractorName; } } @@ -66,6 +67,13 @@ namespace Semmle.Util /// True if the message should be sent to telemetry (defaults to false). /// public bool? Telemetry { get; set; } + + public TspVisibility(bool? statusPage = null, bool? cliSummaryTable = null, bool? telemetry = null) + { + this.StatusPage = statusPage; + this.CLISummaryTable = cliSummaryTable; + this.Telemetry = telemetry; + } } public class TspLocation @@ -78,6 +86,15 @@ namespace Semmle.Util public int? StartColumn { get; set; } public int? EndLine { get; set; } public int? EndColumn { get; set; } + + public TspLocation(string? file = null, int? startLine = null, int? startColumn = null, int? endLine = null, int? endColumn = null) + { + this.File = file; + this.StartLine = startLine; + this.StartColumn = startColumn; + this.EndLine = endLine; + this.EndColumn = endColumn; + } } /// @@ -109,9 +126,6 @@ namespace Semmle.Util /// If true, then this message won't be presented to users. /// public bool Internal { get; set; } - /// - /// - /// public TspVisibility Visibility { get; } public TspLocation Location { get; } /// @@ -162,7 +176,8 @@ namespace Semmle.Util serializer = new JsonSerializer { - ContractResolver = contractResolver + ContractResolver = contractResolver, + NullValueHandling = NullValueHandling.Ignore }; writer = streamWriter;