Detection and warning for common typos in package dependency constraints #11600

kemsguy7 · 2025-04-03T08:43:28Z

@gridbugs, Here's my draft PR for issue. #11575, please kindly review and give feedback let me know if I'm on track.

gridbugs

Nice work so far! In addition to the comments, please add some cram tests (see the contents of test/blackbox-tests/test-cases/ for examples).

gridbugs · 2025-04-04T01:29:42Z

README.md

Avoid including unrelated changes in PRs. If you want to update the readme file, it's more appropriate to do it in a separate PR.

gridbugs · 2025-04-04T01:31:19Z

boot/libs.ml

@@ -1,4 +1,4 @@
-let external_libraries = [ "unix"; "threads" ]
+let external_libraries = [ "threads.posix" ]


Dune's build process is a little annoying in that it sometimes changes this file. Get into the habit of not staging changes to this file when making commits to this repo.

gridbugs · 2025-04-04T01:34:11Z

class.sol

What is this file? Please remove it from the PR.

ok my bad, will do

gridbugs · 2025-04-04T01:39:54Z

src/dune_pkg/package_dependency.ml

+(* Function to emit warnings for common typos when processing dependencies *)
+let warn_on_typos ~loc dependency =
+  (* We'll implement a copy of the check_for_version_typo function here 
+     since it's not directly accessible from the other module *)


See if you can come up with a way to avoid this duplication. You're welcome to extend the interface to Dune_lange.Package_dependency if necessary.

gridbugs · 2025-04-04T01:44:57Z

src/dune_lang/package_dependency.ml

+      ( sprintf
+          "Possible typo in package dependency for %s: '(= version)' might be a mistake."
+          (Package_name.to_string name)
+      , "Did you mean to use the `:version` variable instead? Use: (depends (bar :version))" )


In the example, instead of (depends (bar :version)) it would be better to reproduce the text of the constraint the user actually used, but corrected.

gridbugs · 2025-04-04T01:57:20Z

src/dune_lang/package_dependency.ml

+    (* Check for typos and emit warnings *)
+    (match check_for_version_typo result with
+     | Some (msg, suggestion) ->
+       User_warning.emit [Pp.text msg; Pp.text suggestion]


Take a look at User_message.make. You can create a message, possibly including hints which are formatted differently from the regular warning body and would be suitable for the "Did you mean..." parts of the warning. The message can then be printed as a warning with User_warning.emit_message.

So concretely I'm suggesting that you change the check_for_version_type function to return a User_message.t option.

gridbugs · 2025-04-04T01:59:20Z

src/dune_lang/package_dependency.ml

+          "Possible typo in package dependency for %s: '(= version)' might be a mistake."
+          (Package_name.to_string name)
+      , "Did you mean to use the `:version` variable instead? Use: (depends (bar :version))" )
+  | Some (Bop (Relop.Eq, Value.String_literal "version", _)) ->


I think we only expect version comparisons to use the Uop constructor.

gridbugs · 2025-04-04T02:01:24Z

src/dune_lang/package_dependency.ml

+  | Some (Uop (Relop.Eq, Value.String_literal "version")) ->
+    Some
+      ( sprintf
+          "Possible typo in package dependency for %s: '(= version)' might be a mistake."


I think it would be more accurate to say "Possible typo in constraint for dependency %S: ...".

gridbugs · 2025-04-04T02:03:34Z

src/dune_pkg/package_dependency.ml

+          (match Constraint.opt_of_opam_condition condition with
+           | Ok constraint_ -> 
+              let dep = { name; constraint_ } in
+              warn_on_typos ~loc dep;


Will this print warnings for projects that don't use dune package management?

I think printing the warning in decode is sufficient, since for all operations dune will need to decode the dune-project file anyway.

I'll look at it again and give feedback.

@gridbugs , I've added fixes based on your suggestions apart from cram tests, I'm currently working on that. Please kindly review and let me know if I'm on track, thanks.

gridbugs

Looking pretty good. I've left several minor suggestions. Looking forward to seeing the tests.

gridbugs · 2025-04-07T02:16:41Z

README.md

Try to keep the diff to just contain the meaningful part of your change (e.g. explicitly stage individual files rather than doing git commit -a).

gridbugs · 2025-04-07T02:20:33Z

src/dune_lang/package_dependency.ml

@@ -63,4 +96,4 @@ let to_dyn { name; constraint_ } =
 let equal { name; constraint_ } t =
  Package_name.equal name t.name
  && Option.equal Package_constraint.equal constraint_ t.constraint_
-;;
+;;


Try to avoid adding/removing the trailing newline from the diff (ie. only include the meaningful part of your change). Some editors automatically mess with file endings. The problem with checking this in is that the next person who touches the file will have to deal with the changed line ending from their editor and so on. My advice would be to configure your editor to leave file endings alone, and also to use git add -p or similar to only stage the meaningful changes.

gridbugs · 2025-04-07T02:22:53Z

src/dune_lang/package_dependency.ml

+            "Possible typo in constraint for dependency %S: '(= version)' might be a mistake."
+            (Package_name.to_string name)
+        ]
+        ~hints:[ Pp.text "Did you mean to use the `:version` variable instead? Example: (depends (bar :version))" ]


Instead of (depends (bar :version)), it would be better to print the actual package name from the user's dune-project. Also (depends (bar :version)) isn't the right syntax; it should be (depends (bar (= :version))).

gridbugs · 2025-04-07T02:23:34Z

src/dune_lang/package_dependency.ml

+            "Possible typo in constraint for dependency %S: ':with_test' might be a mistake."
+            (Package_name.to_string name)
+        ]
+        ~hints:[ Pp.text "Did you mean to use ':with-test' instead? Example: (depends (bar :with-test))" ]


Replace the bar with the package name from the offending line.

gridbugs · 2025-04-07T02:24:11Z

src/dune_pkg/package_dependency.ml

-         | Ok constraint_ -> { name; constraint_ }
+         | Ok constraint_ -> 
+            let dep = { name; constraint_ } in
+            (* We don't need to check for typos here since it's already handled in decode *)


I don't think this comment is necessary.

@gridbugs Thanks for the feedback, I've noted all requested changes and will adjust accordingly.

@gridbugs

, here are my updates, attached to this comment is a screenshot showing the result of running the test

Nice. Please include the test in this PR.

Nice. Please include the test in this PR.

Added , please kindly review

gridbugs · 2025-04-07T06:18:02Z

src/dune_pkg/package_dependency.ml

@@ -229,7 +229,9 @@ let list_of_opam_disjunction loc filtered_formula =
      | Atom (name, condition) ->
        let name = Package_name.of_opam_package_name name in
        (match Constraint.opt_of_opam_condition condition with
-         | Ok constraint_ -> { name; constraint_ }
+         | Ok constraint_ -> 
+            let dep = { name; constraint_ } in   


There's no need for this let binding anymore. You can just revert this file to its original state.

There's no need for this let binding anymore. You can just revert this file to its original state.

@gridbugs Updated.

gridbugs · 2025-04-08T07:02:01Z

src/dune_pkg/package_dependency.ml

@@ -229,7 +229,7 @@ let list_of_opam_disjunction loc filtered_formula =
      | Atom (name, condition) ->
        let name = Package_name.of_opam_package_name name in
        (match Constraint.opt_of_opam_condition condition with
-         | Ok constraint_ -> { name; constraint_ }
+         | Ok constraint_ -> {name; constraint_}


Run dune fmt to automatically fix the style.

Alright @gridbugs, thanks for your guiding me all the way. Implementing final changes now.

gridbugs · 2025-04-08T07:05:20Z

This is getting pretty close to done. A few things left before I'll consider merging it:

undo your changes to the readme
fix the style (run dune fmt)
squash all the commits into a single commit, and sign that commit (ie. git commit -s ...)

I'll mark this as not a draft so others can take a look if they want.

kemsguy7 · 2025-04-09T10:59:33Z

This is getting pretty close to done. A few things left before I'll consider merging it:

undo your changes to the readme

fix the style (run dune fmt)

squash all the commits into a single commit, and sign that commit (ie. git commit -s ...)

I'll mark this as not a draft so others can take a look if they want.

@gridbugs all done, please kindly review

gridbugs · 2025-04-10T00:48:13Z

README.md

Please remove unrelated changes from the readme.

gridbugs · 2025-04-10T00:49:34Z

Looks like your commit is not signed (the "DCO" test is failing). Run git commit --amend -s.

kemsguy7 · 2025-04-10T00:56:20Z

Looks like your commit is not signed (the "DCO" test is failing). Run git commit --amend -s.

Okay thanks, let me try again.

gridbugs · 2025-04-10T01:07:40Z

One more thing I'll ask of you is to add a file to doc/changes describing the change (follow the example of the other files in that directory).

kemsguy7 · 2025-04-10T01:09:44Z

One more thing I'll ask of you is to add a file to doc/changes describing the change (follow the example of the other files in that directory).

Alright mentor, on it.

kemsguy7 · 2025-04-11T08:35:52Z

One more thing I'll ask of you is to add a file to doc/changes describing the change (follow the example of the other files in that directory).

@gridbugs I've added the docs. Please kindly review. Thanks.

gridbugs · 2025-04-14T01:59:05Z

doc/changes/11575.md

Thanks for the detailed explanation, but this just needed to be a single line for our change log. You're welcome to write more docs elsewhere but I think for this particular change it won't be necessary. I'll fix up this file to match other changelog entries before and merge this PR today.

Alright, noted with thanks.

kemsguy7 · 2025-04-15T00:25:57Z

@gridbugs , added updates, please kindly review, thanks

Signed-off-by: kemsguy7 <[email protected]>

maiste · 2025-04-15T08:03:09Z

doc/changes/11600.md

Since this is related to package management, shouldn't we move this to an experimental subfolder or not add a changelog?

gridbugs requested changes Apr 4, 2025

View reviewed changes

gridbugs requested changes Apr 7, 2025

View reviewed changes

gridbugs reviewed Apr 7, 2025

View reviewed changes

gridbugs reviewed Apr 8, 2025

View reviewed changes

gridbugs marked this pull request as ready for review April 8, 2025 07:05

kemsguy7 force-pushed the kemsguy7/typo-detection branch 2 times, most recently from efdef19 to d7af4b7 Compare April 8, 2025 17:32

gridbugs reviewed Apr 10, 2025

View reviewed changes

README.md Outdated

Copy link

Collaborator

gridbugs Apr 10, 2025

Choose a reason for hiding this comment

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

Please remove unrelated changes from the readme.

kemsguy7 force-pushed the kemsguy7/typo-detection branch from d7af4b7 to e62f5df Compare April 10, 2025 01:31

gridbugs reviewed Apr 14, 2025

View reviewed changes

gridbugs approved these changes Apr 14, 2025

View reviewed changes

gridbugs force-pushed the kemsguy7/typo-detection branch from 8ab32bc to debedeb Compare April 14, 2025 02:00

gridbugs changed the title ~~Kemsguy7/typo detection~~ Detection and warning for common typos in package dependency constraints Apr 14, 2025

gridbugs force-pushed the kemsguy7/typo-detection branch 2 times, most recently from 4eeb981 to 97f4491 Compare April 14, 2025 04:03

kemsguy7 force-pushed the kemsguy7/typo-detection branch from 61aca2c to c6bb552 Compare April 15, 2025 00:24

Implement warning for common typos in package deps

38b9b8d

Signed-off-by: kemsguy7 <[email protected]>

gridbugs force-pushed the kemsguy7/typo-detection branch from c6bb552 to 38b9b8d Compare April 15, 2025 04:24

gridbugs merged commit df45715 into ocaml:main Apr 15, 2025
24 of 25 checks passed

maiste reviewed Apr 15, 2025

View reviewed changes

maiste mentioned this pull request Apr 15, 2025

updated dune_lint #11628

Closed

gridbugs mentioned this pull request Apr 15, 2025

Allow disabling the warning when dune detects a common typo #11629

Open

		@@ -1,4 +1,4 @@
		let external_libraries = [ "unix"; "threads" ]
		let external_libraries = [ "threads.posix" ]

Detection and warning for common typos in package dependency constraints #11600

Detection and warning for common typos in package dependency constraints #11600

Conversation

kemsguy7 commented Apr 3, 2025 • edited Loading

gridbugs left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gridbugs left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kemsguy7 Apr 7, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gridbugs commented Apr 8, 2025

kemsguy7 commented Apr 9, 2025

Choose a reason for hiding this comment

gridbugs commented Apr 10, 2025

kemsguy7 commented Apr 10, 2025

gridbugs commented Apr 10, 2025

kemsguy7 commented Apr 10, 2025 • edited Loading

kemsguy7 commented Apr 11, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kemsguy7 commented Apr 15, 2025

Choose a reason for hiding this comment

kemsguy7 commented Apr 3, 2025 •

edited

Loading

kemsguy7 Apr 7, 2025 •

edited

Loading

kemsguy7 commented Apr 10, 2025 •

edited

Loading

kemsguy7 commented Apr 11, 2025 •

edited

Loading