Merge pull request #5927 from dra27/relax-warning-41

Relax warning 41 for package variables guarded by a :installed filter

Author: kit-ty-kate

master_changes.md

@@ -59,6 +59,8 @@ users)
   * Fix extraction of tarballs on Windows which contain symlinks both when those symlinks can't be created or if they point to files which don't exist [#5953 @dra27]
 
 ## Lint
+  * W41: Relax warning 41 not to trigger on uses of package variables which are guarded by a package:installed filter [#5927 @dra27]
+  * W41: Tighten w.r.t depends & depopts [#5927 @dra27]
 
 ## Repository
   * Fix download URLs containing invalid characters on Windows (e.g. the ? character in `?full_index=1`) [#5921 @dra27]
@@ -135,6 +137,7 @@ users)
   * env.win32: add regression test for reverting additions to PATH-like variables [#5935 @dra27]
   * env tests: add regression test for append/prepend operators to empty environment variables [#5925, #5935 @dra27]
   * env.win32: add regression test for handling the empty entry in PATH-like variables [#5926, #5935 @dra27]
+  * lint: add W41 examples [#5927 @dra27]
 
 ### Engine
   * Add `sort` command [#5935 @dra27]

src/format/opamFilter.mli

@@ -155,6 +155,9 @@ val commands: env -> command list -> string list list
 (** Process a simpler command, without filters *)
 val single_command: env -> arg list -> string list
 
+(** Extracts the list of variables from an argument *)
+val simple_arg_variables: simple_arg -> full_variable list
+
 (** Extracts variables appearing in a list of commands *)
 val commands_variables: command list -> full_variable list
 

src/format/opamFormula.ml

@@ -379,12 +379,13 @@ let verifies f nv =
       check_version_formula cstr (OpamPackage.version nv))
     name_formula
 
+let all_names f =
+  fold_left (fun acc (name, _) ->
+      OpamPackage.Name.Set.add name acc)
+    OpamPackage.Name.Set.empty f
+
 let packages pkgset f =
-  let names =
-    fold_left (fun acc (name, _) ->
-        OpamPackage.Name.Set.add name acc)
-      OpamPackage.Name.Set.empty f
-  in
+  let names = all_names f in
   (* dnf allows us to transform the formula into a union of intervals, where
      ignoring atoms for different package names works. *)
   let dnf = dnf_of_formula f in

src/format/opamFormula.mli

@@ -161,6 +161,9 @@ val verifies: t -> OpamPackage.t -> bool
 (** Checks if a given set of (installed) packages satisfies a formula *)
 val satisfies_depends: OpamPackage.Set.t -> t -> bool
 
+(** Returns the set of names referred to in a formula *)
+val all_names: (OpamPackage.Name.t * 'a) formula -> OpamPackage.Name.Set.t
+
 (** Returns the subset of packages possibly matching the formula (i.e. including
     all disjunction cases) *)
 val packages: OpamPackage.Set.t -> t -> OpamPackage.Set.t

src/state/opamFileTools.ml

@@ -119,6 +119,91 @@ let map_all_filters f t =
   with_deprecated_build_test (map_commands t.deprecated_build_test) |>
   with_deprecated_build_doc (map_commands t.deprecated_build_doc)
 
+(* unguarded_commands_variables is an alternative implementation of
+   OpamFilter.commands_variables which excludes package variables which are
+   guarded by an unambiguous {package:installed} filter. That is, at each level,
+   if assuming !package:installed reduces the filter to false, then the uses of
+   package:variable are not returned. This allows expressions like:
+   ["--with-foo=%{foo:share}%" {foo:installed}] or even
+   ["--with-foo"] {foo:installed & foo:bar != "baz"} not to trigger warning 41
+   if the package is not explicitly depended on. *)
+
+let unguarded_commands_variables commands =
+  let is_installed_variable filter guarded_packages v =
+    match OpamVariable.Full.package v with
+    | None -> guarded_packages
+    | (Some name) as package ->
+      let is_installed var =
+        String.equal "installed"
+          (OpamVariable.to_string (OpamVariable.Full.variable var))
+      in
+      let env var =
+        if Option.equal OpamPackage.Name.equal
+            (OpamVariable.Full.package var) package &&
+           is_installed var then
+          Some (B false)
+        else
+          None
+      in
+      if is_installed v &&
+         OpamFilter.partial_eval env filter = FBool false then
+        OpamPackage.Name.Set.add name guarded_packages
+      else
+        guarded_packages
+  in
+  let filter_guarded variables guarded_packages =
+    let is_unguarded v =
+      match OpamVariable.Full.package v with
+      | Some package ->
+        not (OpamPackage.Name.Set.mem package guarded_packages)
+      | None -> true
+    in
+    List.filter is_unguarded variables
+  in
+  let unguarded_packages_from_filter guarded_packages = function
+    | None -> guarded_packages, []
+    | Some f ->
+      let filter_variables = OpamFilter.variables f in
+      let guarded_packages =
+        List.fold_left (is_installed_variable f)
+          guarded_packages filter_variables
+      in
+      guarded_packages, filter_guarded filter_variables guarded_packages
+  in
+  let unguarded_argument_variables guarded_packages (argument, filter) =
+    let guarded_packages, filter_variables =
+      unguarded_packages_from_filter guarded_packages filter
+    in
+    let variables_from_arguments =
+      filter_guarded (OpamFilter.simple_arg_variables argument) guarded_packages
+    in
+    guarded_packages, variables_from_arguments @ filter_variables
+  in
+  let unguarded_command_variables guarded_packages (command, filter) =
+    let filter_guarded_packages, filter_variables =
+      unguarded_packages_from_filter OpamPackage.Name.Set.empty filter
+    in
+    let add_argument (guarded_packages, acc) argument =
+      let guarded_packages, unguarded_variables =
+        unguarded_argument_variables guarded_packages argument
+      in
+      guarded_packages, unguarded_variables @ acc
+    in
+    let command_guarded_packages, unguarded_variables =
+      List.fold_left add_argument (filter_guarded_packages, filter_variables)
+        command
+    in
+    OpamPackage.Name.Set.union guarded_packages command_guarded_packages,
+    unguarded_variables
+  in
+  let f (guarded_packages, acc) c =
+    let guarded_packages, unguarded_variables =
+      unguarded_command_variables guarded_packages c
+    in
+    guarded_packages, (unguarded_variables @ acc)
+  in
+  List.fold_left f (OpamPackage.Name.Set.empty, []) commands
+
 (* Returns all variables from all commands (or on given [command]) and all filters *)
 let all_variables ?exclude_post ?command t =
   let commands =
@@ -130,6 +215,18 @@ let all_variables ?exclude_post ?command t =
   List.fold_left (fun acc f -> OpamFilter.variables f @ acc)
     [] (all_filters ?exclude_post t)
 
+(* As all_variables, but any commands or arguments which are fully guarded by
+   package:installed are excluded; used for Warning 41 so that
+   ["%{foo:share}%" {foo:installed}] doesn't trigger a warning on foo *)
+let all_unguarded_variables ?exclude_post t =
+  let guarded_packages, unguarded_commands_variables =
+    unguarded_commands_variables (all_commands t)
+  in
+  guarded_packages,
+  unguarded_commands_variables @
+  List.fold_left (fun acc f -> OpamFilter.variables f @ acc)
+    [] (all_filters ?exclude_post t)
+
 let map_all_variables f t =
   let map_fld (x, flt) = x, OpamFilter.map_variables f flt in
   let map_optfld = function
@@ -456,18 +553,32 @@ let t_lint ?check_extra_files ?(check_upstream=false) ?(all=false) t =
        ~detail:alpha_flags
        (alpha_flags <> []));
 *)
-    (let undep_pkgs =
-       List.fold_left
-         (fun acc v ->
-            match OpamVariable.Full.package v with
-            | Some n when
-                t.OpamFile.OPAM.name <> Some n &&
-                not (OpamPackage.Name.Set.mem n all_depends) &&
-                OpamVariable.(Full.variable v <> of_string "installed")
-              ->
-              OpamPackage.Name.Set.add n acc
-            | _ -> acc)
-         OpamPackage.Name.Set.empty (all_variables ~exclude_post:true t)
+    (let all_mentioned_packages =
+       OpamPackage.Name.Set.union
+         (OpamFormula.all_names t.depends)
+         (OpamFormula.all_names t.depopts)
+     in
+     let undep_pkgs =
+       let guarded_packages, all_unguarded_variables =
+         all_unguarded_variables ~exclude_post:true t
+       in
+       let first_lot =
+         List.fold_left
+           (fun acc v ->
+              match OpamVariable.Full.package v with
+              | Some n when
+                  t.OpamFile.OPAM.name <> Some n &&
+                  not (OpamPackage.Name.Set.mem n all_depends) &&
+                  OpamVariable.(Full.variable v <> of_string "installed")
+                ->
+                OpamPackage.Name.Set.add n acc
+              | _ -> acc)
+           OpamPackage.Name.Set.empty all_unguarded_variables
+       in
+       let second_lot =
+         OpamPackage.Name.Set.diff guarded_packages all_mentioned_packages
+       in
+       OpamPackage.Name.Set.union first_lot second_lot
      in
      cond 41 `Warning
        "Some packages are mentioned in package scripts or features, but \

tests/reftests/lint.test

@@ -400,6 +400,33 @@ bug-reports: "https://nobug"
 messages: "foo" { bar:installed }
 ### opam lint ./lint.opam
 ${BASEDIR}/lint.opam: Passed.
+### <lint.opam>
+opam-version: "2.0"
+synopsis: "A word"
+description: "Two words."
+authors: "the testing team"
+homepage: "egapemoh"
+maintainer: "maint@tain.er"
+license: "ISC"
+dev-repo: "hg+https://to@li.nt"
+bug-reports: "https://nobug"
+depends: [
+  "dependency"
+  "guarded-dependency" {os = ""}
+]
+build: [
+  ["false"] {no-dependency-installed-only:installed}
+  ["true"] {dependency:installed}
+  ["%{guarded-dependency:share}%"] {guarded-dependency:installed}
+  ["%{guarded-dependency:share}%"] {guarded-dependency:installed & guarded-dependency:share != ""}
+  ["%{no-dependency-unguarded:share}%"] {no-dependency-unguarded:installed | no-dependency-unguarded:share != ""}
+  ["%{guarded-dependency:share}%" {guarded-dependency:installed}]
+  ["%{guarded-dependency:share}%%{no-dependency-guarded:share}%" {no-dependency-guarded:installed}] {guarded-dependency:installed}
+  ["%{guarded-dependency:share}%%{no-dependency-unguarded:share}%%{no-dependency-guarded:share}%" {no-dependency-guarded:installed}] {guarded-dependency:installed}
+]
+### opam lint ./lint.opam
+${BASEDIR}/lint.opam: Warnings.
+           warning 41: Some packages are mentioned in package scripts or features, but there is no dependency or depopt toward them: "no-dependency-guarded", "no-dependency-installed-only", "no-dependency-unguarded"
 ### : E42: The 'dev-repo:' field doesn't use version control. You should use URLs of the form "git://", "git+https://", "hg+https://"...
 ### <lint.opam>
 opam-version: "2.0"