Fix #1642: Don't require --force if --inline is specified (#1653)

This addresses one of a set of about 10 bugs I recently filed in the course of fixing #1340 ("Multitool --force option: advertised default is false, actual default is true"). I discovered that there were widespread inconsistencies in the behaviors and underlying implementations of the various commands related to their treatment of command line options. This was one of them.

As usual, I took the opportunity to address IDE code warnings, improve documentation, and generally clean up the code I came in contact with while investigating the bug.

The core of the fix is the introduction of the method(s) `ValidateOutputOptions` in DriverExtensionMethods.cs. The code in each Multitool command is rearranged or enhanced to invoke these methods.

Notes:

1. I implemented the options validation methods in Sarif.Driver rather than in Sarif.Multitool because that's where the options classes themselves are defined. It's not obvious that that's where they belong.

2. The validation methods use a helper method `GetOptionDescription` that takes an option property such as `SingleFileOptionsBase.Force` and creates a description for the option like `"-p, --pretty-print"`, which is the format that the CommandLine package uses. This method was originally defined in Sarif.Multitool, but since it is now needed in Sarif.Driver, I moved it there. The corresponding unit test file moved to Test.UnitTests.Sarif.Driver.

4. As I've mentioned before, our unit test namespaces are a hodgepodge. In the same assembly, you can find tests in the namespaces `Microsoft.CodeAnalysis.Sarif.Multitool`, `Microsoft.CodeAnalysis.Test.UnitTests.Sarif.Multitool`, and `Test.UnitTests.Sarif.Multitool`. This makes it hard to find  your tests in the Test Explorer. Per @michaelcfanning I'm standardizing on `Microsoft.CodeAnalysis.Sarif.Multitool`. I'll modify the test project files (or build.props, if I can think of a way to automate it) accordingly in a later PR.

src/ReleaseHistory.md

@@ -453,5 +453,6 @@
 * BUGFIX: Multitool merge command produced invalid SARIF if there were 0 input files. https://github.com/microsoft/sarif-sdk/issues/1592
 * BUGFIX: FortifyFpr converter produced invalid SARIF. https://github.com/microsoft/sarif-sdk/issues/1593
 * BUGFIX: FxCop converter produced empty `result.message` objects. https://github.com/microsoft/sarif-sdk/issues/1594
+* BUGFIX: Some Multitool commands required --force even if --inline was specified. https://github.com/microsoft/sarif-sdk/issues/1642
 * FEATURE: Add validation rule to ensure correctness of `originalUriBaseIds` entries. https://github.com/microsoft/sarif-sdk/issues/1485
-* FEATURE: Improve presentation of option validation messages from the Multitool `page` command. https://github.com/microsoft/sarif-sdk/issues/1629
+* FEATURE: Improve presentation of option validation messages from the Multitool `page` command. https://github.com/microsoft/sarif-sdk/issues/1629

src/Sarif.Driver/DriverExtensionMethods.cs

@@ -1,6 +1,8 @@
 // Copyright (c) Microsoft. All rights reserved.
 // Licensed under the MIT license. See LICENSE file in the project root for full license information.using System;
 
+using System;
+using System.Globalization;
 using Microsoft.CodeAnalysis.Sarif.Writers;
 
 namespace Microsoft.CodeAnalysis.Sarif.Driver
@@ -17,5 +19,97 @@ public static LoggingOptions ConvertToLoggingOptions(this AnalyzeOptionsBase ana
 
             return loggingOptions;
         }
+
+        /// <summary>
+        /// Ensures the consistency of the SingleFileOptionsBase command line options related to
+        /// the location of the output file, and adjusts the options for ease of use.
+        /// </summary>
+        /// <param name="options">
+        /// A <see cref="SingleFileOptionsBase"/> object containing the relevant options.
+        /// </param>
+        /// <returns>
+        /// true if the options are internally consistent; otherwise false.
+        /// </returns>
+        public static bool ValidateOutputOptions(this SingleFileOptionsBase options)
+        {
+            bool valid = true;
+
+            if (options.Inline)
+            {
+                if (options.OutputFilePath == null)
+                {
+                    options.Force = true;
+                    options.OutputFilePath = options.InputFilePath;
+                }
+                else
+                {
+                    ReportInvalidOutputOptions(options);
+                    valid = false;
+                }
+            }
+            else
+            {
+                if (options.OutputFilePath == null)
+                {
+                    ReportInvalidOutputOptions(options);
+                    valid = false;
+                }
+            }
+
+            return valid;
+        }
+
+        private static void ReportInvalidOutputOptions(SingleFileOptionsBase options)
+        {
+            string inlineOptionsDescription = DriverUtilities.GetOptionDescription<SingleFileOptionsBase>(nameof(options.Inline));
+            string outputFilePathOptionDescription = DriverUtilities.GetOptionDescription<SingleFileOptionsBase>(nameof(options.OutputFilePath));
+
+            Console.Error.WriteLine(
+                string.Format(
+                    CultureInfo.CurrentCulture,
+                    DriverResources.ExactlyOneOfTwoOptionsIsRequired,
+                    inlineOptionsDescription,
+                    outputFilePathOptionDescription));
+        }
+
+        /// <summary>
+        /// Ensures the consistency of the MultipleFilesOptionsBase command line options related to
+        /// the location of the output file, and adjusts the options for ease of use.
+        /// </summary>
+        /// <param name="options">
+        /// A <see cref="MultipleFilesOptionsBase"/> object containing the relevant options.
+        /// </param>
+        /// <returns>
+        /// true if the options are internally consistent; otherwise false.
+        /// </returns>
+        /// <remarks>
+        /// At this time, this method does not actually do any validation. Unlike the case of
+        /// SingleFileOptionsBase, where you have to specify exactly one of --inline and
+        /// --output-file, it is _not_ necessary to specify --output-folder-path if --inline is
+        /// absent, because by default each transformed file is written to the path containing
+        /// corresponding input file.
+        ///
+        /// However, similarly to the case of SingleFileOptionsBase, we _do_ want to set --force
+        /// whenever --inline is true, because there's no reason to force the user to type
+        /// "--force" when they've already said that they want to overwrite the input file
+        /// (see https://github.com/microsoft/sarif-sdk/issues/1642).
+        ///
+        /// So we introduce this method for three reasons:
+        /// 1) For symmetry with the SingleFileOptionsBase,
+        /// 2) To DRY out the logic for making --inline and --force consistent, and
+        /// 3) To leave an obvious place to put output file option consistency logic if it's
+        ///    needed in future.
+        /// </remarks>
+        public static bool ValidateOutputOptions(this MultipleFilesOptionsBase options)
+        {
+            bool valid = true;
+
+            if (options.Inline)
+            {
+                options.Force = true;
+            }
+
+            return valid;
+        }
     }
 }

src/Sarif.Driver/DriverResources.resx

@@ -117,6 +117,9 @@
   <resheader name="writer">
     <value>System.Resources.ResXResourceWriter, System.Windows.Forms, Version=4.0.0.0, Culture=neutral, PublicKeyToken=b77a5c561934e089</value>
   </resheader>
+  <data name="ExactlyOneOfTwoOptionsIsRequired" xml:space="preserve">
+    <value>Exactly one of the '{0}' and '{1}' options must be present.</value>
+  </data>
   <data name="InvalidPE_Description" xml:space="preserve">
     <value>A binary was not analyzed as the it does not appear to be a valid portable executable.</value>
   </data>

src/Sarif.Driver/DriverUtilities.cs

@@ -4,6 +4,9 @@
 using System;
 using System.Collections.Generic;
 using System.Globalization;
+using System.Reflection;
+using System.Text;
+using CommandLine;
 
 namespace Microsoft.CodeAnalysis.Sarif.Driver
 {
@@ -85,5 +88,49 @@ public static bool ReportWhetherOutputFileCanBeCreated(string outputFilePath, bo
         /// </returns>
         public static bool CanCreateOutputFile(string outputFilePath, bool force, IFileSystem fileSystem)
             => !fileSystem.FileExists(outputFilePath) || force;
+
+        /// <summary>
+        /// Constructs a description of a command line option.
+        /// </summary>
+        /// <typeparam name="T">
+        /// The type that defines the property corresponding to the command line option.
+        /// </typeparam>
+        /// <param name="optionPropertyName">
+        /// The name of the property corresponding to the command line option.
+        /// </param>
+        /// <returns>
+        /// A description of the specified command line option in the format "shortName", "longName",
+        /// or "shortName, longName", depending on which names are available.
+        /// </returns>
+        /// <remarks>
+        /// The CommandLine package defines <see cref="CommandLine.OptionAttribute"/> to mark
+        /// properties that correspond to command line options. CommandLine performs some validation,
+        /// but sometimes it is necessary to perform additional validation. In that case, it is
+        /// desirable for the validation message to refer to the invalid parameter in the same
+        /// format that CommandLine itself does.
+        /// </remarks>
+        public static string GetOptionDescription<T>(string optionPropertyName)
+            => GetOptionDescription(typeof(T), optionPropertyName);
+
+        public static string GetOptionDescription(Type optionsType, string optionPropertyName)
+        {
+            PropertyInfo propertyInfo =
+                optionsType.GetProperty(optionPropertyName, BindingFlags.Public | BindingFlags.Instance) ??
+                throw new ArgumentException(
+                    $"The type {optionsType.FullName} does not contain a public instance property named {optionPropertyName}.",
+                    nameof(optionPropertyName));
+
+            var optionAttribute =
+                propertyInfo.GetCustomAttribute<OptionAttribute>() ??
+                throw new ArgumentException(
+                    $"The {optionPropertyName} property of the type {optionsType.FullName} does not define a command line option.",
+                    nameof(optionPropertyName));
+
+            var builder = new StringBuilder(optionAttribute.ShortName);
+            if (builder.Length > 0 && optionAttribute.LongName != string.Empty) { builder.Append(", "); }
+            builder.Append(optionAttribute.LongName);
+
+            return builder.ToString();
+        }
     }
-}
+}

src/Sarif.Driver/Sdk/DriverSdkExtensions.cs

@@ -5,7 +5,6 @@
 using System.Collections.Generic;
 using System.IO;
 using System.Linq;
-using System.Security;
 
 namespace Microsoft.CodeAnalysis.Sarif.Driver.Sdk
 {

src/Sarif.Driver/Sdk/MultifileOptionsBase.cs

@@ -6,7 +6,7 @@
 
 namespace Microsoft.CodeAnalysis.Sarif.Driver
 {
-    internal class MultipleFilesOptionsBase : CommonOptionsBase
+    public class MultipleFilesOptionsBase : CommonOptionsBase
     {
         [Option(
             'r',
@@ -18,14 +18,14 @@ internal class MultipleFilesOptionsBase : CommonOptionsBase
         [Option(
             'o',
             "output-folder",
-            HelpText = "A folder to output the transformed files to.")]
+            HelpText = "A folder to output the transformed files to. If absent, each transformed file is written to the same directory as the corresponding input file.")]
         public string OutputFolderPath { get; internal set; }
 
         [Option(
             'i',
             "inline",
             Default = false,
-            HelpText = "Write all newly generated content to the input file.")]
+            HelpText = "Overwrite each input file with the corresponding transformed file.")]
         public bool Inline { get; set; }
 
 

src/Sarif.Multitool/AbsoluteUriCommand.cs

@@ -26,9 +26,7 @@ public int Run(AbsoluteUriOptions absoluteUriOptions)
             {
                 IEnumerable<AbsoluteUriFile> absoluteUriFiles = GetAbsoluteUriFiles(absoluteUriOptions);
 
-
-                bool outputFilesCanBeCreated = DriverUtilities.ReportWhetherOutputFilesCanBeCreated(absoluteUriFiles.Select(f => f.OutputFilePath), absoluteUriOptions.Force, _fileSystem);
-                if (!outputFilesCanBeCreated) { return 1; }
+                if (!ValidateOptions(absoluteUriOptions, absoluteUriFiles)) { return Failure; }
 
                 if (!absoluteUriOptions.Inline)
                 {
@@ -49,10 +47,21 @@ public int Run(AbsoluteUriOptions absoluteUriOptions)
             catch (Exception ex)
             {
                 Console.WriteLine(ex);
-                return 1;
+                return Failure;
             }
 
-            return 0;
+            return Success;
+        }
+
+        private bool ValidateOptions(AbsoluteUriOptions absoluteUriOptions, IEnumerable<AbsoluteUriFile> absoluteUriFiles)
+        {
+            bool valid = true;
+
+            valid &= absoluteUriOptions.ValidateOutputOptions();
+
+            valid &= DriverUtilities.ReportWhetherOutputFilesCanBeCreated(absoluteUriFiles.Select(f => f.OutputFilePath), absoluteUriOptions.Force, _fileSystem);
+
+            return valid;
         }
 
         private IEnumerable<AbsoluteUriFile> GetAbsoluteUriFiles(AbsoluteUriOptions absoluteUriOptions)

src/Sarif.Multitool/CommandBase.cs

@@ -13,13 +13,16 @@ namespace Microsoft.CodeAnalysis.Sarif.Multitool
 {
     public abstract class CommandBase
     {
+        public const int Success = 0;
+        public const int Failure = 1;
+
         protected static bool ValidateNonNegativeCommandLineOption<T>(int optionValue, string optionName)
         {
             bool valid = true;
 
             if (optionValue < 0)
             {
-                string optionDescription = CommandUtilities.GetOptionDescription<T>(optionName);
+                string optionDescription = DriverUtilities.GetOptionDescription<T>(optionName);
                 Console.Error.WriteLine(
                     string.Format(
                         CultureInfo.CurrentCulture,

src/Sarif.Multitool/CommandUtilities.cs

@@ -3,9 +3,6 @@
 
 using System;
 using System.IO;
-using System.Reflection;
-using System.Text;
-using CommandLine;
 using Microsoft.CodeAnalysis.Sarif.Driver;
 
 namespace Microsoft.CodeAnalysis.Sarif.Multitool
@@ -21,7 +18,7 @@ internal static string GetTransformedOutputFileName(SingleFileOptionsBase option
                 return filePath;
             }
 
-            if (!String.IsNullOrEmpty(options.OutputFilePath))
+            if (!string.IsNullOrEmpty(options.OutputFilePath))
             {
                 return options.OutputFilePath;
             }
@@ -38,49 +35,5 @@ internal static string GetTransformedOutputFileName(SingleFileOptionsBase option
             // For an input file named MyFile.json, return MyFile.json.transformed.sarif.
             return Path.GetFullPath(options.InputFilePath + TransformedExtension);
         }
-
-        /// <summary>
-        /// Constructs a description of a command line option.
-        /// </summary>
-        /// <typeparam name="T">
-        /// The type that defines the property corresponding to the command line option.
-        /// </typeparam>
-        /// <param name="optionPropertyName">
-        /// The name of the property corresponding to the command line option.
-        /// </param>
-        /// <returns>
-        /// A description of the specified command line option in the format "shortName", "longName",
-        /// or "shortName, longName", depending on which names are available.
-        /// </returns>
-        /// <remarks>
-        /// The CommandLine package defines <see cref="CommandLine.OptionAttribute"/> to mark
-        /// properties that correspond to command line options. CommandLine performs some validation,
-        /// but sometimes it is necessary to perform additional validation. In that case, it is
-        /// desirable for the validation message to refer to the invalid parameter in the same
-        /// format that CommandLine itself does.
-        /// </remarks>
-        internal static string GetOptionDescription<T>(string optionPropertyName)
-            => GetOptionDescription(typeof(T), optionPropertyName);
-
-        internal static string GetOptionDescription(Type optionsType, string optionPropertyName)
-        {
-            PropertyInfo propertyInfo =
-                optionsType.GetProperty(optionPropertyName, BindingFlags.Public | BindingFlags.Instance) ??
-                throw new ArgumentException(
-                    $"The type {optionsType.FullName} does not contain a public instance property named {optionPropertyName}.",
-                    nameof(optionPropertyName));
-
-            var optionAttribute =
-                propertyInfo.GetCustomAttribute<OptionAttribute>() ??
-                throw new ArgumentException(
-                    $"The {optionPropertyName} property of the type {optionsType.FullName} does not define a command line option.",
-                    nameof(optionPropertyName));
-
-            var builder = new StringBuilder(optionAttribute.ShortName);
-            if (builder.Length > 0 && optionAttribute.LongName != string.Empty) { builder.Append(", "); }
-            builder.Append(optionAttribute.LongName);
-
-            return builder.ToString();
-        }
     }
 }