Fix #1571: FxCop converter doesn't emit run.logicalLocations + add validation rule

src/ReleaseHistory.md

@@ -420,4 +420,8 @@
 * BUGFIX: The `SarifCurrentToVersionOneVisitor` was dropping `run.id` and emitting an empty `run.stableId`. https://github.com/microsoft/sarif-sdk/issues/1557
 
 ## **v2.1.8** [Sdk](https://www.nuget.org/packages/Sarif.Sdk/2.1.8) | [Driver](https://www.nuget.org/packages/Sarif.Driver/2.1.8) | [Converters](https://www.nuget.org/packages/Sarif.Converters/2.1.8) | [Multitool](https://www.nuget.org/packages/Sarif.Multitool/2.1.8)
-* BUGFIX: Add missing `"additionalProperties": false` constraints to schema; add missing object descriptions and improve other object descriptions in schema; update schema version to -rtm.4.
+* BUGFIX: Add missing `"additionalProperties": false` constraints to schema; add missing object descriptions and improve other object descriptions in schema; update schema version to -rtm.4.
+
+## **v2.1.9** [Sdk](https://www.nuget.org/packages/Sarif.Sdk/2.1.9) | [Driver](https://www.nuget.org/packages/Sarif.Driver/2.1.9) | [Converters](https://www.nuget.org/packages/Sarif.Converters/2.1.9) | [Multitool](https://www.nuget.org/packages/Sarif.Multitool/2.1.9)
+* BUGFIX: FxCop converter produced logicalLocation.index but did not produce the run.logicalLocations array. https://github.com/microsoft/sarif-sdk/issues/1571
+* FEATURE: Add validation rule to ensure that `logicalLocation.index` is not present unless `run.logicalLocations` is present.

src/Sarif.Converters/FxCopConverter.cs

@@ -68,6 +68,8 @@ public override void Convert(Stream input, IResultLogWriter output, OptionallyEm
             };
 
             PersistResults(output, results, run);
+
+            output.WriteLogicalLocations(LogicalLocations);
         }
 
         internal ReportingDescriptor CreateRule(FxCopLogReader.Context context)
@@ -138,6 +140,11 @@ internal Result CreateResult(FxCopLogReader.Context context)
                 sourceFile = string.IsNullOrWhiteSpace(sourceFile) ? targetFile : sourceFile;
             }
 
+            // Don't emit a location if neither physical location nor logical location information
+            // is present. This is the case for CA0001, (unexpected error in analysis tool).
+            // https://docs.microsoft.com/en-us/visualstudio/code-quality/ca0001?view=vs-2019
+            bool emitLocation = false;
+
             // If we have a value, set physical location
             if (!string.IsNullOrWhiteSpace(sourceFile))
             {
@@ -146,6 +153,8 @@ internal Result CreateResult(FxCopLogReader.Context context)
                     ArtifactLocation = BuildFileLocationFromFxCopReference(sourceFile),
                     Region = context.Line == null ? null : Extensions.CreateRegion(context.Line.Value)
                 };
+
+                emitLocation = true;
             }
 
             string fullyQualifiedLogicalName = CreateFullyQualifiedLogicalName(context, out int logicalLocationIndex);
@@ -157,9 +166,14 @@ internal Result CreateResult(FxCopLogReader.Context context)
                     FullyQualifiedName = fullyQualifiedLogicalName,
                     Index = logicalLocationIndex
                 };
+
+                emitLocation = true;
             }
 
-            result.Locations = new List<Location> { location };
+            if (emitLocation)
+            {
+                result.Locations = new List<Location> { location };
+            }
 
             bool mapsDirectlyToSarifName;
 

src/Sarif.Multitool/Rules/RuleId.cs

@@ -16,5 +16,6 @@ public static class RuleId
         public const string UriBaseIdRequiresRelativeUri = "SARIF1014";
         public const string UriMustBeAbsolute = "SARIF1015";
         public const string ContextRegionRequiresRegion = "SARIF1016";
+        public const string InvalidIndex = "SARIF1017";
     }
 }

src/Sarif.Multitool/Rules/RuleResources.resx

@@ -186,4 +186,10 @@
   <data name="SARIF1016_Default" xml:space="preserve">
     <value>{0}: This "physicalLocation" object contains a "contextRegion" property, but it does not contain a "region" property.</value>
   </data>
+  <data name="SARIF1017_Default" xml:space="preserve">
+    <value>{0}: This "{1}" object contains a property "{2}" with value {3}, but either "{4}" is absent, or it has fewer than {5} elements.</value>
+  </data>
+  <data name="SARIF1017_InvalidIndex" xml:space="preserve">
+    <value>If an object contains a property that is used as an array index, then that array must be present and must contain at least "index + 1" elements.</value>
+  </data>
 </root>

src/Sarif.Multitool/Rules/SARIF1017.InvalidIndex.cs

@@ -0,0 +1,209 @@
+// 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.Collections.Generic;
+
+namespace Microsoft.CodeAnalysis.Sarif.Multitool.Rules
+{
+    public class InvalidIndex : SarifValidationSkimmerBase
+    {
+        private readonly MultiformatMessageString _fullDescription = new MultiformatMessageString
+        {
+            Text = RuleResources.SARIF1017_InvalidIndex
+        };
+
+        public override MultiformatMessageString FullDescription => _fullDescription;
+
+        public override FailureLevel DefaultLevel => FailureLevel.Error;
+
+        /// <summary>
+        /// SARIF1017
+        /// </summary>
+        public override string Id => RuleId.InvalidIndex;
+
+        protected override IEnumerable<string> MessageResourceNames => new string[]
+        {
+            nameof(RuleResources.SARIF1017_Default)
+        };
+
+        protected override void Analyze(Address address, string addressPointer)
+        {
+            ValidateArrayIndex(
+                address.Index,
+                Context.CurrentRun.Addresses,
+                addressPointer,
+                "address",
+                "index",
+                $"runs[{Context.CurrentRunIndex}].addresses");
+
+            ValidateArrayIndex(
+                address.ParentIndex,
+                Context.CurrentRun.Addresses,
+                addressPointer,
+                "address",
+                "parentIndex",
+                $"runs[{Context.CurrentRunIndex}].addresses");
+        }
+
+        protected override void Analyze(Artifact artifact, string artifactPointer)
+        {
+            ValidateArrayIndex(
+                artifact.ParentIndex,
+                Context.CurrentRun.Artifacts,
+                artifactPointer,
+                "artifact",
+                "parentIndex",
+                $"runs[{Context.CurrentRunIndex}].artifacts");
+        }
+
+        protected override void Analyze(ArtifactLocation artifactLocation, string artifactLocationPointer)
+        {
+            ValidateArrayIndex(
+                artifactLocation.Index,
+                Context.CurrentRun.Artifacts,
+                artifactLocationPointer,
+                "artifactLocation",
+                "index",
+                $"runs[{Context.CurrentRunIndex}].artifacts");
+        }
+
+        protected override void Analyze(GraphTraversal graphTraversal, string graphTraversalPointer)
+        {
+            ValidateArrayIndex(
+                graphTraversal.RunGraphIndex,
+                Context.CurrentRun.Graphs,
+                graphTraversalPointer,
+                "graphTraversal",
+                "runGraphIndex",
+                $"runs[{Context.CurrentRunIndex}].graphTraversals");
+
+            ValidateArrayIndex(
+                graphTraversal.ResultGraphIndex,
+                Context.CurrentResult.Graphs,
+                graphTraversalPointer,
+                "graphTraversal",
+                "resultGraphIndex",
+                $"runs[{Context.CurrentRunIndex}].results[{Context.CurrentResultIndex}].graphTraversals");
+        }
+
+        protected override void Analyze(LogicalLocation logicalLocation, string logicalLocationPointer)
+        {
+            ValidateArrayIndex(
+                logicalLocation.Index,
+                Context.CurrentRun.LogicalLocations,
+                logicalLocationPointer,
+                "logicalLocation",
+                "index",
+                $"runs[{Context.CurrentRunIndex}].logicalLocations");
+        }
+
+        protected override void Analyze(ReportingDescriptorReference reportingDescriptorReference, string reportingDescriptorReferencePointer)
+        {
+            string arrayPropertyName;
+            IList<ReportingDescriptor> reportingDescriptors;
+
+            if (Context.CurrentReportingDescriptorKind == SarifValidationContext.ReportingDescriptorKind.Rule)
+            {
+                arrayPropertyName = "rules";
+                reportingDescriptors = Context.CurrentRun.Tool.Driver.Rules;
+            }
+            else if (Context.CurrentReportingDescriptorKind == SarifValidationContext.ReportingDescriptorKind.Notification)
+            {
+                arrayPropertyName = "notifications";
+                reportingDescriptors = Context.CurrentRun.Tool.Driver.Notifications;
+            }
+            else
+            {
+                throw new InvalidOperationException("Unexpected call to Analyze(ReportingDescriptorReference)");
+            }
+
+            ValidateArrayIndex(
+                reportingDescriptorReference.Index,
+                reportingDescriptors,
+                reportingDescriptorReferencePointer,
+                "reportingDescriptorReference",
+                "index",
+                $"runs[{Context.CurrentRunIndex}].tool.driver.{arrayPropertyName}");
+        }
+
+        protected override void Analyze(Result result, string resultPointer)
+        {
+            ValidateArrayIndex(
+                result.RuleIndex,
+                Context.CurrentRun.Tool.Driver.Rules,
+                resultPointer,
+                "result",
+                "ruleIndex",
+                $"runs[{Context.CurrentRunIndex}].tool.driver.rules");
+        }
+
+        protected override void Analyze(ResultProvenance resultProvenance, string resultProvenancePointer)
+        {
+            ValidateArrayIndex(
+                resultProvenance.InvocationIndex,
+                Context.CurrentRun.Invocations,
+                resultProvenancePointer,
+                "resultProvenance",
+                "invocationIndex",
+                $"runs[{Context.CurrentRunIndex}].invocations");
+        }
+
+        protected override void Analyze(ThreadFlowLocation threadFlowLocation, string threadFlowLocationPointer)
+        {
+            ValidateArrayIndex(
+                threadFlowLocation.Index,
+                Context.CurrentRun.ThreadFlowLocations,
+                threadFlowLocationPointer,
+                "threadFlowLocation",
+                "index",
+                $"runs[{Context.CurrentRunIndex}].threadFlowLocations");
+        }
+
+        protected override void Analyze(WebRequest webRequest, string webRequestPointer)
+        {
+            ValidateArrayIndex(
+                webRequest.Index,
+                Context.CurrentRun.WebRequests,
+                webRequestPointer,
+                "webRequest",
+                "index",
+                $"runs[{Context.CurrentRunIndex}].webRequests");
+        }
+
+        protected override void Analyze(WebResponse webResponse, string webResponsePointer)
+        {
+            ValidateArrayIndex(
+                webResponse.Index,
+                Context.CurrentRun.WebResponses,
+                webResponsePointer,
+                "webResponse",
+                "index",
+                $"runs[{Context.CurrentRunIndex}].webResponses");
+        }
+
+        private void ValidateArrayIndex<T>(
+            int index,
+            IList<T> container,
+            string jsonPointer,
+            string objectName,
+            string propertyName,
+            string arrayName)
+        {
+            if (!IndexIsValid(index, container))
+            {
+                LogResult(
+                    jsonPointer,
+                    nameof(RuleResources.SARIF1017_Default),
+                    objectName,
+                    propertyName,
+                    index.ToInvariantString(),
+                    arrayName,
+                    (index + 1).ToInvariantString());
+            }
+        }
+
+        private static bool IndexIsValid<T>(int index, IList<T> container)
+                => index == -1 || (index >= 0 && container?.Count > index);
+    }
+}