Dune & Jbuild validation for atoms #891

rgrinberg · 2018-06-19T09:02:45Z

Atoms can now be constructed and pretty printed with a syntax = Jbuild | Dune.
The syntax controls validation that will be used to make sure we are printing
something/reading valid

An issue here is that I'm hard coding the dune syntax in a couple of places where we should probably be flexible - like the sub systems file. But it seems to work.

ghost · 2018-06-19T09:25:51Z

src/usexp/atom.ml

+  let rec jbuild s i len =
+    i = len ||
+    match String.unsafe_get s i with
+    | '"' | '(' | ')' | ';' | '\000'..'\032' | '\127'..'\255' -> false


We need to get back the old function here. The one that was checking for |# and #| as well

rgrinberg · 2018-06-19T11:34:34Z

I think we were only doing this check on the lexer side before. So the lexer would actually parse only a subset of symbols that were is_valid. In any case, I agree that we should harmonize the two. Should I do it in this pr?

…

On Tue, Jun 19, 2018, 4:26 PM Jérémie Dimino ***@***.***> wrote: ***@***.**** requested changes on this pull request. ------------------------------ In src/usexp/atom.ml <#891 (comment)>: > @@ -0,0 +1,55 @@ +type t = A of string [@@unboxed] + +let invalid_argf fmt = Printf.ksprintf invalid_arg fmt + +type syntax = Jbuild | Dune + +let string_of_syntax = function + | Jbuild -> "jbuild" + | Dune -> "dune" + +let (is_valid_jbuild, is_valid_dune) = + let rec jbuild s i len = + i = len || + match String.unsafe_get s i with + | '"' | '(' | ')' | ';' | '\000'..'\032' | '\127'..'\255' -> false We need to get back the old function here. The one that was checking for |# and #| as well — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#891 (review)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAIe-7Usjyx1-vE8c-rkc5sL9laf8ZJIks5t-MO0gaJpZM4UtHSB> .

ghost · 2018-06-19T12:43:49Z

Yes, we might as well do it now.

rgrinberg · 2018-06-19T15:22:02Z

@diml I improved the checks in the jbuild atom validator

ghost · 2018-06-19T15:29:26Z

test/unit-tests/sexp.mlt

@@ -140,7 +140,7 @@ parse {|"$bar%foo%"|}

 parse {|\%{foo}|}
 [%%expect{|
- : parse_result = Same (Ok [\%{foo}])
+Exception: Invalid_argument "'\\%{foo}' is not a valid dune atom".


Could you also modify parse so that Invalid_argument exceptions are captured? So that we can see if the behavior is different or not.

ghost · 2018-06-19T15:33:05Z

BTW, following the conversation on slack, I assumed that of_string wouldn't do the validation anymore and it would be done in the pretty-printer. In particular we shouldn't need to pass Dune or Jbuild to Atom.of_string.

rgrinberg · 2018-06-19T15:48:14Z

Yup. Will remove that extra validation.

…

On Tue, Jun 19, 2018, 10:35 PM Jérémie Dimino ***@***.***> wrote: BTW, following the conversation on slack, I assumed that of_string wouldn't do the validation anymore and it would be done in the pretty-printer. In particular we shouldn't need to pass Dune or Jbuild to Atom.of_string. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#891 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAIe-y-5qEXdqYjF1fqcxpoVZsewStHlks5t-Rm3gaJpZM4UtHSB> .

rgrinberg · 2018-06-20T08:22:18Z

@diml this is now rebased on master. PS, we should probably get rid of Invalid_argument in the sexp library as well, but for that we'll need to invert the dependency of stdlib and usexp first.

rgrinberg · 2018-06-20T08:39:30Z

@diml taht failure from camomile is concerning:

# opam-version 1.2.2
# os           linux
# command      jbuilder build -p camomile -j 4
# path         /home/travis/.opam/system/build/camomile.1.0.1
# compiler     system (4.05.0)
# exit-code    1
# env-file     /home/travis/.opam/system/build/camomile.1.0.1/camomile-4431-ecffd3.env
# stdout-file  /home/travis/.opam/system/build/camomile.1.0.1/camomile-4431-ecffd3.out
# stderr-file  /home/travis/.opam/system/build/camomile.1.0.1/camomile-4431-ecffd3.err
### stderr ###
# File "Camomile/charmaps/jbuild.inc", line 5, characters 11-12:
# Error: Invalid atom character '%'

Is it because we aren't setting the version properly in include?

rgrinberg · 2018-06-20T08:45:40Z

Wondering how should we version include. I suppose we could just make it use the same syntax as the file it's used in.

ghost · 2018-06-20T08:39:56Z

src/usexp/atom.ml

+    match String.unsafe_get s i with
+    | '#' -> disallow_next '|' s (i + 1) len
+    | '|' -> disallow_next '#' s (i + 1) len
+    | '"' | '(' | ')' | ';' | '\000'..'\032' | '\127'..'\255' -> false


Some of these characters where allowed in jbuild syntax. Let's just restore the old function:

let is_valid str = let len = String.length str in len > 0 && let rec loop ix = match str.[ix] with | '"' | '(' | ')' | ';' -> true | '|' -> ix > 0 && let next = ix - 1 in str.[next] = '#' || loop next | '#' -> ix > 0 && let next = ix - 1 in str.[next] = '|' || loop next | ' ' | '\t' | '\n' | '\012' | '\r' -> true | _ -> ix > 0 && loop (ix - 1) in not (loop (len - 1))

The old function was this:

let is_valid = let rec loop s i len = i = len || match String.unsafe_get s i with | '"' | '(' | ')' | ';' | '\000'..'\032' | '\127'..'\255' -> false | _ -> loop s (i + 1) len in fun s -> let len = String.length s in len > 0 && loop s 0 len

I'll use your definition however.

I meant from before #837

ghost · 2018-06-20T08:41:57Z

src/usexp/atom.mli

+
+val of_string : string -> t
+
+val to_string : t -> syntax -> string


I don't think to_string should do the validation. to_string should just return the contents of the atom. For instance I think that's why the build of camomile is broken in travis.

The validation should be done by a separate printing function.

ghost · 2018-06-20T08:47:40Z

test/unit-tests/sexp_tests.ml

@@ -23,13 +23,14 @@ let () =
        | Atom _ -> true
        | _      -> false
      in
-      if Usexp.Atom.is_valid s <> parser_recognizes_as_atom then begin
+      let valid_dune_atom = Usexp.Atom.is_valid_dune s in
+      if valid_dune_atom <> parser_recognizes_as_atom then begin


Could you update this test so that it runs for both the jbuild and Dune syntax? For instance it would have caught the issue related to is_valid_jbuild

ghost · 2018-06-20T08:50:24Z

The camomile failure is because Of_sexp.string validates the atom using Dune syntax. Includes currently reuse the same syntax as the file they are included from.

ghost · 2018-06-20T09:24:42Z

src/usexp/usexp.mli

@@ -7,20 +7,18 @@ module Atom : sig
  (** Acceptable atoms are composed of chars in the range ['!' .. '~'] excluding
      [' ' '"' '(' ')' ';' '\\'], and must be nonempty. *)


This comment is no longer valid BTW, we can just remove it.

ghost · 2018-06-20T09:45:19Z

test/unit-tests/sexp.mlt

+Different
+ {jbuild =
+   Ok
+    [<printer pp_sexp_ast raised an exception: Invalid_argument("atom '\\%{foo}' cannot be in dune syntax")>];


Maybe we should do #remove_printer pp_sexp_ast before these tests, it took me a while to understand what was going on here

Hmm, I think we should print the atoms unambigiously instead. Otherwise it sucks that we can't pp some of these ast's for debugging.

rgrinberg · 2018-06-20T10:17:13Z

The last commit prints Sexp ast in a clearer way for debugging. Atoms are expanded out using an (atom <payload>) constructor.

Atoms can now be constructed and pretty printed with a syntax = Jbuild | Dune. The syntax controls validation that will be used to make sure we are printing something/reading valid Signed-off-by: Rudi Grinberg <rudi.grinberg@gmail.com>

Signed-off-by: Rudi Grinberg <rudi.grinberg@gmail.com>

And make the tests reflect back Invalid_argument Signed-off-by: Rudi Grinberg <rudi.grinberg@gmail.com>

Signed-off-by: Rudi Grinberg <rudi.grinberg@gmail.com>

rgrinberg requested a review from a user June 19, 2018 09:02

ghost suggested changes Jun 19, 2018

View reviewed changes

rgrinberg force-pushed the atom-jbuild-dune branch from ea04cad to 4e0e165 Compare June 19, 2018 15:06

ghost suggested changes Jun 19, 2018

View reviewed changes

rgrinberg force-pushed the atom-jbuild-dune branch 2 times, most recently from 958ac68 to 260591c Compare June 20, 2018 08:18

ghost suggested changes Jun 20, 2018

View reviewed changes

ghost reviewed Jun 20, 2018

View reviewed changes

rgrinberg force-pushed the atom-jbuild-dune branch from e3acf92 to 5771a15 Compare June 20, 2018 09:27

ghost suggested changes Jun 20, 2018

View reviewed changes

ghost approved these changes Jun 20, 2018

View reviewed changes

rgrinberg added 9 commits June 20, 2018 18:23

Dune & Jbuild validation for atoms

ad3a956

Atoms can now be constructed and pretty printed with a syntax = Jbuild | Dune. The syntax controls validation that will be used to make sure we are printing something/reading valid Signed-off-by: Rudi Grinberg <rudi.grinberg@gmail.com>

Improve validation of jbuild atoms

46eba2e

Signed-off-by: Rudi Grinberg <rudi.grinberg@gmail.com>

Fix 4.02.3 compat

5618be7

Signed-off-by: Rudi Grinberg <rudi.grinberg@gmail.com>

Remove constructor side validation

99fbac2

And make the tests reflect back Invalid_argument Signed-off-by: Rudi Grinberg <rudi.grinberg@gmail.com>

Fix definition of is_valid_jbuild

a1d714f

Signed-off-by: Rudi Grinberg <rudi.grinberg@gmail.com>

Move validation to a print function

8a87b5b

Signed-off-by: Rudi Grinberg <rudi.grinberg@gmail.com>

Parameterize sexp_tests on dune and jbuild syntax

7d2c7d9

Signed-off-by: Rudi Grinberg <rudi.grinberg@gmail.com>

Remove outdated comment about atoms

f180470

Signed-off-by: Rudi Grinberg <rudi.grinberg@gmail.com>

Print atom using atom constructor

53d9c64

Signed-off-by: Rudi Grinberg <rudi.grinberg@gmail.com>

rgrinberg force-pushed the atom-jbuild-dune branch from 28100dd to 53d9c64 Compare June 20, 2018 11:53

rgrinberg merged commit 411552e into ocaml:master Jun 20, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Dune & Jbuild validation for atoms #891

Dune & Jbuild validation for atoms #891

rgrinberg commented Jun 19, 2018

ghost Jun 19, 2018

rgrinberg commented Jun 19, 2018 via email

ghost commented Jun 19, 2018

rgrinberg commented Jun 19, 2018

ghost Jun 19, 2018

ghost commented Jun 19, 2018

rgrinberg commented Jun 19, 2018 via email

rgrinberg commented Jun 20, 2018

rgrinberg commented Jun 20, 2018

rgrinberg commented Jun 20, 2018

ghost Jun 20, 2018

rgrinberg Jun 20, 2018

ghost Jun 20, 2018

ghost Jun 20, 2018

ghost Jun 20, 2018

ghost commented Jun 20, 2018 •

edited by ghost

Loading

ghost Jun 20, 2018

ghost Jun 20, 2018

rgrinberg Jun 20, 2018

rgrinberg commented Jun 20, 2018


		val of_string : string -> t

		val to_string : t -> syntax -> string

		@@ -7,20 +7,18 @@ module Atom : sig
		(** Acceptable atoms are composed of chars in the range ['!' .. '~'] excluding
		[' ' '"' '(' ')' ';' '\\'], and must be nonempty. *)

Dune & Jbuild validation for atoms #891

Dune & Jbuild validation for atoms #891

Conversation

rgrinberg commented Jun 19, 2018

Choose a reason for hiding this comment

rgrinberg commented Jun 19, 2018 via email

ghost commented Jun 19, 2018

rgrinberg commented Jun 19, 2018

Choose a reason for hiding this comment

ghost commented Jun 19, 2018

rgrinberg commented Jun 19, 2018 via email

rgrinberg commented Jun 20, 2018

rgrinberg commented Jun 20, 2018

rgrinberg commented Jun 20, 2018

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

ghost commented Jun 20, 2018 • edited by ghost Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rgrinberg commented Jun 20, 2018

ghost commented Jun 20, 2018 •

edited by ghost

Loading