Feat/good error message extensions non ascii #12844

benodiwal · 2025-12-02T09:20:26Z

The fix modifies the s-expression lexer to accept non-ASCII characters in atoms, then validates version format at the decoder level where we have semantic context to provide helpful hints.

Previously, non-ASCII characters were rejected by the lexer with a generic "invalid atom" error. Now, the lexer accepts them, and validation occurs at the decoder level where we have context about which extension/lang the version is for. This allows us to provide consistent error messages ("Invalid version. Version must be two numbers separated by a dot.") with helpful, context-specific hints for both ASCII and non-ASCII invalid versions.

Tests cover single extensions, multiple extensions, and various non-ASCII characters including East Asian characters and emoji.

rgrinberg

I think the approach of separately validating using declarations is rather weird and does not solve the underlying issue in all other stanzas as far as I can tell. We should be handling this sort of stuff at the level of dune_sexp.

benodiwal · 2025-12-02T13:58:11Z

Hey @rgrinberg, You're right that this approach is specific to these two declaration types and doesn't solve the general problem. I initially followed the existing pattern for lang declarations (versioned_file_first_line.mll) and applied the same approach to using declarations.

The core issue is that non-ASCII characters in version strings get rejected by the s-expression parser before Syntax.Version.decode can run and provide a helpful error message. My current approach pre-validates specific declarations before s-expression parsing to catch these cases early.

benodiwal · 2025-12-02T14:07:04Z

I followed the existing pattern in the codebase. The lang dune version already uses versioned_file_first_line.mll which does exactly this: it scans raw text before s-expression parsing, extracts lang and version as strings, then validates them with better error messages. I extended this same pattern to using declarations with using_declaration_parser.mll.

However I'm happy to rework this to handle it at the dune_sexp level if you can point me in the right direction.

rgrinberg · 2025-12-02T20:59:25Z

The lang dune version already uses versioned_file_first_line.mll which does exactly this: it scans raw text before s-expression parsing, extracts lang and version as strings, then validates them with better error messages. I extended this same pattern to using declarations with using_declaration_parser.mll.

The reason versioned_file_first_line is this way is because it cannot assume that it is parsing a sexp file. The first line is what determines the format (language + version) of this document, and what follows after this line isn't necessarily sexp. So this pattern is necessary only to solve this rather specific problem. It is not a general purpose solution.

I think the way to go is to modify our existing lexers instead of adding separate validation passes. In short, the lexing stage should reject all non ascii files that we cannot handle (even better would be to handle them of course) and produce appropriate error messages. The most important lexer where this issue is relevant is dune_sexp/lexer.mll. Solving the problem there would solve the issue for the vast majority of users. Therefore, I would suggest to start with that lexer.

A word of caution: this file is quite important to the performance of dune. So I'd recommend some sanity checks to make sure that we haven't considerably slowed down the parsing of valid sexp files.

benodiwal · 2025-12-02T21:22:53Z

Thank you for the clarification! That makes much more sense now. I misunderstood versioned_file_first_line as a general pattern when it's actually solving a very specific problem where the document format itself is unknown until the first line is parsed.

I will start reworking it with this different approach.

rgrinberg · 2025-12-03T18:07:30Z

src/dune_lang/dune_project.ml

+    match sexp with
+    | Atom (loc, A s) ->
+      (* Check if version has invalid format (non-ASCII or not X.Y pattern) *)
+      let has_invalid_format =


Why do we do this check here? Why not just do it in lexer.mll or wherever else we might be creating an invalid atom?

We need this check at the decoder level (rather than just the lexer) because that's where we have the semantic context to provide helpful, extension-specific hints.

At the decoder level in dune_project.ml, we know:

This atom is specifically a version for an extension

Which extension it's for (menhir, melange, etc.)

What the latest valid version is for that extension

This allows us to provide context-aware error messages like Hint: using menhir 3.0 instead of a generic lexer error.

You do have more context, but I think you're going to find it rather tedious to add such hints everywhere. In the end, all the information the user needs is to remove the special characters to form a valid atom.

if you do it at the lexer, the error would be simpler, but it would work everywhere and not just in this one specific case.

Or do you intend to perhaps support non-ascii characters in some places where dune accepts atoms? Then I think it would make sense to handle this stuff at the decoder.

You're right, the lexer-level approach is simpler and more robust. Adding validation everywhere would be tedious and error-prone.
I'm happy to implement the lexer-level solution instead. However, since the idea of providing context-specific hints for version errors was discussed in earlier PRs, can we check with @Alizter as well before reverting to the simpler approach.

cc: @Alizter

rgrinberg · 2025-12-04T11:27:40Z

src/dune_sexp/versioned_file.ml

+              ]
+          else
+            Code_error.raise
+              "Atom.parse failed for unexpected reason"


Is it really unexpected? Can't it happen for some other invalid character? A regular error should suffice here.

Ya, my bad, fixed it now.

rgrinberg · 2025-12-04T11:27:53Z

src/dune_sexp/versioned_file.ml

+            User_error.raise
+              ~loc:ver_loc
+              [ Pp.text
+                  "Invalid atom: contains non-ASCII character(s). Atoms must only \


Could you share this error message between the two files?

I shared it through atom.ml as both files are using atom parsing, can u check once.

rgrinberg

LGTM. @Alizter do you intend to review this?

rgrinberg · 2025-12-04T21:42:53Z

src/dune_sexp/versioned_file.ml

+          let has_non_ascii = String.exists ver ~f:(fun c -> Char.code c >= 128) in
+          if has_non_ascii
+          then User_error.raise ~loc:ver_loc [ Pp.text Atom.non_ascii_error_message ]
+          else User_error.raise ~loc:ver_loc [ Pp.textf "Invalid atom: %S" ver ]


I think you should preserve the message for the else clause:

[ Pp.text "Invalid version. Version must be two numbers separated by a dot." ]

made the change

Alizter · 2025-12-05T08:39:18Z

@rgrinberg Yes, I will give it a review.

Alizter · 2025-12-05T08:46:06Z

test/blackbox-tests/test-cases/extensions-invalid-version.t

+CR-someday benodiwal: The version_loc is greedy and captures the closing
+parenthesis.


Looks like these are OK now?

They are partially fixed. Apparently they are only for the extensions part, for the first line we have to handle it differently. I have explained this in detail in the related issue.

I think we should handle that in different PR, I will able to test this more for extensions as well there along with fix for first line.

Alizter · 2025-12-05T08:58:43Z

test/blackbox-tests/test-cases/lang-invalid-version.t

-  Error: Invalid version. Version must be two numbers separated by a dot.
-  Hint: lang dune 3.21
+  Error: Invalid atom: contains non-ASCII character(s). Atoms must only contain
+  ASCII characters.


We've lost the hint here which is fine since this is a different kind of error. I think the hint is still useful in the ASCII case and looking above to the Ali case we don't provide one. Could you add another CR about adding that hint to the validation step?

Hey @Alizter, I am afk for a while, will do it in some time.

benodiwal · 2025-12-05T10:37:42Z

Hey @Alizter, I have updated the CR somedays, you can check now. Thanks

src/dune_sexp/lexer.mll

… versions Signed-off-by: Sachin Beniwal <s474996633@gmail.com>

Signed-off-by: Sachin Beniwal <s474996633@gmail.com>

…I characters Signed-off-by: Sachin Beniwal <s474996633@gmail.com>

Signed-off-by: Sachin Beniwal <s474996633@gmail.com>

@ElectreAAS

CHANGES: ### Fixed - Fix `include_subdirs qualified` incorrectly picking the furthest module instead of the closest when resolving module name ambiguities. (ocaml/dune#12587, @ElectreAAS and @Alizter) - Fix: include the module alias in the transitive dependency closure with `(include_subdirs qualified)`. (ocaml/dune#12299, @anmonteiro) - Improve error messages for invalid version formats containing non-ASCII characters. Previously, non-ASCII characters in version strings (e.g., `(lang dune è)` or `(using menhir π3.14)`) would fail with a generic "Invalid file" error. Now they display a clear message: "Invalid atom: contains non-ASCII character(s). Atoms must only contain ASCII characters." The fix is implemented at the lexer level, providing consistent error handling across all s-expression parsing. (ocaml/dune#12844, fixes ocaml/dune#12836, @benodiwal) - Pass private modules with -H when this is available (ocaml/dune#12666, @rgrinberg) - Allow multiple modules in `(modules_flags ...)`, in `coq.theory` (ocaml/dune#12733, @rlepigre) - Improve error message for invalid version formats in both `(lang dune ...)` and `(using extension ...)` declarations. Changes "Atom of the form NNN.NNN expected" to "Invalid version. Version must be two numbers separated by a dot." (ocaml/dune#12833, @benodiwal) - Fix crash when running `dune build @check` on a library with virtual modules. (ocaml/dune#12644, fixes ocaml/dune#12636, @Alizter) - Provide a more informative error message when `(pkg enabled)` is put in `dune-project` instead of `dune-workspace`. (ocaml/dune#12802, fixes ocaml/dune#12801, @benodiwal) - Improve error message when invalid version strings are used in `dune-project` files. Non-ASCII characters and malformed versions now show a helpful hint with an example of the correct format. (ocaml/dune#12794, fixes ocaml/dune#12751, @benodiwal) - Stop hiding the `root_module` from the include path (ocaml/dune#12239, @rgrinberg) - Allow `$ dune init` to work on absolute paths (ocaml/dune#12601, fixes ocaml/dune#7806, @rgrinberg) - `(include_subdirs qualified)`: Add missing alias dependency to module group. (ocaml/dune#12530, @anmonteiro) - Add Melange compilation to the `@all` alias in libraries (ocaml/dune#12628, @anmonteiro) - Fix greedy version location in lang declarations. Previously, error locations for invalid lang versions would span multiple bytes for multi-byte UTF-8 characters, causing carets to appear misaligned and seemingly include the closing parenthesis. Now, error locations for ASCII strings show the full length (e.g., "Ali" shows `^^^`), while non-ASCII strings show only the first byte (e.g., "è" shows `^`) to avoid multi-byte character display issues. (ocaml/dune#12869, fixes ocaml/dune#12806, @benodiwal) - melange support: don't emit empty JavaScript modules for generated module aliases. (ocaml/dune#12464, @anmonteiro) ### Added - (Experimental): Introduce the `library_parameter` stanza. It allows users to declare a parameter when using the OxCaml compiler. (ocaml/dune#11963, implements ocaml/dune#12084, @maiste) - Added the ability to scroll horizontally in TUI. (ocaml/dune#12386, @Alizter) - Feature: Include shell command that was executed when a cram test has occurred in the error message (ocaml/dune#12307, @rgrinberg) - support expanding variables in `(promote (into ..))` (ocaml/dune#12832, fixes ocaml/dune#12742, @anmonteiro) - Add support for `%{cmt:...}` and `%{cmti:...}` variables to reference compiled annotation files (.cmt and .cmti) containing typed abstract syntax trees with location and type information. (ocaml/dune#12634, grants ocaml/dune#12633, @Alizter) - Add `$ dune describe tests` to describe the tests in the workspace (@Gromototo, ocaml/dune#12545, fixes ocaml/dune#12030) - Add `argv`, the process environment, and the dune version to the config event in the trace (ocaml/dune#12909, @rgrinberg) - Allow `dune runtest` to properly run while a watch mode server is running. (ocaml/dune#12473, grants ocaml/dune#8114, @gridbugs and @ElectreAAS) - Use copy-on-write (COW) when copying files on filesystems that support it (Btrfs, ZFS, XFS, etc), under Linux. (ocaml/dune#12074, fixes ocaml/dune#12071, @nojb) - Add support for Tangled ATproto-based code repositories (ocaml/dune#12197, @avsm) - Add support for instantiating OxCaml parameterised libraries. (ocaml/dune#12561, @art-w) - Add a `(conflict_markers error|ignore)` option to the cram stanza. When `(conflict_markers error)` is set, the cram test will fail in the presence of conflict markers. Git, diff3 and jujutsu conflict markers are detected. (ocaml/dune#12538, ocaml/dune#12617, ocaml/dune#12655, fixes ocaml/dune#12512, @rgrinberg, @Alizter) - Introduce a `%{ppx:lib1+..+libn}` stanza to make it possible to refer to ppx executables built by dune. This is useful for writing tests (ocaml/dune#12711, @rgrinberg) - Introduce a `(dir ..)` field on packages defined in the `dune-project`. This field allows to associate a directory with a particular package. This makes dune automatically filter out all stanzas in this directory and its descendants with `--only-packages`. All users are recommended to switch to using this field. (ocaml/dune#12614, fixes ocaml/dune#3255, @rgrinberg) - Add support for `DUNE_ROOT` environment variable, similar to the existing `--root` CLI parameter. (fixes ocaml/dune#12399 @sir4ur0n) - Introduce an `unused-libs` alias to detect unused libraries. (ocaml/dune#12623, fixes ocaml/dune#650, @rgrinberg) - Add `--files` flag to `dune describe opam-files` to print only the names of the opam files line by line. (ocaml/dune#9793, @reynir and @Alizter) - `dune exec` now accepts absolute paths inside the workspace. (ocaml/dune#12094, @Alizter) - Add `coqdoc_header` and `coqdoc_footer` fields to the `coq` field of the `env` stanza, and to the `coq.theory` stanza, allowing to configure a custom header or footer respectively in the HTML output of `coqdoc`. (ocaml/dune#11131, @rlepigre) - Allow `dune fmt` to properly run while a watch mode server is running. Note that the `--preview` flag is not supported in this mode. (ocaml/dune#12064, @ElectreAAS) - Support for generating `_CoqProject` files for `coq.theory` stanzas. (ocaml/dune#11752, @rlepigre) - Added `(files)` stanza, similar to `(dirs)` to control which files are visible to Dune on a per-directory basis. (ocaml/dune#12879, @nojb) - Add support for %{ocaml-config:ox} (ocaml/dune#12236, @jonludlam) - Introduce `dune promotion show` command to display the contents of corrected files that are ready for promotion. This allows users to preview changes before running `dune promote`. The command accepts file arguments to show specific files, or displays all promotable files when called without arguments. (ocaml/dune#12669, fixes ocaml/dune#3883, @MixiMaxiMouse) - New `(lang rocq)` build mode for Rocq 9.0 and later. This new mode is very similar to the existing `(lang coq)`, except that it doesn't need the `coq*` compatibility wrappers. As of today `(lang rocq)` doesn't support yet composed builds with Rocq itself, this will be added later. `(lang coq)` is deprecated, development is frozen, and will be removed at some point in the future. (ocaml/dune#12035, @ejgallego, @Lysxia, fixes ocaml/dune#11572) ### Changed - Don't run `ocamldep` to compute false dependencies on the `root_module` (ocaml/dune#12227, @rgrinberg) - `dune format-dune-file` now uses the syntax version of the Dune project that contains the file being formatted (if any) instead of using the latest version available, which remains the default if there is no Dune project in scope. (ocaml/dune#11865, @nojb) - Persistent DB and process events have been slightly modified. Persistent DB events have more concise names and job events always include full information. (ocaml/dune#12867, @rgrinberg) - Removed the `--trace-extended` flag. Its functionality is always enabled when tracing is active (ocaml/dune#12908, @rgrinberg) - The `test/dune` file generated by `dune init proj` now depends on the project library. (ocaml/dune#12791, @shonfeder) - Starting with version 3.21 of the Dune language, Dune no longer changes the default set of compiler warnings. For users that would like to keep the old behaviour, the variable `%{dune-warnings}` can be used in an `(env)` stanza in a top-level Dune file: `(env (dev (flags :standard %{dune-warnings})))`. (ocaml/dune#12766, @nojb) - Fix: stop generating `cmt` files for cinaps binaries (ocaml/dune#12530, @rgrinberg)

@ElectreAAS

CHANGES: ### Fixed - Fix `include_subdirs qualified` incorrectly picking the furthest module instead of the closest when resolving module name ambiguities. (ocaml/dune#12587, @ElectreAAS and @Alizter) - Fix: include the module alias in the transitive dependency closure with `(include_subdirs qualified)`. (ocaml/dune#12299, @anmonteiro) - Improve error messages for invalid version formats containing non-ASCII characters. Previously, non-ASCII characters in version strings (e.g., `(lang dune è)` or `(using menhir π3.14)`) would fail with a generic "Invalid file" error. Now they display a clear message: "Invalid atom: contains non-ASCII character(s). Atoms must only contain ASCII characters." The fix is implemented at the lexer level, providing consistent error handling across all s-expression parsing. (ocaml/dune#12844, fixes ocaml/dune#12836, @benodiwal) - Pass private modules with -H when this is available (ocaml/dune#12666, @rgrinberg) - Allow multiple modules in `(modules_flags ...)`, in `coq.theory` (ocaml/dune#12733, @rlepigre) - Improve error message for invalid version formats in both `(lang dune ...)` and `(using extension ...)` declarations. Changes "Atom of the form NNN.NNN expected" to "Invalid version. Version must be two numbers separated by a dot." (ocaml/dune#12833, @benodiwal) - Fix crash when running `dune build @check` on a library with virtual modules. (ocaml/dune#12644, fixes ocaml/dune#12636, @Alizter) - Provide a more informative error message when `(pkg enabled)` is put in `dune-project` instead of `dune-workspace`. (ocaml/dune#12802, fixes ocaml/dune#12801, @benodiwal) - Improve error message when invalid version strings are used in `dune-project` files. Non-ASCII characters and malformed versions now show a helpful hint with an example of the correct format. (ocaml/dune#12794, fixes ocaml/dune#12751, @benodiwal) - Stop hiding the `root_module` from the include path (ocaml/dune#12239, @rgrinberg) - Allow `$ dune init` to work on absolute paths (ocaml/dune#12601, fixes ocaml/dune#7806, @rgrinberg) - `(include_subdirs qualified)`: Add missing alias dependency to module group. (ocaml/dune#12530, @anmonteiro) - Add Melange compilation to the `@all` alias in libraries (ocaml/dune#12628, @anmonteiro) - Fix greedy version location in lang declarations. Previously, error locations for invalid lang versions would span multiple bytes for multi-byte UTF-8 characters, causing carets to appear misaligned and seemingly include the closing parenthesis. Now, error locations for ASCII strings show the full length (e.g., "Ali" shows `^^^`), while non-ASCII strings show only the first byte (e.g., "è" shows `^`) to avoid multi-byte character display issues. (ocaml/dune#12869, fixes ocaml/dune#12806, @benodiwal) - melange support: don't emit empty JavaScript modules for generated module aliases. (ocaml/dune#12464, @anmonteiro) ### Added - (Experimental): Introduce the `library_parameter` stanza. It allows users to declare a parameter when using the OxCaml compiler. (ocaml/dune#11963, implements ocaml/dune#12084, @maiste) - Added the ability to scroll horizontally in TUI. (ocaml/dune#12386, @Alizter) - Feature: Include shell command that was executed when a cram test has occurred in the error message (ocaml/dune#12307, @rgrinberg) - support expanding variables in `(promote (into ..))` (ocaml/dune#12832, fixes ocaml/dune#12742, @anmonteiro) - Add support for `%{cmt:...}` and `%{cmti:...}` variables to reference compiled annotation files (.cmt and .cmti) containing typed abstract syntax trees with location and type information. (ocaml/dune#12634, grants ocaml/dune#12633, @Alizter) - Add `$ dune describe tests` to describe the tests in the workspace (@Gromototo, ocaml/dune#12545, fixes ocaml/dune#12030) - Add `argv`, the process environment, and the dune version to the config event in the trace (ocaml/dune#12909, @rgrinberg) - Allow `dune runtest` to properly run while a watch mode server is running. (ocaml/dune#12473, grants ocaml/dune#8114, @gridbugs and @ElectreAAS) - Use copy-on-write (COW) when copying files on filesystems that support it (Btrfs, ZFS, XFS, etc), under Linux. (ocaml/dune#12074, fixes ocaml/dune#12071, @nojb) - Add support for Tangled ATproto-based code repositories (ocaml/dune#12197, @avsm) - Add support for instantiating OxCaml parameterised libraries. (ocaml/dune#12561, @art-w) - Add a `(conflict_markers error|ignore)` option to the cram stanza. When `(conflict_markers error)` is set, the cram test will fail in the presence of conflict markers. Git, diff3 and jujutsu conflict markers are detected. (ocaml/dune#12538, ocaml/dune#12617, ocaml/dune#12655, fixes ocaml/dune#12512, @rgrinberg, @Alizter) - Introduce a `%{ppx:lib1+..+libn}` stanza to make it possible to refer to ppx executables built by dune. This is useful for writing tests (ocaml/dune#12711, @rgrinberg) - Introduce a `(dir ..)` field on packages defined in the `dune-project`. This field allows to associate a directory with a particular package. This makes dune automatically filter out all stanzas in this directory and its descendants with `--only-packages`. All users are recommended to switch to using this field. (ocaml/dune#12614, fixes ocaml/dune#3255, @rgrinberg) - Add support for `DUNE_ROOT` environment variable, similar to the existing `--root` CLI parameter. (fixes ocaml/dune#12399 @sir4ur0n) - Introduce an `unused-libs` alias to detect unused libraries. (ocaml/dune#12623, fixes ocaml/dune#650, @rgrinberg) - Add `--files` flag to `dune describe opam-files` to print only the names of the opam files line by line. (ocaml/dune#9793, @reynir and @Alizter) - `dune exec` now accepts absolute paths inside the workspace. (ocaml/dune#12094, @Alizter) - Add `coqdoc_header` and `coqdoc_footer` fields to the `coq` field of the `env` stanza, and to the `coq.theory` stanza, allowing to configure a custom header or footer respectively in the HTML output of `coqdoc`. (ocaml/dune#11131, @rlepigre) - Allow `dune fmt` to properly run while a watch mode server is running. Note that the `--preview` flag is not supported in this mode. (ocaml/dune#12064, @ElectreAAS) - Support for generating `_CoqProject` files for `coq.theory` stanzas. (ocaml/dune#11752, @rlepigre) - Added `(files)` stanza, similar to `(dirs)` to control which files are visible to Dune on a per-directory basis. (ocaml/dune#12879, @nojb) - Add support for %{ocaml-config:ox} (ocaml/dune#12236, @jonludlam) - Introduce `dune promotion show` command to display the contents of corrected files that are ready for promotion. This allows users to preview changes before running `dune promote`. The command accepts file arguments to show specific files, or displays all promotable files when called without arguments. (ocaml/dune#12669, fixes ocaml/dune#3883, @MixiMaxiMouse) - New `(lang rocq)` build mode for Rocq 9.0 and later. This new mode is very similar to the existing `(lang coq)`, except that it doesn't need the `coq*` compatibility wrappers. As of today `(lang rocq)` doesn't support yet composed builds with Rocq itself, this will be added later. `(lang coq)` is deprecated, development is frozen, and will be removed at some point in the future. (ocaml/dune#12035, @ejgallego, @Lysxia, fixes ocaml/dune#11572) ### Changed - Don't run `ocamldep` to compute false dependencies on the `root_module` (ocaml/dune#12227, @rgrinberg) - `dune format-dune-file` now uses the syntax version of the Dune project that contains the file being formatted (if any) instead of using the latest version available, which remains the default if there is no Dune project in scope. (ocaml/dune#11865, @nojb) - Persistent DB and process events have been slightly modified. Persistent DB events have more concise names and job events always include full information. (ocaml/dune#12867, @rgrinberg) - Removed the `--trace-extended` flag. Its functionality is always enabled when tracing is active (ocaml/dune#12908, @rgrinberg) - The `test/dune` file generated by `dune init proj` now depends on the project library. (ocaml/dune#12791, @shonfeder) - Starting with version 3.21 of the Dune language, Dune no longer changes the default set of compiler warnings. For users that would like to keep the old behaviour, the variable `%{dune-warnings}` can be used in an `(env)` stanza in a top-level Dune file: `(env (dev (flags :standard %{dune-warnings})))`. (ocaml/dune#12766, @nojb) - Fix: stop generating `cmt` files for cinaps binaries (ocaml/dune#12530, @rgrinberg)

benodiwal force-pushed the feat/good-error-message-extensions-non-ascii branch from 3c0162b to 1784558 Compare December 2, 2025 09:20

Alizter self-requested a review December 2, 2025 10:09

rgrinberg requested changes Dec 2, 2025

View reviewed changes

benodiwal marked this pull request as draft December 2, 2025 21:26

benodiwal force-pushed the feat/good-error-message-extensions-non-ascii branch 3 times, most recently from 4912f08 to b489524 Compare December 3, 2025 09:31

rgrinberg reviewed Dec 3, 2025

View reviewed changes

benodiwal force-pushed the feat/good-error-message-extensions-non-ascii branch from b489524 to 512b27f Compare December 3, 2025 18:37

benodiwal mentioned this pull request Dec 4, 2025

Location excerpts miscompute character positions for unicode and greedy ranges #12806

Closed

benodiwal marked this pull request as ready for review December 4, 2025 11:17

rgrinberg reviewed Dec 4, 2025

View reviewed changes

benodiwal force-pushed the feat/good-error-message-extensions-non-ascii branch from e61943a to fb1288e Compare December 4, 2025 15:16

rgrinberg approved these changes Dec 4, 2025

View reviewed changes

benodiwal force-pushed the feat/good-error-message-extensions-non-ascii branch from 2a11379 to 1a0d184 Compare December 5, 2025 08:06

Alizter reviewed Dec 5, 2025

View reviewed changes

benodiwal force-pushed the feat/good-error-message-extensions-non-ascii branch from f15ae27 to 7c3a713 Compare December 5, 2025 10:36

benodiwal force-pushed the feat/good-error-message-extensions-non-ascii branch from 7c3a713 to c1f4aff Compare December 5, 2025 11:28

Alizter reviewed Dec 5, 2025

View reviewed changes

src/dune_sexp/lexer.mll Outdated Show resolved Hide resolved

benodiwal added 3 commits December 5, 2025 18:22

feat: added logic for handling non ascii cases for invalid extensions…

1c632b1

… versions Signed-off-by: Sachin Beniwal <s474996633@gmail.com>

fix: valid format logic

2ccf121

Signed-off-by: Sachin Beniwal <s474996633@gmail.com>

fix: refactored code

025b55a

Signed-off-by: Sachin Beniwal <s474996633@gmail.com>

benodiwal added 11 commits December 5, 2025 18:22

tests: added tests for multiple extensions cases

2c6da6c

Signed-off-by: Sachin Beniwal <s474996633@gmail.com>

chore: CR someday

8376b40

Signed-off-by: Sachin Beniwal <s474996633@gmail.com>

fix: reverted changes, kept the test cases

2b635f1

Signed-off-by: Sachin Beniwal <s474996633@gmail.com>

fix: improve error messages for invalid version formats with non-ASCI…

718357c

…I characters Signed-off-by: Sachin Beniwal <s474996633@gmail.com>

feat: implemented lexer level approach for non ascii characters

7e06d55

Signed-off-by: Sachin Beniwal <s474996633@gmail.com>

fix: refactor

b6487ce

Signed-off-by: Sachin Beniwal <s474996633@gmail.com>

tests: promoted non-ascii-characters.t to new logic

9a0b127

Signed-off-by: Sachin Beniwal <s474996633@gmail.com>

fix: else claude message

0864ca5

Signed-off-by: Sachin Beniwal <s474996633@gmail.com>

chore: added entry for CHANGES.md

cfae018

Signed-off-by: Sachin Beniwal <s474996633@gmail.com>

chore: updated CR someday

1a7bdba

Signed-off-by: Sachin Beniwal <s474996633@gmail.com>

fix: ascii start

344ccb4

Signed-off-by: Sachin Beniwal <s474996633@gmail.com>

benodiwal force-pushed the feat/good-error-message-extensions-non-ascii branch from 763c294 to 344ccb4 Compare December 5, 2025 12:52

Alizter approved these changes Dec 5, 2025

View reviewed changes

Alizter enabled auto-merge December 5, 2025 12:53

Alizter merged commit 15eead0 into ocaml:main Dec 5, 2025
29 checks passed

benodiwal mentioned this pull request Dec 5, 2025

fix: greedy version location in lang declarations #12869

Merged

shonfeder mentioned this pull request Dec 11, 2025

[new release] dune (17 packages) (3.21.0~alpha3) ocaml/opam-repository#29070

Closed

shonfeder mentioned this pull request Dec 15, 2025

[new release] dune (17 packages) (3.21.0~alpha4) ocaml/opam-repository#29096

Closed

shonfeder mentioned this pull request Jan 5, 2026

[new release] dune (17 packages) (3.21.0~alpha5) ocaml/opam-repository#29189

Open

		CR-someday benodiwal: The version_loc is greedy and captures the closing
		parenthesis.

Feat/good error message extensions non ascii #12844

Feat/good error message extensions non ascii #12844

Uh oh!

Conversation

benodiwal commented Dec 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rgrinberg left a comment

Choose a reason for hiding this comment

Uh oh!

benodiwal commented Dec 2, 2025

Uh oh!

benodiwal commented Dec 2, 2025

Uh oh!

rgrinberg commented Dec 2, 2025

Uh oh!

benodiwal commented Dec 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

rgrinberg left a comment

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!

Alizter commented Dec 5, 2025

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!

benodiwal commented Dec 5, 2025

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

benodiwal commented Dec 2, 2025 •

edited

Loading

benodiwal commented Dec 2, 2025 •

edited

Loading