sarif1008: ContextRegionMustBeProperSupersetOfRegion check

src/Sarif.Multitool/Rules/RuleResources.resx

@@ -154,10 +154,16 @@
     <value>Certain URIs are required to be absolute.</value>
   </data>
   <data name="SARIF1008_PhysicalLocationPropertiesMustBeConsistent_FullDescription_Text" xml:space="preserve">
-    <value>If the "contextRegion" property is present, the "region" property must also be present.</value>
+    <value>A SARIF 'physicalLocation' object has two related properties 'region' and 'contextRegion'. If 'contextRegion' is present, then 'region' must also be present, and 'contextRegion' must be a "proper superset" of 'region'. That is, 'contextRegion' must completely contain 'region', and it must be larger than 'region'. To understand why this is so we must understand the roles of the 'region' and 'contextRegion' properties.
+
+'region' allows both users and tools to distinguish similar results within the same artifact. If a SARIF viewer has access to the artifact, it can display it, and highlight the location identified by the analysis tool. If the region has a 'snippet' property, then even if the viewer doesn't have access to the artifact (which might be the case for a web-based viewer), it can still display the faulty code.
+
+'contextRegion' provides users with a broader view of the result location. Typically, it consists of a range starting a few lines before 'region' and ending a few lines after. Again, if a SARIF viewer has access to the artifact, it can display it, and highlight the context region (perhaps in a lighter shade than the region itself). This isn't terribly useful since the user can already see the whole file, with the 'region' already highlighted. But if 'contextRegion' has a 'snippet' property, then even a viewer without access to the artifact can display a few lines of code surrounding the actual result, which is helpful to users.
+
+If the SARIF validator reports that 'contextRegion' is present but 'region' is absent, then it's possible that the tool should have populated 'region' rather than 'contextRegion', or that it simply neglected to populate 'region'. If the validator reports that 'contextRegion' is not a proper superset of 'region', then it's possible that the tool reversed 'region' and 'contextRegion'. If 'region' and 'contextRegion' are identical, the tool should simply omit 'contextRegion'.</value>
   </data>
   <data name="SARIF1008_PhysicalLocationPropertiesMustBeConsistent_Error_ContextRegionRequiresRegion_Text" xml:space="preserve">
-    <value>{0}: This "physicalLocation" object contains a "contextRegion" property, but it does not contain a "region" property.</value>
+    <value>{0}: This 'physicalLocation' object contains a 'contextRegion' property, but it does not contain a 'region' property. This is invalid because the purpose of 'contextRegion' is to provide a viewing context around the 'region' which is the location of the result. If the tool incorrectly populated 'contextRegion' instead of 'region', then fix it so that it populates only the 'region'. If the tool simply neglected to populate 'region', then fix it so that it does.</value>
   </data>
   <data name="SARIF1009_IndexPropertiesMustBeConsistentWithArrays_Error_TargetArrayMustBeLongEnough_Text" xml:space="preserve">
     <value>{0}: This "{1}" object contains a property "{2}" with value {3}, but "{4}" has fewer than {5} elements.</value>
@@ -204,4 +210,7 @@
   <data name="SARIF1007_RegionPropertiesMustBeConsistent_FullDescription_Text" xml:space="preserve">
     <value>Placeholder_SARIF1007_RegionPropertiesMustBeConsistent_FullDescription_Text</value>
   </data>
+  <data name="SARIF1008_PhysicalLocationPropertiesMustBeConsistent_Error_ContextRegionMustBeProperSupersetOfRegion_Text" xml:space="preserve">
+    <value>{0}: This 'physicalLocation' object contains both a 'region' and a 'contextRegion' property, but 'contextRegion' is not a proper superset of 'region'. This is invalid because the purpose of 'contextRegion' is to provide a viewing context around the 'region' which is the location of the result. If the tool simply reversed 'region' and 'contextRegion', then fix it so it puts the correct values in the correct properties. If 'region' and 'contextRegion' are identical, the 'contextRegion' is unnecessary, and (by the spec) the tool must not populate it.</value>
+  </data>
 </root>

src/Sarif.Multitool/Rules/SARIF1008.PhysicalLocationPropertiesMustBeConsistent.cs

@@ -7,25 +7,82 @@ namespace Microsoft.CodeAnalysis.Sarif.Multitool.Rules
 {
     public class PhysicalLocationPropertiesMustBeConsistent : SarifValidationSkimmerBase
     {
-        public override MultiformatMessageString FullDescription => new MultiformatMessageString
-        {
-            Text = RuleResources.SARIF1008_PhysicalLocationPropertiesMustBeConsistent_FullDescription_Text
-        };
+        /// <summary>
+        /// SARIF1008
+        /// </summary>
+        public override string Id => RuleId.PhysicalLocationPropertiesMustBeConsistent;
 
-        public override FailureLevel DefaultLevel => FailureLevel.Error;
+        /// <summary>
+        /// A SARIF 'physicalLocation' object has two related properties 'region' and 'contextRegion'.
+        /// If 'contextRegion' is present, then 'region' must also be present, and 'contextRegion' must
+        /// be a "proper superset" of 'region'. That is, 'contextRegion' must completely contain 'region',
+        /// and it must be larger than 'region'. To understand why this is so we must understand the
+        /// roles of the 'region' and 'contextRegion' properties. 
+        ///
+        /// 'region' allows both users and tools to distinguish similar results within the same artifact.
+        /// If a SARIF viewer has access to the artifact, it can display it, and highlight the location 
+        /// identified by the analysis tool.If the region has a 'snippet' property, then even if the viewer
+        /// doesn't have access to the artifact (which might be the case for a web-based viewer), it can
+        /// still display the faulty code.
+        /// 
+        /// 'contextRegion' provides users with a broader view of the result location. Typically, it consists
+        /// of a range starting a few lines before 'region' and ending a few lines after. Again, if a SARIF
+        /// viewer has access to the artifact, it can display it, and highlight the context region (perhaps in
+        /// a lighter shade than the region itself). This isn't terribly useful since the user can already see
+        /// the whole file, with the 'region' already highlighted. But if 'contextRegion' has a 'snippet'
+        /// property, then even a viewer without access to the artifact can display a few lines of code surrounding
+        /// the actual result, which is helpful to users.
+        /// 
+        /// If the SARIF validator reports that 'contextRegion' is present but 'region' is absent, then it's
+        /// possible that the tool should have populated 'region' rather than 'contextRegion', or that it simply
+        /// neglected to populate 'region'. If the validator reports that 'contextRegion' is not a proper superset
+        /// of 'region', then it's possible that the tool reversed 'region' and 'contextRegion'. If 'region' and
+        /// 'contextRegion' are identical, the tool should simply omit
+        /// </summary>
+        public override MultiformatMessageString FullDescription => new MultiformatMessageString { Text = RuleResources.SARIF1008_PhysicalLocationPropertiesMustBeConsistent_FullDescription_Text };
 
-        public override string Id => RuleId.PhysicalLocationPropertiesMustBeConsistent;
+        protected override IEnumerable<string> MessageResourceNames => new string[] {
+                    nameof(RuleResources.SARIF1008_PhysicalLocationPropertiesMustBeConsistent_Error_ContextRegionRequiresRegion_Text),
+                    nameof(RuleResources.SARIF1008_PhysicalLocationPropertiesMustBeConsistent_Error_ContextRegionMustBeProperSupersetOfRegion_Text)
+                };
 
-        protected override IEnumerable<string> MessageResourceNames => new string[]
-        {
-            nameof(RuleResources.SARIF1008_PhysicalLocationPropertiesMustBeConsistent_Error_ContextRegionRequiresRegion_Text)
-        };
+        public override FailureLevel DefaultLevel => FailureLevel.Error;
 
         protected override void Analyze(PhysicalLocation physicalLocation, string physicalLocationPointer)
         {
-            if (physicalLocation.ContextRegion != null && physicalLocation.Region == null)
+            if (physicalLocation.ContextRegion == null)
+            {
+                return;
+            }
+
+            // ContextRegionRequiresRegion: If 'contextRegion' is present, then 'region' must also be present.
+            if (physicalLocation.Region == null)
+            {
+                // {0}: This 'physicalLocation' object contains a 'contextRegion' property, but it does
+                // not contain a 'region' property. This is invalid because the purpose of 'contextRegion'
+                // is to provide a viewing context around the 'region' which is the location of the result.
+                // If the tool incorrectly populated 'contextRegion' instead of 'region', then fix it so
+                // that it populates only the 'region'. If the tool simply neglected to populate 'region',
+                // then fix it so that it does.
+                LogResult(
+                    physicalLocationPointer,
+                    nameof(RuleResources.SARIF1008_PhysicalLocationPropertiesMustBeConsistent_Error_ContextRegionRequiresRegion_Text));
+                return;
+            }
+
+            // ContextRegionMustBeProperSupersetOfRegion: 'contextRegion' must be a proper superset of 'region'.
+            if (!physicalLocation.ContextRegion.IsProperSupersetOf(physicalLocation.Region))
             {
-                LogResult(physicalLocationPointer, nameof(RuleResources.SARIF1008_PhysicalLocationPropertiesMustBeConsistent_Error_ContextRegionRequiresRegion_Text));
+                // {0}: This 'physicalLocation' object contains both a 'region' and a 'contextRegion'
+                // property, but 'contextRegion' is not a proper superset of 'region'. This is invalid
+                // because the purpose of 'contextRegion' is to provide a viewing context around the
+                // 'region' which is the location of the result. If the tool simply reversed 'region'
+                // and 'contextRegion', then fix it so it puts the correct values in the correct
+                // properties. If 'region' and 'contextRegion' are identical, the 'contextRegion' is
+                // unnecessary, and (by the spec) the tool must not populate it.
+                LogResult(
+                    physicalLocationPointer,
+                    nameof(RuleResources.SARIF1008_PhysicalLocationPropertiesMustBeConsistent_Error_ContextRegionMustBeProperSupersetOfRegion_Text));
             }
         }
     }

src/Sarif/Core/Region.cs

@@ -1,27 +1,184 @@
 // 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.Runtime.CompilerServices;
+
 namespace Microsoft.CodeAnalysis.Sarif
 {
     public partial class Region
     {
-        public bool IsBinaryRegion
+        public bool IsBinaryRegion => this.ByteOffset >= 0;
+
+        public bool IsLineColumnBasedTextRegion => this.StartLine >= 1;
+
+        public bool IsOffsetBasedTextRegion => this.CharOffset >= 0;
+
+        public override string ToString()
+        {
+            return this.FormatForVisualStudio();
+        }
+
+        public void PopulateDefaults()
         {
-            get
+            if (this.IsLineColumnBasedTextRegion)
+            {
+                this.PopulateLineColumnBasedTextDefaults();
+            }
+
+            if (this.IsOffsetBasedTextRegion)
             {
-                // Is this right? What about an insertion point right after a BOM in a text file??
-                // Do we need to just bite the bullet and make these Nullable type so that we have a
-                // clear indicator of whether the region is binary vs. textual? I tend to think so.
-                return
-                     this.StartLine == 0 &&
-                     this.CharLength == 0 &&
-                     this.CharOffset == 0;
+                this.PopulateOffsetBasedTextDefaults();
+            }
+
+            if (this.IsBinaryRegion)
+            {
+                this.PopulateBinaryDefaults();
             }
         }
 
-        public override string ToString()
+        private void PopulateLineColumnBasedTextDefaults()
         {
-            return this.FormatForVisualStudio();
+            if (this.EndLine == 0)
+            {
+                this.EndLine = this.StartLine;
+            }
+
+            if (this.StartColumn == 0)
+            {
+                this.StartColumn = 1;
+            }
+
+            if (this.EndColumn == 0)
+            {
+                this.EndColumn = int.MaxValue;
+            }
+        }
+
+        private void PopulateOffsetBasedTextDefaults()
+        {
+            if (this.CharLength == -1)
+            {
+                this.CharLength = 0;
+            }
+        }
+
+        private void PopulateBinaryDefaults()
+        {
+            if (this.ByteLength == -1)
+            {
+                this.ByteLength = 0;
+            }
+        }
+
+        public bool IsProperSupersetOf(Region subRegion)
+        {
+            this.PopulateDefaults();
+            subRegion.PopulateDefaults();
+
+            if (this.IsLineColumnBasedTextRegion &&
+                subRegion.IsLineColumnBasedTextRegion &&
+                !IsLineColumnBasedTextRegionProperSupersetOf(subRegion))
+            {
+                return false;
+            }
+
+            if (this.IsOffsetBasedTextRegion &&
+                subRegion.IsOffsetBasedTextRegion &&
+                !IsOffsetBasedTextRegionProperSupetSetOf(subRegion))
+            {
+                return false;
+            }
+
+            if (this.IsBinaryRegion &&
+                subRegion.IsBinaryRegion &&
+                !IsBinaryRegionProperSupersetOf(subRegion))
+            {
+                return false;
+            }
+
+            // if we reach here, the region and context region have been expressed as different property sets,
+            // and it is not possible to judge validity without looking at the actual content.
+            // It is a potential false negative.
+            return true;
+        }
+
+        private bool IsLineColumnBasedTextRegionProperSupersetOf(Region subRegion)
+        {
+            if (this.StartLine > subRegion.StartLine || this.EndLine < subRegion.EndLine)
+            {
+                return false;
+            }
+
+            if (this.StartLine == subRegion.StartLine && this.StartColumn > subRegion.StartColumn)
+            {
+                return false;
+            }
+
+            if (this.EndLine == subRegion.EndLine && this.EndColumn < subRegion.EndColumn)
+            {
+                return false;
+            }
+
+            if (this.StartLine == subRegion.StartLine &&
+                this.EndLine == subRegion.EndLine &&
+                this.StartColumn == subRegion.StartColumn &&
+                this.EndColumn == subRegion.EndColumn)
+            {
+                return false;
+            }
+
+            return true;
+        }
+
+        private bool IsBinaryRegionProperSupersetOf(Region subRegion)
+        {
+            if (this.ByteOffset > subRegion.ByteOffset)
+            {
+                return false;
+            }
+
+            if (GetByteEndOffset(this) < GetByteEndOffset(subRegion))
+            {
+                return false;
+            }
+
+            if (this.ByteOffset == subRegion.ByteOffset && this.ByteLength <= subRegion.ByteLength)
+            {
+                return false;
+            }
+
+            return true;
+        }
+
+        private bool IsOffsetBasedTextRegionProperSupetSetOf(Region subRegion)
+        {
+            if (this.CharOffset > subRegion.CharOffset)
+            {
+                return false;
+            }
+
+            if (GetCharEndOffset(this) < GetCharEndOffset(subRegion))
+            {
+                return false;
+            }
+
+            if (this.CharOffset == subRegion.CharOffset && this.CharLength <= subRegion.CharLength)
+            {
+                return false;
+            }
+
+            return true;
+        }
+
+        private static int GetCharEndOffset(Region region)
+        {
+            return region.CharOffset + region.CharLength;
+        }
+
+        private static int GetByteEndOffset(Region region)
+        {
+            return region.ByteOffset + region.ByteLength;
         }
     }
 }