8000 feat: add configurable typo detection for package dependency constraints by kemsguy7 · Pull Request #11759 · ocaml/dune · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

feat: add configurable typo detection for package dependency constraints #11759

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
+115 −43
Open

feat: add configurable typo detection for package dependency constraints #11759

Show file tree
Hide file tree
Changes from all commits
Commits
8000
Show all changes
2 commits
Select commit Hold shift + click to select a range
50c69a1
Implement disabled warnings configuration and fixed merge conflicts w…
kemsguy7 May 20, 2025
659a6dc
fix-update: updated package dependency not to catch exceptions and re…
kemsguy7 Jun 19, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions src/dune_config/config.ml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,7 @@ let make ~name ~of_string ~default =
;;

let make_toggle ~name ~default = make ~name ~default ~of_string:Toggle.of_string

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please try to remember to run the autoformatter on your changes. I thought we set up your vscode to automatically format files when you save them. Did that stop working?

let global_lock = make ~name:"global_lock" ~of_string:Toggle.of_string ~default:`Enabled

let cutoffs_that_reduce_concurrency_in_watch_mode =
Expand Down Expand Up @@ -147,3 +148,7 @@ let threaded_console_frames_per_second =
| None -> Error (sprintf "could not parse %S as an integer" x))
~default:`Default
;;

let typo_warnings =
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this should have a more descriptive name, as it only enables checking for a very specific type of typo.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you mean this file config.ml?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mean the variable typo_warnings.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This isn't resolved. The variable is still named typo_warnings. It should have a more descriptive name reflecting the nature of typos which it enables warnings for, since it only warns about typos in a very specific situation.

make ~name:"typo_warnings" ~of_string:Toggle.of_string ~default:`Enabled
;;
5 changes: 4 additions & 1 deletion src/dune_config/config.mli
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,10 +60,13 @@ val threaded_console : Toggle.t t
(** The number of frames per second for the threaded console. *)
val threaded_console_frames_per_second : [ `Default | `Custom of int ] t

(** Control whether to show warnings for common typos in package dependencies *)
val typo_warnings : Toggle.t t

(** Before any configuration value is accessed, this function must be called
with all the configuration values from the relevant config file
([dune-workspace], or [dune-config]).

Note that environment variables take precedence over the values passed here
for easy overriding. *)
val init : (Loc.t * string) String.Map.t -> unit
val init : (Loc.t * string) String.Map.t -> unit
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please do not remove the trailing newlines. It creates changed lines where there are no changes actually.

88 changes: 46 additions & 42 deletions src/dune_lang/package_dependency.ml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,57 +28,62 @@ module Well_formed_name = struct
module TT = Dune_util.Stringlike.Make (T)

let decode =
let open Decoder in
let open Dune_sexp.Decoder in
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did something change to make this addition necessary now?

TT.decode >>| T.to_package_name
;;
end

let encode { name; constraint_ } =
let open Encoder in
let open Dune_sexp.Encoder in
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this change necessary?

Copy link
Contributor Author
@kemsguy7 kemsguy7 Jun 19, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

reverted.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I still see the change in the diff. Can you remove this change (and the one above that I commented on).

match constraint_ with
| None -> Package_name.encode name
| Some c -> pair Package_name.encode Package_constraint.encode (name, c)
;;

(* Check for common typos in package dependency constraints *)
let check_for_typo ~loc { name; constraint_ } =
let open Package_constraint in
match constraint_ with
| Some (Uop (Relop.Eq, Value.String_literal "version")) ->
let message =
User_message.make
~loc
[ Pp.textf
"Possible typo in constraint for dependency %S: '(= version)' might be a \
mistake."
(Package_name.to_string name)
]
~hints:
[ Pp.textf
"Did you mean to use the `:version` variable instead? Example: (depends \
(%s (= :version)))"
(Package_name.to_string name)
]
in
Some message
| Some (Bvar var) when String.equal (Package_variable_name.to_string var) "with_test" ->
let message =
User_message.make
~loc
[ Pp.textf
"Possible typo in constraint for dependency %S: ':with_test' might be a \
mistake."
(Package_name.to_string name)
]
~hints:
[ Pp.textf
"Did you mean to use ':with-test' instead? Example: (depends (%s \
:with-test))"
(Package_name.to_string name)
]
in
Some message
| _ -> None
let dependency_constraint_variable_typo_warnings ~loc { name; constraint_ } =
match Dune_config.Config.(get typo_warnings) with
| `Enabled ->
(match constraint_ with
| Some
(Package_constraint.Uop
(Relop.Eq, Package_constraint.Value.String_literal "version")) ->
let message =
User_message.make
~loc
[ Pp.textf
"Possible typo in constraint for dependency %S: '(= version)' might be a \
mistake."
(Package_name.to_string name)
]
~hints:
[ Pp.textf
"Did you mean to use the `:version` variable instead? Example: (depends \
(%s (= :version)))"
(Package_name.to_string name)
]
in
Some message
| Some (Package_constraint.Bvar var)
when String.equal (Package_variable_name.to_string var) "with_test" ->
let message =
User_message.make
~loc
[ Pp.textf
"Possible typo in constraint for dependency %S: ':with_test' might be a \
mistake."
(Package_name.to_string name)
]
~hints:
[ Pp.textf
"Did you mean to use ':with-test' instead? Example: (depends (%s \
:with-test))"
(Package_name.to_string name)
]
in
Some message
| _ -> None)
| `Disabled -> None
;;

let decode =
Expand All @@ -88,8 +93,7 @@ let decode =
F438 and+ name = Package_name.decode
and+ expr = Package_constraint.decode in
let result = { name; constraint_ = Some expr } in
(* Check for typos and emit warnings *)
(match check_for_typo ~loc result with
(match dependency_constraint_variable_typo_warnings ~loc result with
| Some msg -> User_warning.emit_message msg
| None -> ());
result
Expand Down
60 changes: 60 additions & 0 deletions test/blackbox-tests/test-cases/disabled_warning.t
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,60 @@
Test detection and disabling of common typos in package dependencies

First, create a dune-project file with typos to see warnings with default settings (warnings enabled)
$ cat > dune-project <<EOF
> (lang dune 3.11)
> (package
> (name foo)
> (allow_empty)
> (depends
> (bar (= version)) ; Should detect typo and suggest :version
> (baz :with_test) ; Should detect typo and suggest :with-test
> qux))
> EOF

$ dune pkg lock
File "dune-project", line 6, characters 3-20:
6 | (bar (= version)) ; Should detect typo and suggest :version
^^^^^^^^^^^^^^^^^
Warning: Possible typo in constraint for dependency "bar": '(= version)' might be a mistake.
Hint: Did you mean to use the `:version` variable instead? Example: (depends (bar (= :version)))
File "dune-project", line 7, characters 3-19:
7 | (baz :with_test) ; Should detect typo and suggest :with-test
^^^^^^^^^^^^^^^^
Warning: Possible typo in constraint for dependency "baz": ':with_test' might be a mistake.
Hint: Did you mean to use ':with-test' instead? Example: (depends (baz :with-test))

Now demonstrate that fixed dependencies don't cause warnings
$ cat > fixed-dune-project <<EOF
> (lang dune 3.11)
> (package
> (name foo)
> (allow_empty)
> (depends
> (bar (= :version)) ; Fixed version
> (baz :with-test) ; Fixed with-test
> qux))
> EOF

$ dune pkg lock --project-file=fixed-dune-project

Now test that we can disable warnings via the environment variable
$ DUNE_CONFIG__TYPO_WARNINGS=disabled dune pkg lock

$ # Create clean directory for next test
$ mkdir disable_test
$ cd disable_test

$ cat > dune-project <<EOF
> (lang dune 3.11)
> (package
> (name foo)
> (allow_empty)
> (depends
> (bar (= version)) ; Should NOT show warning with typo_warnings disabled
> (baz :with_test) ; Should NOT show warning with typo_warnings disabled
> qux))
> EOF

$ DUNE_CONFIG__TYPO_WARNINGS=disabled dune pkg lock
$ # No warnings should be produced when the feature is disabled
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is no command being run here, so it is missing the test.

Loading
0