Display positions in SourceDuplicate errors

source/interprocedural_analyses/taint/annotationParser.ml

@@ -19,6 +19,7 @@ type taint_kind =
 type source_or_sink = {
   name: string;
   kind: taint_kind;
+  location: JsonParsing.JsonAst.LocationWithPath.t option;
 }
 
 let parse_source ~allowed ?subkind name =

source/interprocedural_analyses/taint/annotationParser.mli

@@ -12,6 +12,7 @@ type taint_kind =
 type source_or_sink = {
   name: string;
   kind: taint_kind;
+  location: JsonParsing.JsonAst.LocationWithPath.t option;
 }
 
 val parse_source

source/interprocedural_analyses/taint/taintConfiguration.ml

@@ -489,12 +489,12 @@ module Heap = struct
   let default =
     let sources =
       List.map
-        ~f:(fun name -> { AnnotationParser.name; kind = Named })
+        ~f:(fun name -> { AnnotationParser.name; kind = Named; location = None })
         ["Demo"; "Test"; "UserControlled"; "PII"; "Secrets"; "Cookies"]
     in
     let sinks =
       List.map
-        ~f:(fun name -> { AnnotationParser.name; kind = Named })
+        ~f:(fun name -> { AnnotationParser.name; kind = Named; location = None })
         [
           "Demo";
           "FileSystem";
@@ -733,7 +733,10 @@ module Error = struct
         previous_location: JsonAst.LocationWithPath.t option;
       }
     | OptionDuplicate of string
-    | SourceDuplicate of string
+    | SourceDuplicate of {
+        name: string;
+        previous_location: JsonAst.LocationWithPath.t option;
+      }
     | SinkDuplicate of string
     | TransformDuplicate of string
     | FeatureDuplicate of string
@@ -807,7 +810,17 @@ module Error = struct
           previous_location.location
     | OptionDuplicate name ->
         Format.fprintf formatter "Multiple values were passed in for option `%s`" name
-    | SourceDuplicate name -> Format.fprintf formatter "Duplicate entry for source: `%s`" name
+    | SourceDuplicate { name; previous_location = None } ->
+        Format.fprintf formatter "Duplicate entry for source: `%s`" name
+    | SourceDuplicate { name; previous_location = Some previous_location } ->
+        Format.fprintf
+          formatter
+          "Duplicate entry for source: `%s`, previous entry was at `%a:%a`"
+          name
+          PyrePath.pp
+          previous_location.path
+          JsonAst.Location.pp_start
+          previous_location.location
     | SinkDuplicate name -> Format.fprintf formatter "Duplicate entry for sink: `%s`" name
     | TransformDuplicate name -> Format.fprintf formatter "Duplicate entry for transform: `%s`" name
     | FeatureDuplicate name -> Format.fprintf formatter "Duplicate entry for feature: `%s`" name
@@ -899,6 +912,13 @@ let from_json_list source_json_list =
     json_exception_to_error ~path ~section:key (fun () ->
         JsonAst.Json.Util.member_exn key value |> JsonAst.Json.Util.to_string_exn |> Result.return)
   in
+  let json_string_member_with_location ~path key value =
+    json_exception_to_error ~path ~section:key (fun () ->
+        let node = JsonAst.Json.Util.member_exn key value in
+        let value = JsonAst.Json.Util.to_string_exn node in
+        let location = JsonAst.Json.Util.to_location_exn node in
+        Result.return (value, location))
+  in
   let json_integer_member_with_location ~path key value =
     json_exception_to_error ~path ~section:key (fun () ->
         let node = JsonAst.Json.Util.member_exn key value in
@@ -961,8 +981,15 @@ let from_json_list source_json_list =
       ~current_keys:(JsonAst.Json.Util.keys json)
       ~valid_keys:["name"; "kind"; "comment"]
     >>= fun () ->
-    json_string_member ~path "name" json
-    >>= fun name -> parse_kind ~path json >>| fun kind -> { AnnotationParser.name; kind }
+    json_string_member_with_location ~path "name" json
+    >>= fun (name, location) ->
+    parse_kind ~path json
+    >>| fun kind ->
+    {
+      AnnotationParser.name;
+      kind;
+      location = Some (JsonAst.LocationWithPath.create ~path ~location);
+    }
   in
   let parse_source_annotations (path, json) =
     array_member ~path "sources" json
@@ -1573,9 +1600,11 @@ let validate ({ Heap.sources; sinks; transforms; features; rules; _ } as configu
   in
   Result.combine_errors_unit
     [
-      ensure_list_unique
-        ~get_name:(fun { AnnotationParser.name; _ } -> name)
-        ~get_error:(fun name -> Error.SourceDuplicate name)
+      ensure_list_unique_with_location
+        ~get_key:(fun { AnnotationParser.name; _ } -> name)
+        ~get_location:(fun { AnnotationParser.location; _ } -> location)
+        ~get_error:(fun { AnnotationParser.name; _ } previous_location ->
+          Error.SourceDuplicate { name; previous_location })
         sources;
       ensure_list_unique
         ~get_name:(fun { AnnotationParser.name; _ } -> name)

source/interprocedural_analyses/taint/taintConfiguration.mli

@@ -149,7 +149,10 @@ module Error : sig
         previous_location: JsonParsing.JsonAst.LocationWithPath.t option;
       }
     | OptionDuplicate of string
-    | SourceDuplicate of string
+    | SourceDuplicate of {
+        name: string;
+        previous_location: JsonParsing.JsonAst.LocationWithPath.t option;
+      }
     | SinkDuplicate of string
     | TransformDuplicate of string
     | FeatureDuplicate of string

source/interprocedural_analyses/taint/test/annotationParserTest.ml

@@ -9,9 +9,9 @@ open OUnit2
 open Core
 open Taint
 
-let named name = { AnnotationParser.name; kind = Named }
+let named name = { AnnotationParser.name; kind = Named; location = None }
 
-let parametric name = { AnnotationParser.name; kind = Parametric }
+let parametric name = { AnnotationParser.name; kind = Parametric; location = None }
 
 let test_parse_source _ =
   let assert_parsed ~allowed ?subkind ~expected source =

source/interprocedural_analyses/taint/test/configurationTest.ml

@@ -75,7 +75,13 @@ let assert_rules_equal ~actual ~expected =
     expected
 
 
-let named name = { AnnotationParser.name; kind = Named }
+let without_locations sources_or_sinks =
+  List.map
+    ~f:(fun source_or_sink -> { source_or_sink with AnnotationParser.location = None })
+    sources_or_sinks
+
+
+let named name = { AnnotationParser.name; kind = Named; location = None }
 
 let test_simple _ =
   let configuration =
@@ -108,8 +114,8 @@ let test_simple _ =
     }
   |}
   in
-  assert_equal configuration.sources [named "A"; named "B"];
-  assert_equal configuration.sinks [named "C"; named "D"];
+  assert_equal (without_locations configuration.sources) [named "A"; named "B"];
+  assert_equal (without_locations configuration.sinks) [named "C"; named "D"];
   assert_equal configuration.features ["E"; "F"];
   assert_equal (List.length configuration.rules) 1;
   assert_equal (List.hd_exn configuration.rules).code 2001;
@@ -345,7 +351,7 @@ let test_combined_source_rules _ =
     }
   |}
   in
-  assert_equal configuration.sources [named "A"; named "B"];
+  assert_equal (without_locations configuration.sources) [named "A"; named "B"];
   assert_equal configuration.sinks [];
   assert_rules_equal
     ~actual:configuration.rules
@@ -413,7 +419,7 @@ let test_combined_source_rules _ =
     }
   |}
   in
-  assert_equal configuration.sources [named "A"; named "B"; named "C"];
+  assert_equal (without_locations configuration.sources) [named "A"; named "B"; named "C"];
   assert_equal configuration.sinks [];
   assert_equal
     (TaintConfiguration.PartialSinkLabelsMap.to_alist configuration.partial_sink_labels)
@@ -565,7 +571,7 @@ let test_string_combine_rules _ =
     }
   |}
   in
-  assert_equal configuration.sources [named "A"; named "B"; named "C"];
+  assert_equal (without_locations configuration.sources) [named "A"; named "B"; named "C"];
   assert_equal
     [
       ( "UserDefinedPartialSink",
@@ -744,8 +750,8 @@ let test_multiple_configurations _ =
     | Error _ -> assert_failure "Failed to parse configuration"
     | Ok configuration -> configuration
   in
-  assert_equal configuration.sources [named "A"; named "B"];
-  assert_equal configuration.sinks [named "C"; named "D"];
+  assert_equal (without_locations configuration.sources) [named "A"; named "B"];
+  assert_equal (without_locations configuration.sinks) [named "C"; named "D"];
   assert_equal configuration.features ["E"; "F"];
   assert_equal (List.length configuration.rules) 1;
   assert_equal (List.hd_exn configuration.rules).code 2001;
@@ -776,7 +782,24 @@ let test_validate _ =
     assert_equal ~printer (Error [error]) result
   in
   assert_parse_error
-    ~errors:[TaintConfiguration.Error.SourceDuplicate "UserControlled"]
+    ~errors:
+      [
+        TaintConfiguration.Error.SourceDuplicate
+          {
+            name = "UserControlled";
+            previous_location =
+              Some
+                {
+                  JsonParsing.JsonAst.LocationWithPath.path =
+                    PyrePath.create_absolute "/taint.config";
+                  location =
+                    {
+                      JsonParsing.JsonAst.Location.start = { line = 4; column = 18 };
+                      stop = { line = 4; column = 33 };
+                    };
+                };
+          };
+      ]
     {|
     {
       "sources": [
@@ -830,7 +853,21 @@ let test_validate _ =
   assert_parse_error
     ~errors:
       [
-        TaintConfiguration.Error.SourceDuplicate "UserControlled";
+        TaintConfiguration.Error.SourceDuplicate
+          {
+            name = "UserControlled";
+            previous_location =
+              Some
+                {
+                  JsonParsing.JsonAst.LocationWithPath.path =
+                    PyrePath.create_absolute "/taint.config";
+                  location =
+                    {
+                      JsonParsing.JsonAst.Location.start = { line = 4; column = 18 };
+                      stop = { line = 4; column = 33 };
+                    };
+                };
+          };
         TaintConfiguration.Error.SinkDuplicate "Test";
         TaintConfiguration.Error.FeatureDuplicate "concat";
       ]
@@ -851,7 +888,24 @@ let test_validate _ =
     }
     |};
   assert_parse_error
-    ~errors:[TaintConfiguration.Error.SourceDuplicate "UserControlled"]
+    ~errors:
+      [
+        TaintConfiguration.Error.SourceDuplicate
+          {
+            name = "UserControlled";
+            previous_location =
+              Some
+                {
+                  JsonParsing.JsonAst.LocationWithPath.path =
+                    PyrePath.create_absolute "/taint.config";
+                  location =
+                    {
+                      JsonParsing.JsonAst.Location.start = { line = 5; column = 18 };
+                      stop = { line = 5; column = 33 };
+                    };
+                };
+          };
+      ]
     {|
     {
       "sources": [
@@ -1131,7 +1185,7 @@ let test_implicit_sources _ =
            }
        |}
   in
-  assert_equal configuration.sources [named "StringDigit"];
+  assert_equal (without_locations configuration.sources) [named "StringDigit"];
   match configuration.implicit_sources with
   | { TaintConfiguration.literal_strings = [{ pattern; source_kind }] } ->
       assert_equal ~cmp:Sources.equal source_kind (Sources.NamedSource "StringDigit");
@@ -1169,7 +1223,7 @@ let test_implicit_sinks _ =
            }
      |}
   in
-  assert_equal configuration.sinks [named "HTMLContainer"];
+  assert_equal (without_locations configuration.sinks) [named "HTMLContainer"];
   match configuration.implicit_sinks with
   | { TaintConfiguration.literal_string_sinks = [{ pattern; sink_kind }]; _ } ->
       assert_equal ~cmp:Sinks.equal sink_kind (Sinks.NamedSink "HTMLContainer");

source/interprocedural_analyses/taint/test/modelTest.ml

@@ -30,14 +30,14 @@ let set_up_environment
   in
   let project = ScratchProject.setup ~context ["test.py", source] in
   let taint_configuration =
-    let named name = { AnnotationParser.name; kind = Named } in
+    let named name = { AnnotationParser.name; kind = Named; location = None } in
     let sources =
       [
         named "TestTest";
         named "UserControlled";
         named "Test";
         named "Demo";
-        { AnnotationParser.name = "WithSubkind"; kind = Parametric };
+        { AnnotationParser.name = "WithSubkind"; kind = Parametric; location = None };
       ]
     in
     let sinks =
@@ -47,7 +47,7 @@ let set_up_environment
         named "Test";
         named "Demo";
         named "XSS";
-        { AnnotationParser.name = "TestSinkWithSubkind"; kind = Parametric };
+        { AnnotationParser.name = "TestSinkWithSubkind"; kind = Parametric; location = None };
       ]
     in
     let transforms = [TaintTransform.Named "TestTransform"; TaintTransform.Named "DemoTransform"] in
@@ -183,8 +183,13 @@ let assert_invalid_model ?path ?source ?(sources = []) ~context ~model_source ~e
       {
         empty with
         sources =
-          List.map ~f:(fun name -> { AnnotationParser.name; kind = Named }) ["A"; "B"; "Test"];
-        sinks = List.map ~f:(fun name -> { AnnotationParser.name; kind = Named }) ["X"; "Y"; "Test"];
+          List.map
+            ~f:(fun name -> { AnnotationParser.name; kind = Named; location = None })
+            ["A"; "B"; "Test"];
+        sinks =
+          List.map
+            ~f:(fun name -> { AnnotationParser.name; kind = Named; location = None })
+            ["X"; "Y"; "Test"];
         features = ["featureA"; "featureB"];
         rules = [];
         partial_sink_labels =