From 94fbd3977eeafd2782735d3c03cab6a02ff663e4 Mon Sep 17 00:00:00 2001
From: Jules Villard <jul@fb.com>
Date: Wed, 27 May 2020 08:23:50 -0700
Subject: [PATCH] [issues] make sure the immutable parts of issues are always
 the same

Summary:
We allow some fields of issues to be defined dynamically, more precisely
when loading AL files. We don't want this to be abused and in particular
we don't want to miss an issue being declared once for a checker and
another time for another as this would be hard to debug.

Also, only register unknown issue types as coming from AL if they
haven't been registered already.

Reviewed By: skcho

Differential Revision: D21685879

fbshipit-source-id: 9e9438a75
---
 infer/src/base/Config.ml     |  9 ++++++++-
 infer/src/base/IssueType.ml  | 24 ++++++++++++++++++++----
 infer/src/base/IssueType.mli |  3 +++
 3 files changed, 31 insertions(+), 5 deletions(-)

diff --git a/infer/src/base/Config.ml b/infer/src/base/Config.ml
index 6ad610d3e..abd228898 100644
--- a/infer/src/base/Config.ml
+++ b/infer/src/base/Config.ml
@@ -1184,7 +1184,14 @@ and () =
     let (_ : string list ref) =
       CLOpt.mk_string_list ?deprecated ~long
         ~f:(fun issue_id ->
-          let issue = IssueType.register_from_string ~id:issue_id Linters in
+          let issue =
+            match IssueType.find_from_string ~id:issue_id with
+            | Some issue ->
+                issue
+            | None ->
+                (* unknown issue type: assume it will be defined in AL *)
+                IssueType.register_from_string ~id:issue_id Linters
+          in
           IssueType.set_enabled issue b ;
           issue_id )
         ?default ~meta:"issue_type"
diff --git a/infer/src/base/IssueType.ml b/infer/src/base/IssueType.ml
index 427256cf1..6c84731d8 100644
--- a/infer/src/base/IssueType.ml
+++ b/infer/src/base/IssueType.ml
@@ -7,6 +7,7 @@
 
 open! IStd
 module F = Format
+module L = Die
 
 (* Make sure we cannot create new issue types other than by calling [register_from_string]. This is because
      we want to keep track of the list of all the issues ever declared. *)
@@ -24,6 +25,8 @@ module Unsafe : sig
 
   val pp : F.formatter -> t -> unit
 
+  val find_from_string : id:string -> t option
+
   val register_from_string :
        ?enabled:bool
     -> ?hum:string
@@ -77,6 +80,8 @@ end = struct
 
   let set_enabled issue b = issue.enabled <- b
 
+  let find_from_string ~id:unique_id = IssueSet.find_rank !all_issues unique_id
+
   (** Avoid creating new issue types. The idea is that there are three types of issue types:
 
       + Statically pre-defined issue types, namely the ones in this module
@@ -90,10 +95,21 @@ end = struct
         of the issue type, eg in AL. *)
   let register_from_string ?(enabled = true) ?hum:hum0 ?doc_url ?linters_def_file ~id:unique_id
       checker =
-    match IssueSet.find_rank !all_issues unique_id with
-    | Some issue ->
-        (* update human-readable string in case it was supplied this time, but keep the previous
-           value of enabled (see doc comment) *)
+    match find_from_string ~id:unique_id with
+    | ((Some
+         ( { unique_id= _ (* we know it has to be the same *)
+           ; checker= checker_old
+           ; enabled= _ (* not touching this one since [Config] will have set it *)
+           ; hum= _ (* mutable field to update *)
+           ; doc_url= _ (* mutable field to update *)
+           ; linters_def_file= _ (* mutable field to update *) } as issue ))[@warning "+9"]) ->
+        (* update fields that were supplied this time around, but keep the previous values of others
+           and assert that the immutable fields are the same (see doc comment) *)
+        if not (Checker.equal checker checker_old) then
+          L.die InternalError
+            "Checker definition for issue \"%s\" doesn't match: found new checker \"%s\" but \
+             checker \"%s\" was already registered for this issue type"
+            unique_id (Checker.get_name checker) (Checker.get_name checker_old) ;
         Option.iter hum0 ~f:(fun hum -> issue.hum <- hum) ;
         if Option.is_some doc_url then issue.doc_url <- doc_url ;
         if Option.is_some linters_def_file then issue.linters_def_file <- linters_def_file ;
diff --git a/infer/src/base/IssueType.mli b/infer/src/base/IssueType.mli
index f2445dfc3..894eebf72 100644
--- a/infer/src/base/IssueType.mli
+++ b/infer/src/base/IssueType.mli
@@ -24,6 +24,9 @@ val all_issues : unit -> t list
 val pp : Format.formatter -> t -> unit
 (** pretty print a localised string *)
 
+val find_from_string : id:string -> t option
+(** return the issue type if it was previously registered *)
+
 val register_from_string :
      ?enabled:bool
   -> ?hum:string