Logical locations notes (#1185) (#1187)

* Respond to a small # of PR comments related to recent logical locations change. * Fix visibility on helper * Fix up v1 transformation with keys that collide * Preserve decorated name data * Rebaseline test for decorated name propagation * Respond to PR feedback. Update tests to close test holes. * Rebaseline updated test * Test key collision in annotated code locations. * Update baseline
microsoft · Dec 28, 2018 · 0e37c07 · 0e37c07
1 parent b0ffbe5
commit 0e37c07
Show file tree

Hide file tree

Showing 7 changed files with 312 additions and 62 deletions.
diff --git a/src/Sarif.UnitTests/TestData/SarifVersionOneToCurrentVisitor/v1/BasicResult.sarif b/src/Sarif.UnitTests/TestData/SarifVersionOneToCurrentVisitor/v1/BasicResult.sarif
@@ -31,12 +31,57 @@
           "kind": "type",
           "parentKey": "collections"
         },
+        "collections::list-KEY_COLLISION": {
+          "name": "list",
+          "kind": "member",
+          "parentKey": "collections"
+        },
         "collections": {
           "name": "collections",
           "kind": "namespace"
         }
       },
       "results": [
+        {
+          "ruleId": "C2001",
+          "formattedRuleMessage": {
+            "formatId": "default",
+            "arguments": [
+              "collections::list"
+            ]
+          },
+          "locations": [
+            {
+              "analysisTarget": {
+                "uri": "file:///home/buildAgent/src/collections/list.cpp"
+              },
+              "resultFile": {
+                "uri": "file:///home/buildAgent/src/collections/list.h",
+                "region": {
+                  "startLine": 1,
+                  "startColumn": 1,
+                  "endLine": 1,
+                  "endColumn": 28,
+                  "length": 27
+                }
+              },
+              "fullyQualifiedLogicalName": "collections::list",
+              "decoratedName": "?add@list@@_decorated_type_name"
+            },
+            {
+              "resultFile": {
+                "uri": "file:///home/buildAgent/src/collections/list_collision.cpp",
+                "region": {
+                  "startLine": 12,
+                  "endLine": 12
+                }
+              },
+              "fullyQualifiedLogicalName": "collections::list",
+              "logicalLocationKey": "collections::list-KEY_COLLISION",
+              "decoratedName": "?add@list@@_decorated_member_name"
+            }
+          ]
+        },
         {
           "ruleId": "C2001",
           "formattedRuleMessage": {
@@ -91,6 +136,18 @@
                 }
               },
               "fullyQualifiedLogicalName": "collections::list"
+            },
+            {
+              "message": "This reference to \"collections::list\" is not relevant to the problem.",
+              "physicalLocation": {
+                "uri": "file:///home/buildAgent/src/collections/list_collision.cpp",
+                "region": {
+                  "startLine": 12,
+                  "endLine": 12
+                }
+              },
+              "fullyQualifiedLogicalName": "collections::list",
+              "logicalLocationKey": "collections::list-KEY_COLLISION"
             }
           ]
         }
@@ -103,6 +160,13 @@
           "messageFormats": {
             "default": "Variable \"{0}\" was used without being initialized."
           }
+        },
+        "C2002": {
+          "id": "C2002",
+          "shortDescription": "A potentially confusing naming collision exists.",
+          "messageFormats": {
+            "default": "The fully qualified name \"{0}\" is shared between multiple constructs in this component, potentially compromising maintainability."
+          }
         }
       }
     }

diff --git a/...nitTests/TestData/SarifVersionOneToCurrentVisitor/v1/NotificationExceptionWithStack.sarif b/...nitTests/TestData/SarifVersionOneToCurrentVisitor/v1/NotificationExceptionWithStack.sarif
@@ -18,7 +18,7 @@
                   "message": "Exception thrown",
                   "module": "RuleLibrary",
                   "threadId": 52,
-                  "fullyQualifiedLogicalName": "Rules.SecureHashAlgorithmRule.Evaluate",
+                  "fullyQualifiedLogicalName": "Ambiguous.Item",
                   "uri": "file:///C:/src/main.cs",
                   "address": 10092852,
                   "line": 15,
@@ -40,10 +40,10 @@
                   "offset": 10475
                 },
                 {
-                  "module": "ExecutionEngine",
+                  "module": "Target",
                   "threadId": 52,
-                  "fullyQualifiedLogicalName": "ExecutionEngine.Engine.EvaluateRule",
-                  "logicalLocationKey": "ExecutionEngine.Engine.TestTarget",
+                  "fullyQualifiedLogicalName": "Ambiguous.Item",
+                  "logicalLocationKey": "SYNTHESIZED_KEY",
                   "uri": "file:///C:/src/testtarget.cs",
                   "address": 10073356,
                   "offset": 10475
@@ -60,15 +60,15 @@
         }
       ],
       "logicalLocations": {
-        "Rules.SecureHashAlgorithmRule.Evaluate": {
-          "name": "Evaluate",
+        "Ambiguous.Item": {
+          "name": "Item",
           "kind": "some kind"
         },
         "Rules.SecureHashAlgorithmRule.Register": {
-          "name": "InvalidName"
+          "name": "Register"
         },
-        "ExecutionEngine.Engine.TestTarget": {
-          "name": "TestTarget",
+        "SYNTHESIZED_KEY": {
+          "name": "Item",
           "kind": "another kind"
         }
       },

diff --git a/src/Sarif.UnitTests/TestData/SarifVersionOneToCurrentVisitor/v2/BasicResult.sarif b/src/Sarif.UnitTests/TestData/SarifVersionOneToCurrentVisitor/v2/BasicResult.sarif
@@ -27,17 +27,69 @@
         {
           "name": "list",
           "fullyQualifiedName": "collections::list",
+          "decoratedName": "?add@list@@_decorated_type_name",
           "parentIndex": 0,
           "kind": "type"
         },
         {
           "name": "add",
           "fullyQualifiedName": "collections::list::add",
+          "decoratedName": "?add@list@collections@@QAEXH@Z",
           "parentIndex": 1,
           "kind": "function"
+        },
+        {
+          "name": "list",
+          "fullyQualifiedName": "collections::list",
+          "decoratedName": "?add@list@@_decorated_member_name",
+          "parentIndex": 0,
+          "kind": "member"
         }
       ],
       "results": [
+        {
+          "ruleId": "C2001",
+          "message": {
+            "messageId": "default",
+            "arguments": [
+              "collections::list"
+            ]
+          },
+          "analysisTarget": {
+            "uri": "file:///home/buildAgent/src/collections/list.cpp"
+          },
+          "locations": [
+            {
+              "physicalLocation": {
+                "fileLocation": {
+                  "uri": "file:///home/buildAgent/src/collections/list.h"
+                },
+                "region": {
+                  "startLine": 1,
+                  "startColumn": 1,
+                  "endLine": 1,
+                  "endColumn": 28,
+                  "byteLength": 27
+                }
+              },
+              "fullyQualifiedLogicalName": "collections::list",
+              "logicalLocationIndex": 1
+            },
+            {
+              "physicalLocation": {
+                "fileLocation": {
+                  "uri": "file:///home/buildAgent/src/collections/list_collision.cpp"
+                },
+                "region": {
+                  "startLine": 12,
+                  "endLine": 12
+                }
+              },
+              "fullyQualifiedLogicalName": "collections::list",
+              "logicalLocationIndex": 3
+            }
+          ]
+        },
         {
           "ruleId": "C2001",
           "level": "error",
@@ -104,6 +156,22 @@
               "message": {
                 "text": "Another reference to \"count\" was declared here."
               }
+            },
+            {
+              "physicalLocation": {
+                "fileLocation": {
+                  "uri": "file:///home/buildAgent/src/collections/list_collision.cpp"
+                },
+                "region": {
+                  "startLine": 12,
+                  "endLine": 12
+                }
+              },
+              "fullyQualifiedLogicalName": "collections::list",
+              "logicalLocationIndex": 3,
+              "message": {
+                "text": "This reference to \"collections::list\" is not relevant to the problem."
+              }
             }
           ],
           "suppressionStates": ["suppressedExternally"],
@@ -123,6 +191,15 @@
             "messageStrings": {
               "default": "Variable \"{0}\" was used without being initialized."
             }
+          },
+          "C2002": {
+            "id": "C2002",
+            "shortDescription": {
+              "text": "A potentially confusing naming collision exists."
+            },
+            "messageStrings": {
+              "default": "The fully qualified name \"{0}\" is shared between multiple constructs in this component, potentially compromising maintainability."
+            }
           }
         }
       },

diff --git a/...nitTests/TestData/SarifVersionOneToCurrentVisitor/v2/NotificationExceptionWithStack.sarif b/...nitTests/TestData/SarifVersionOneToCurrentVisitor/v2/NotificationExceptionWithStack.sarif
@@ -38,6 +38,7 @@
                             "endColumn": 9
                           }
                         },
+                        "fullyQualifiedLogicalName": "Ambiguous.Item",
                         "logicalLocationIndex": 0,
                         "message": {
                           "text": "Exception thrown"
@@ -54,7 +55,8 @@
                             "uri": "file:///C:/src/main.cs"
                           }
                         },
-                        "logicalLocationIndex": 0
+                        "fullyQualifiedLogicalName": "Rules.SecureHashAlgorithmRule.Register",
+                        "logicalLocationIndex": 1
                       },
                       "module": "RuleLibrary",
                       "threadId": 52,
@@ -67,7 +69,7 @@
                             "uri": "file:///C:/src/utils.cs"
                           }
                         },
-                        "logicalLocationIndex": 0
+                        "fullyQualifiedLogicalName": "ExecutionEngine.Engine.EvaluateRule"
                       },
                       "module": "ExecutionEngine",
                       "threadId": 52,
@@ -81,10 +83,10 @@
                             "uri": "file:///C:/src/testtarget.cs"
                           }
                         },
-                        "fullyQualifiedLogicalName": "ExecutionEngine.Engine.TestTarget",
-                        "logicalLocationIndex": 0
+                        "fullyQualifiedLogicalName": "Ambiguous.Item",
+                        "logicalLocationIndex": 2
                       },
-                      "module": "ExecutionEngine",
+                      "module": "Target",
                       "threadId": 52,
                       "address": 10073356,
                       "offset": 10475
@@ -106,17 +108,17 @@
       ],
       "logicalLocations": [
         {
-          "name": "Evaluate",
-          "fullyQualifiedName": "Rules.SecureHashAlgorithmRule.Evaluate",
+          "name": "Item",
+          "fullyQualifiedName": "Ambiguous.Item",
           "kind": "some kind"
         },
         {
-          "name": "InvalidName",
+          "name": "Register",
           "fullyQualifiedName": "Rules.SecureHashAlgorithmRule.Register"
         },
         {
-          "name": "TestTarget",
-          "fullyQualifiedName": "ExecutionEngine.Engine.TestTarget",
+          "name": "Item",
+          "fullyQualifiedName": "Ambiguous.Item",
           "kind": "another kind"
         }
       ],

diff --git a/src/Sarif.UnitTests/Visitors/SarifVersionOneToCurrentVisitorTests.cs b/src/Sarif.UnitTests/Visitors/SarifVersionOneToCurrentVisitorTests.cs
@@ -16,6 +16,12 @@ namespace Microsoft.CodeAnalysis.Sarif.UnitTests.Transformers
 {
     public class SarifVersionOneToCurrentVisitorTests : FileDiffingTests
     {
+        // IMPORTANT: set this static member to true and rerun tests in order to reset all baselines. The test
+        // are constructed to automatically fail when baselining, to prevent this property from being mistakenly
+        // checked in as 'true'. When rebaselining, be sure to scrutinize baseline file deltas carefully to
+        // ensure any modifications are correct.
+        private static bool s_Rebaseline = false;
+
         public SarifVersionOneToCurrentVisitorTests(ITestOutputHelper outputHelper) : base(outputHelper) { }
 
         private static SarifLogVersionOne GetSarifLogVersionOne(string logText)
@@ -39,8 +45,6 @@ private static void VerifyVersionOneToCurrentTransformation(string v1LogText, st
             v2LogText.Should().Be(v2LogExpectedText);
         }
 
-        private static bool s_Rebaseline = false;
-
         private void VerifyVersionOneToCurrentTransformationFromResource(string v1InputResourceName, string v2ExpectedResourceName = null)
         {
             v2ExpectedResourceName = v2ExpectedResourceName ?? v1InputResourceName;