diff --git a/src/dune_lang/package.ml b/src/dune_lang/package.ml index 091d4c9741a..9b2c062c786 100644 --- a/src/dune_lang/package.ml +++ b/src/dune_lang/package.ml @@ -13,6 +13,14 @@ type original_opam_file = ; contents : string } +module Duplicate_dep_warning = struct + type t = + { loc : Loc.t + ; dep_string : string + ; field_name : string + } +end + type t = { id : Id.t ; opam_file : Path.Source.t @@ -31,6 +39,7 @@ type t = ; allow_empty : bool ; original_opam_file : original_opam_file option ; exclusive_dir : (Loc.t * Path.Source.t) option + ; duplicate_dep_warnings : Duplicate_dep_warning.t list } (* Package name are globally unique, so we can reasonably expect that there will @@ -55,6 +64,7 @@ let original_opam_file t = t.original_opam_file let set_inside_opam_dir t ~dir = { t with opam_file = Name.file t.id.name ~dir } let set_version_and_info t ~version ~info = { t with version; info } let exclusive_dir t = t.exclusive_dir +let duplicate_dep_warnings t = t.duplicate_dep_warnings let encode (name : Name.t) @@ -75,6 +85,7 @@ let encode ; opam_file = _ ; original_opam_file = _ ; exclusive_dir + ; duplicate_dep_warnings = _ } = let open Encoder in @@ -121,6 +132,25 @@ let decode = ; Pp.textf "- %s" (print_value loc2) ] in + let collect_duplicate_deps deps field_name = + let warnings, _ = + List.fold_left deps ~init:([], []) ~f:(fun (warnings, seen) (loc, dep) -> + let is_duplicate = + List.exists seen ~f:(fun (_, seen_dep) -> + Package_name.equal + dep.Package_dependency.name + seen_dep.Package_dependency.name) + in + if is_duplicate + then ( + let dep_sexp = Package_dependency.encode dep in + let dep_string = Dune_sexp.to_string dep_sexp in + let warning = { Duplicate_dep_warning.loc; dep_string; field_name } in + warning :: warnings, (loc, dep) :: seen) + else warnings, (loc, dep) :: seen) + in + warnings + in fun ~dir -> fields @@ let* version = Syntax.get_exn Stanza.syntax in @@ -130,9 +160,12 @@ let decode = and+ description = field_o "description" string and+ version = field_o "version" (Syntax.since Stanza.syntax (2, 5) >>> Package_version.decode) - and+ depends = field ~default:[] "depends" (repeat Package_dependency.decode) - and+ conflicts = field ~default:[] "conflicts" (repeat Package_dependency.decode) - and+ depopts = field ~default:[] "depopts" (repeat Package_dependency.decode) + and+ depends_with_locs = + field ~default:[] "depends" (repeat (located Package_dependency.decode)) + and+ conflicts_with_locs = + field ~default:[] "conflicts" (repeat (located Package_dependency.decode)) + and+ depopts_with_locs = + field ~default:[] "depopts" (repeat (located Package_dependency.decode)) and+ info = Package_info.decode ~since:(2, 0) () and+ tags = field "tags" (enter (repeat string)) ~default:[] and+ exclusive_dir = @@ -161,6 +194,16 @@ let decode = and+ allow_empty = field_b "allow_empty" ~check:(Syntax.since Stanza.syntax (3, 0)) and+ lang_version = Syntax.get_exn Stanza.syntax in let allow_empty = lang_version < (3, 0) || allow_empty in + let duplicate_dep_warnings = + List.concat + [ collect_duplicate_deps depends_with_locs "depends" + ; collect_duplicate_deps conflicts_with_locs "conflicts" + ; collect_duplicate_deps depopts_with_locs "depopts" + ] + in + let depends = List.map ~f:snd depends_with_locs in + let conflicts = List.map ~f:snd conflicts_with_locs in + let depopts = List.map ~f:snd depopts_with_locs in let id = Id.create ~name ~dir in let opam_file = Name.file id.name ~dir:id.dir in { id @@ -180,6 +223,7 @@ let decode = ; opam_file ; original_opam_file = None ; exclusive_dir + ; duplicate_dep_warnings } ;; @@ -208,6 +252,7 @@ let to_dyn ; opam_file = _ ; original_opam_file = _ ; exclusive_dir = _ + ; duplicate_dep_warnings = _ } = let open Dyn in @@ -272,5 +317,6 @@ let create ; original_opam_file ; exclusive_dir = Option.map contents_basename ~f:(fun (loc, s) -> loc, Path.Source.relative dir s) + ; duplicate_dep_warnings = [] } ;; diff --git a/src/dune_lang/package.mli b/src/dune_lang/package.mli index 315b7d494f9..a2d65841453 100644 --- a/src/dune_lang/package.mli +++ b/src/dune_lang/package.mli @@ -74,3 +74,13 @@ val create -> t val original_opam_file : t -> original_opam_file option + +module Duplicate_dep_warning : sig + type t = + { loc : Loc.t + ; dep_string : string + ; field_name : string + } +end + +val duplicate_dep_warnings : t -> Duplicate_dep_warning.t list diff --git a/src/dune_rules/gen_rules.ml b/src/dune_rules/gen_rules.ml index 0228a5e30eb..56c722c76d1 100644 --- a/src/dune_rules/gen_rules.ml +++ b/src/dune_rules/gen_rules.ml @@ -335,6 +335,11 @@ let missing_project_name = ~since:(3, 11) ;; +(* Warn about duplicate dependencies in package definitions *) +let duplicate_deps = + Warning.make ~default:(fun _ -> `Enabled) ~name:"duplicate_deps" ~since:(3, 18) +;; + (* To be called once per project, when we are generating the rules for the root directory of the project *) let gen_project_rules = @@ -386,6 +391,28 @@ let gen_project_rules = to your dune-project file to make sure that $ dune subst works in \ release or pinned builds" ])) + and+ () = + (* Emit warnings for duplicate dependencies in packages *) + let packages = Dune_project.packages project in + Dune_lang.Package.Name.Map.values packages + |> Memo.parallel_iter ~f:(fun pkg -> + Dune_lang.Package.duplicate_dep_warnings pkg + |> Memo.parallel_iter + ~f:(fun (warning : Dune_lang.Package.Duplicate_dep_warning.t) -> + Warning_emit.emit + duplicate_deps + (Warning_emit.Context.project project) + (fun () -> + Memo.return + (User_message.make + ~loc:warning.loc + [ Pp.textf + "Duplicate dependency on package %s in '%s' field. If you \ + want to specify multiple constraints, combine them using \ + (and ...)." + warning.dep_string + warning.field_name + ])))) in () in diff --git a/test/blackbox-tests/test-cases/dune-project-meta/basic-generate.t b/test/blackbox-tests/test-cases/dune-project-meta/basic-generate.t index 2a9bb1217bd..30f0ff38317 100644 --- a/test/blackbox-tests/test-cases/dune-project-meta/basic-generate.t +++ b/test/blackbox-tests/test-cases/dune-project-meta/basic-generate.t @@ -54,6 +54,36 @@ The `dune build` should generate the opam file > EOF $ dune build @install + File "dune-project", line 21, characters 2-23: + 21 | (fieldslib (< v0.13)))) + ^^^^^^^^^^^^^^^^^^^^^ + Warning: Duplicate dependency on package (fieldslib (< v0.13)) in 'depends' + field. If you want to specify multiple constraints, combine them using (and + ...). + Hint: To disable this warning, add the following to your dune-project file: + (warnings (duplicate_deps disabled)) + File "dune-project", line 19, characters 2-17: + 19 | (uri (< 2.0.0)) + ^^^^^^^^^^^^^^^ + Warning: Duplicate dependency on package (uri (< 2.0.0)) in 'depends' field. + If you want to specify multiple constraints, combine them using (and ...). + Hint: To disable this warning, add the following to your dune-project file: + (warnings (duplicate_deps disabled)) + File "dune-project", line 33, characters 2-19: + 33 | (async (< v0.12)))) + ^^^^^^^^^^^^^^^^^ + Warning: Duplicate dependency on package (async (< v0.12)) in 'depends' + field. If you want to specify multiple constraints, combine them using (and + ...). + Hint: To disable this warning, add the following to your dune-project file: + (warnings (duplicate_deps disabled)) + File "dune-project", line 46, characters 2-17: + 46 | (lwt (< 6.0.0)))) + ^^^^^^^^^^^^^^^ + Warning: Duplicate dependency on package (lwt (< 6.0.0)) in 'depends' field. + If you want to specify multiple constraints, combine them using (and ...). + Hint: To disable this warning, add the following to your dune-project file: + (warnings (duplicate_deps disabled)) $ cat cohttp.opam # This file is generated by dune, edit dune-project instead diff --git a/test/blackbox-tests/test-cases/dune-project-meta/duplicate-dependencies.t b/test/blackbox-tests/test-cases/dune-project-meta/duplicate-dependencies.t new file mode 100644 index 00000000000..e0c77936e62 --- /dev/null +++ b/test/blackbox-tests/test-cases/dune-project-meta/duplicate-dependencies.t @@ -0,0 +1,241 @@ +Duplicate dependencies detection +================================= + +Exact duplicate without constraints should warn +------------------------------------------------ + + $ cat >dune-project < (lang dune 3.0) + > (name test-pkg) + > (package + > (name test-pkg) + > (allow_empty) + > (depends ocaml ocaml)) + > EOF + + $ dune build + File "dune-project", line 6, characters 16-21: + 6 | (depends ocaml ocaml)) + ^^^^^ + Warning: Duplicate dependency on package ocaml in 'depends' field. If you + want to specify multiple constraints, combine them using (and ...). + Hint: To disable this warning, add the following to your dune-project file: + (warnings (duplicate_deps disabled)) + +Exact duplicate with same constraint should warn +------------------------------------------------- + + $ cat >dune-project < (lang dune 3.0) + > (name test-pkg) + > (package + > (name test-pkg) + > (allow_empty) + > (depends (ocaml (>= 4.08)) (ocaml (>= 4.08)))) + > EOF + + $ dune build + File "dune-project", line 6, characters 28-45: + 6 | (depends (ocaml (>= 4.08)) (ocaml (>= 4.08)))) + ^^^^^^^^^^^^^^^^^ + Warning: Duplicate dependency on package (ocaml (>= 4.08)) in 'depends' + field. If you want to specify multiple constraints, combine them using (and + ...). + Hint: To disable this warning, add the following to your dune-project file: + (warnings (duplicate_deps disabled)) + +Same package with different constraints should warn +---------------------------------------------------- + +Multiple constraints on the same package should be combined using (and ...). + + $ cat >dune-project < (lang dune 3.0) + > (name test-pkg) + > (package + > (name test-pkg) + > (allow_empty) + > (depends (ocaml (>= 4.08)) (ocaml (< 5.0)))) + > EOF + + $ dune build + File "dune-project", line 6, characters 28-43: + 6 | (depends (ocaml (>= 4.08)) (ocaml (< 5.0)))) + ^^^^^^^^^^^^^^^ + Warning: Duplicate dependency on package (ocaml (< 5.0)) in 'depends' field. + If you want to specify multiple constraints, combine them using (and ...). + Hint: To disable this warning, add the following to your dune-project file: + (warnings (duplicate_deps disabled)) + +Multiple duplicates in the same field +-------------------------------------- + + $ cat >dune-project < (lang dune 3.0) + > (name test-pkg) + > (package + > (name test-pkg) + > (allow_empty) + > (depends ocaml ocaml dune dune)) + > EOF + + $ dune build + File "dune-project", line 6, characters 27-31: + 6 | (depends ocaml ocaml dune dune)) + ^^^^ + Warning: Duplicate dependency on package dune in 'depends' field. If you want + to specify multiple constraints, combine them using (and ...). + Hint: To disable this warning, add the following to your dune-project file: + (warnings (duplicate_deps disabled)) + File "dune-project", line 6, characters 16-21: + 6 | (depends ocaml ocaml dune dune)) + ^^^^^ + Warning: Duplicate dependency on package ocaml in 'depends' field. If you + want to specify multiple constraints, combine them using (and ...). + Hint: To disable this warning, add the following to your dune-project file: + (warnings (duplicate_deps disabled)) + +Duplicates in conflicts field +------------------------------ + + $ cat >dune-project < (lang dune 3.0) + > (name test-pkg) + > (package + > (name test-pkg) + > (allow_empty) + > (conflicts base base)) + > EOF + + $ dune build + File "dune-project", line 6, characters 17-21: + 6 | (conflicts base base)) + ^^^^ + Warning: Duplicate dependency on package base in 'conflicts' field. If you + want to specify multiple constraints, combine them using (and ...). + Hint: To disable this warning, add the following to your dune-project file: + (warnings (duplicate_deps disabled)) + +Duplicates in depopts field +---------------------------- + + $ cat >dune-project < (lang dune 3.0) + > (name test-pkg) + > (package + > (name test-pkg) + > (allow_empty) + > (depopts lwt lwt)) + > EOF + + $ dune build + File "dune-project", line 6, characters 14-17: + 6 | (depopts lwt lwt)) + ^^^ + Warning: Duplicate dependency on package lwt in 'depopts' field. If you want + to specify multiple constraints, combine them using (and ...). + Hint: To disable this warning, add the following to your dune-project file: + (warnings (duplicate_deps disabled)) + +Mix of valid and invalid duplicates +------------------------------------ + +Even different constraints on the same package are now warned about. + + $ cat >dune-project < (lang dune 3.0) + > (name test-pkg) + > (package + > (name test-pkg) + > (allow_empty) + > (depends + > (ocaml (>= 4.08)) + > (ocaml (< 5.0)) + > (dune (>= 2.0)) + > (dune (>= 2.0)))) + > EOF + + $ dune build + File "dune-project", line 10, characters 2-17: + 10 | (dune (>= 2.0)))) + ^^^^^^^^^^^^^^^ + Warning: Duplicate dependency on package (dune (>= 2.0)) in 'depends' field. + If you want to specify multiple constraints, combine them using (and ...). + Hint: To disable this warning, add the following to your dune-project file: + (warnings (duplicate_deps disabled)) + File "dune-project", line 8, characters 2-17: + 8 | (ocaml (< 5.0)) + ^^^^^^^^^^^^^^^ + Warning: Duplicate dependency on package (ocaml (< 5.0)) in 'depends' field. + If you want to specify multiple constraints, combine them using (and ...). + Hint: To disable this warning, add the following to your dune-project file: + (warnings (duplicate_deps disabled)) + +Triple duplicate +---------------- + +When the same dependency appears three times, each duplicate is reported. + + $ cat >dune-project < (lang dune 3.0) + > (name test-pkg) + > (package + > (name test-pkg) + > (allow_empty) + > (depends ocaml ocaml ocaml)) + > EOF + + $ dune build + File "dune-project", line 6, characters 22-27: + 6 | (depends ocaml ocaml ocaml)) + ^^^^^ + Warning: Duplicate dependency on package ocaml in 'depends' field. If you + want to specify multiple constraints, combine them using (and ...). + Hint: To disable this warning, add the following to your dune-project file: + (warnings (duplicate_deps disabled)) + File "dune-project", line 6, characters 16-21: + 6 | (depends ocaml ocaml ocaml)) + ^^^^^ + Warning: Duplicate dependency on package ocaml in 'depends' field. If you + want to specify multiple constraints, combine them using (and ...). + Hint: To disable this warning, add the following to your dune-project file: + (warnings (duplicate_deps disabled)) + +Duplicate with filter constraint +--------------------------------- + + $ cat >dune-project < (lang dune 3.0) + > (name test-pkg) + > (package + > (name test-pkg) + > (allow_empty) + > (depends (alcotest :with-test) (alcotest :with-test))) + > EOF + + $ dune build + File "dune-project", line 6, characters 32-53: + 6 | (depends (alcotest :with-test) (alcotest :with-test))) + ^^^^^^^^^^^^^^^^^^^^^ + Warning: Duplicate dependency on package (alcotest :with-test) in 'depends' + field. If you want to specify multiple constraints, combine them using (and + ...). + Hint: To disable this warning, add the following to your dune-project file: + (warnings (duplicate_deps disabled)) + +Disabling the warning +--------------------- + +The warning can be disabled using the (warnings ...) field in dune-project. + + $ cat >dune-project < (lang dune 3.18) + > (name test-pkg) + > (warnings (duplicate_deps disabled)) + > (package + > (name test-pkg) + > (allow_empty) + > (depends ocaml ocaml)) + > EOF + + $ dune build