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

kemsguy7 · 2025-05-05T12:47:41Z

No description provided.

Leonidas-from-XIV

Looks overall pretty good, but I have some suggestions that could improve readability of the code.

src/dune_config/config.ml

src/dune_lang/package_dependency.ml

test/blackbox-tests/test-cases/typo-detection.t

kemsguy7 · 2025-05-05T12:57:50Z

@gridbugs , here's updates I made for issue #11629 .

test/blackbox-tests/test-cases/typo-detection2.t

kemsguy7 · 2025-05-07T04:46:15Z

@Leonidas-from-XIV @gridbugs , I've created updates for this pr here: #11766 . Please kindly review.

Leonidas-from-XIV · 2025-05-07T08:36:06Z

@kemsguy7 Why is that a separate PR? It would be better to push your improvements in this PR as then we can refer to the comments that were made in previous reviews.

kemsguy7 · 2025-05-19T06:24:27Z

@Leonidas-from-XIV @gridbugs , please kindly review, I've applied updates ass suggested in the comments above

Leonidas-from-XIV · 2025-05-19T07:45:54Z

@kemsguy7 There are still review comments that I wrote that are unadressed, could you handle them please?

Also, it seems you need to solve a merge conflict in package_dependency.ml.

kemsguy7 · 2025-05-19T10:39:41Z

@kemsguy7 There are still review comments that I wrote that are unadressed, could you handle them please?

Also, it seems you need to solve a merge conflict in package_dependency.ml.

Sure, please do you mind pointing out the parts that have been unaddressed If you have the time? I did a lot of rebasing, patching e.t.c to fix my PR as it got messy at some point so I might have reverted some of the files.

I'll take a second look though and also fix the merge conflict

Leonidas-from-XIV · 2025-05-19T10:55:08Z

Sure, please do you mind pointing out the parts that have been unaddressed If you have the time?

Sure. I checked the current state of the PR and these are still unaddressed:

gridbugs · 2025-05-20T01:34:37Z

@kemsguy7 the branch this P 8000 R is coming from does not include df45715, since it's based on a version of origin/main from before your previous PR was merged. This is why it looks like the changes from your previous PR are duplicated in this PR which is making it hard to review.

Running git pull origin main --rebase (and dealing with any conflicts) will update your branch so that all your recent changes are applied to the current tip of origin/main.

kemsguy7 · 2025-05-20T10:00:00Z

@kemsguy7 the branch this PR is coming from does not include df45715, since it's based on a version of origin/main from before your previous PR was merged. This is why it looks like the changes from your previous PR are duplicated in this PR which is making it hard to review.

Running git pull origin main --rebase (and dealing with any conflicts) will update your branch so that all your recent changes are applied to the current tip of origin/main.

Thank you for this.
@Leonidas-from-XIV , @gridbugs please kindly review, I've updated with origin main and also implemented changes as requested

Leonidas-from-XIV

Thanks for the improvements. There's still some things that I think needs adjusting before they can be merged.

In particular you also need to run dune fmt to reformat the code to our preferred formatting setting.

src/dune_lang/package_dependency.ml

Leonidas-from-XIV · 2025-05-20T13:10:06Z

src/dune_lang/package_dependency.ml

-        ]
-        ~hints:
+  (* Early return if warnings are disabled *)
+  if Dune_config.Config.(get typo_warnings) <> `Enabled then None


I'd probably suggest matching on `Enabled and the other case, that avoids code that reads oddly:

match Dune_config.Config.(get typo_warnings) with | `Disabled -> None | `Enabled -> (* your else branch *)

Leonidas-from-XIV · 2025-05-20T13:10:23Z

src/dune_lang/package_dependency.ml

+  (* Early return if warnings are disabled *)
+  if Dune_config.Config.(get typo_warnings) <> `Enabled then None
+  else
+    let open Package_constraint in


This still has the local open. Can you please remove it?

@Leonidas-from-XIV please kindly review

gridbugs · 2025-05-22T03:58:17Z

src/dune_config/config.ml

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


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

Do you mean this file config.ml?

I mean the variable typo_warnings.

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.

test/blackbox-tests/test-cases/disabled_warning.t

src/dune_lang/package_dependency.ml

gridbugs · 2025-06-02T06:42:52Z

src/dune_lang/package_dependency.ml

@@ -28,61 +28,73 @@ module Well_formed_name = struct
  module TT = Dune_util.Stringlike.Make (T)

  let decode =
-    let open Decoder in
+    let open Dune_sexp.Decoder in


Did something change to make this addition necessary now?

Leonidas-from-XIV · 2025-06-03T09:10:51Z

src/dune_lang/package_dependency.ml

+let dependency_constraint_variable_typo_warnings ~loc { name; constraint_ } =
+  (* Handle case where config system isn't initialized yet *)
+  let warnings_enabled =
+    try Dune_config.Config.(get typo_warnings) = `Enabled with


What is the exception here? I think for the most part we try not to use exceptions.

@Leonidas-from-XIV I wrote this in response to a failing test that was working before my PR test/blackbox-tests/test-cases/pkg/additional-constraints.t.

Yes, but does Dune_config.Config.get throw an exception? If so, which one and why? If so, the with case should mention the exception and not just blanket-catch every exception. If not, then this try line should be removed.

What is the exception here? I think for the most part we try not to use exceptions.

@Leonidas-from-XIV i've updated this , please check.

src/dune_lang/package_dependency.ml

gridbugs · 2025-06-17T02:08:15Z

src/dune_lang/package_dependency.ml

+    try Dune_config.Config.(get typo_warnings) = `Enabled with
+    | Code_error.E _ -> true (* Only catches Code_error.E exceptions *)
+  in
+  (* Early return if warnings are disabled *)


Nitpick about terminology, but this is not an early return. I don't think this comment adds information that isn't obvious reading the code.

gridbugs · 2025-06-17T02:11:15Z

src/dune_lang/package_dependency.ml

+  (* Handle case where config system isn't initialized yet *)
+  let warnings_enabled =
+    try Dune_config.Config.(get typo_warnings) = `Enabled with
+    | Code_error.E _ -> true (* Only catches Code_error.E exceptions *)


We definitely shouldn't ever be catching Code_error exceptions. The intention of these types of exceptions is to catch programming mistakes inside dune, similar to an assert statement. Did you find that calling Dune_config.Config.(get typo_warnings) raised a Code_error.E exception?

@Leonidas-from-XIV please can you review the change i did here? it's making the additional constraint test fail

src/dune_lang/package_dependency.ml

gridbugs

Left a few suggestions for improving the readability. Also the current approach to handling exceptions raised by Dune_config.Config.(get typo_warnings) doesn't look correct to me.

gridbugs · 2025-06-17T02:27:48Z

src/dune_lang/package_dependency.ml

+  (* Handle case where config system isn't initialized yet *)
+  let warnings_enabled =
+    try Dune_config.Config.(get typo_warnings) = `Enabled with
+    | Code_error.E _ -> true (* Only catches Code_error.E exceptions *)


Possibly moot because this line will be removed/changed while applying feedback from this review, but try to avoid comments that don't add any clarity. Assume readers know ocaml syntax, in which case they already understand that | Code_error.E _ -> ... in the context of a try statement means to catch Code_error.E exceptions. A more useful comment would explain why it's necessary to catch such exceptions, or perhaps why other exceptions are not caught. The comment should explain your intent so readers understand it, and maintainers can preserve your intent through changes in the future, or understand the consequence of deleting this code if necessary.

But as I said, it doesn't really matter in this specific instance since I suspect this line will be deleted anyway and as @Leonidas-from-XIV said above, we try to avoid using exceptions as much as possible in dune.

Leonidas-from-XIV

Please make sure to run dune fmt to format the source code automatically and take a look at the outstanding review comments.

Leonidas-from-XIV · 2025-06-19T08:16:33Z

test/blackbox-tests/test-cases/disabled_warning.t

+  > EOF
+
+  $ DUNE_CONFIG__TYPO_WARNINGS=disabled dune pkg lock
+  $ # No warnings should be produced when the feature is disabled


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

Leonidas-from-XIV · 2025-06-19T08:17:04Z

src/dune_lang/package_dependency.ml

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

 let encode { name; constraint_ } =
-  let open Encoder in
+  let open Dune_sexp.Encoder in


Is this change necessary?

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

Leonidas-from-XIV · 2025-06-19T08:17:37Z

src/dune_config/config.mli

 (** 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


…ith main Signed-off-by: kemsguy7 <mattidungafa@gmail.com>

…moved unnecessary comments Signed-off-by: kemsguy7 <mattidungafa@gmail.com>

gridbugs

Left a few more comments about formatting. Also please make sure to address some of the older comments still outstanding (use the "Files changed" tab to easily see them).

gridbugs · 2025-06-20T12:06:39Z

src/dune_config/config.ml

@@ -86,6 +86,7 @@ let make ~name ~of_string ~default =
 ;;

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


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?

gridbugs · 2025-06-20T12:07:44Z

src/dune_lang/package_dependency.ml

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

 let encode { name; constraint_ } =
-  let open Encoder in
+  let open Dune_sexp.Encoder in


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

Leonidas-from-XIV reviewed May 5, 2025

View reviewed changes

maiste added config Everything related to dune configuration (workspace, project, dune, env) cli Command line related labels May 5, 2025

gridbugs reviewed May 6, 2025

View reviewed changes

test/blackbox-tests/test-cases/typo-detection2.t Outdated Show resolved Hide resolved

kemsguy7 marked this pull request as draft May 7, 2025 04:46

kemsguy7 force-pushed the disabled_warnings branch from 3af87dc to 5d88a1c Compare May 19, 2025 06:12

kemsguy7 force-pushed the disabled_warnings branch from 81c8c90 to 2219401 Compare May 20, 2025 09:54

Leonidas-from-XIV requested changes May 20, 2025

View reviewed changes

kemsguy7 force-pushed the disabled_warnings branch from 8daef1b to fc0d681 Compare May 22, 2025 02:22

gridbugs reviewed May 22, 2025

View reviewed changes

test/blackbox-tests/test-cases/disabled_warning.t Show resolved Hide resolved

gridbugs reviewed May 26, 2025

View reviewed changes

test/blackbox-tests/test-cases/disabled_warning.t Show resolved Hide resolved

kemsguy7 force-pushed the disabled_warnings branch 4 times, most recently from 00d5f6b to 21496b1 Compare June 2, 2025 06:22

gridbugs reviewed Jun 2, 2025

View reviewed changes

kemsguy7 force-pushed the disabled_warnings branch 3 times, most recently from 7735143 to 7b05815 Compare June 2, 2025 07:30

kemsguy7 changed the title ~~Disabled warnings~~ package_dependency_typo_warnings Jun 2, 2025

kemsguy7 force-pushed the disabled_warnings branch from 7b05815 to 8d4aa1a Compare June 2, 2025 07:41

kemsguy7 changed the title ~~package_dependency_typo_warnings~~ feat: add configurable typo detection for package dependency constraints Jun 2, 2025

kemsguy7 marked this pull request as ready for review June 2, 2025 10:10

kemsguy7 requested a review from Leonidas-from-XIV June 2, 2025 15:45

Leonidas-from-XIV reviewed Jun 3, 2025

View reviewed changes

kemsguy7 force-pushed the disabled_warnings branch from 8d4aa1a to 475823f Compare June 4, 2025 08:34

kemsguy7 force-pushed the disabled_warnings branch from 7e9cba8 to 854c75e Compare June 17, 2025 01:27

gridbugs reviewed Jun 17, 2025

View reviewed changes

src/dune_lang/package_dependency.ml Outdated Show resolved Hide resolved

gridbugs requested changes Jun 17, 2025

View reviewed changes

gridbugs reviewed Jun 17, 2025

View reviewed changes

kemsguy7 force-pushed the disabled_warnings branch 2 times, most recently from de12c7e to 2a70998 Compare June 19, 2025 07:29

Leonidas-from-XIV requested changes Jun 19, 2025

View reviewed changes

kemsguy7 added 2 commits June 19, 2025 10:05

Implement disabled warnings configuration and fixed merge conflicts w…

50c69a1

…ith main Signed-off-by: kemsguy7 <mattidungafa@gmail.com>

fix-update: updated package dependency not to catch exceptions and re…

659a6dc

…moved unnecessary comments Signed-off-by: kemsguy7 <mattidungafa@gmail.com>

kemsguy7 force-pushed the disabled_warnings branch from 2a70998 to 659a6dc Compare June 19, 2025 09:05

gridbugs requested changes Jun 20, 2025

View reviewed changes

		@@ -86,6 +86,7 @@ let make ~name ~of_string ~default =
		;;

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

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

Are you sure you want to change the base?

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

Uh oh!

Conversation

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment