From 12e2adf97b04a99f5587e453fd2cde9051e4aae6 Mon Sep 17 00:00:00 2001 From: Etienne Millon Date: Mon, 2 Sep 2019 15:04:34 +0200 Subject: [PATCH 1/2] len attribute should be an int literal Closes #264 This rejects things like `[@len ""]` and `[@len 1l]` which were previously silently ignored. --- ppx/ppx_cstruct.ml | 21 ++++++++------ ppx_test/errors/cstruct_len_int32.ml | 6 ++++ ppx_test/errors/cstruct_len_int32.ml.expected | 2 ++ ppx_test/errors/cstruct_len_not_int.ml | 6 ++++ .../errors/cstruct_len_not_int.ml.expected | 2 ++ ppx_test/errors/dune.inc | 28 +++++++++++++++++++ 6 files changed, 57 insertions(+), 8 deletions(-) create mode 100644 ppx_test/errors/cstruct_len_int32.ml create mode 100644 ppx_test/errors/cstruct_len_int32.ml.expected create mode 100644 ppx_test/errors/cstruct_len_not_int.ml create mode 100644 ppx_test/errors/cstruct_len_not_int.ml.expected diff --git a/ppx/ppx_cstruct.ml b/ppx/ppx_cstruct.ml index cf63cff4..e5a182dc 100644 --- a/ppx/ppx_cstruct.ml +++ b/ppx/ppx_cstruct.ml @@ -526,15 +526,20 @@ let constr_enum = function | {pcd_loc = loc; _} -> loc_err loc "invalid cenum variant" +let get_len = function + | [ ({txt = "len"; _}, + PStr + [{pstr_desc = + Pstr_eval ({pexp_desc = Pexp_constant (Pconst_integer (sz, None)); _}, _) + ; _}])] -> + Some (int_of_string sz) + | [{txt = "len"; loc}, _ ] -> + loc_err loc "[@len] argument should be an integer" + | _ -> + None + let constr_field {pld_name = fname; pld_type = fty; pld_loc = loc; pld_attributes = att; _} = - let get = function - | [{txt = "len"; _}, PStr - [{pstr_desc = Pstr_eval ({pexp_desc = Pexp_constant (Pconst_integer(sz, _)); _}, _); _}]] -> - Some (int_of_string sz) - | _ -> - None - in - let sz = match get fty.ptyp_attributes, get att with + let sz = match get_len fty.ptyp_attributes, get_len att with | Some sz, None | None, Some sz -> Some sz | Some _, Some _ -> loc_err loc "multiple field length attribute" diff --git a/ppx_test/errors/cstruct_len_int32.ml b/ppx_test/errors/cstruct_len_int32.ml new file mode 100644 index 00000000..91d0dce8 --- /dev/null +++ b/ppx_test/errors/cstruct_len_int32.ml @@ -0,0 +1,6 @@ +[%%cstruct + type t = { + a: uint8_t [@len 8l] + } + [@@little_endian] +] diff --git a/ppx_test/errors/cstruct_len_int32.ml.expected b/ppx_test/errors/cstruct_len_int32.ml.expected new file mode 100644 index 00000000..0822f73d --- /dev/null +++ b/ppx_test/errors/cstruct_len_int32.ml.expected @@ -0,0 +1,2 @@ +File "cstruct_len_int32.ml", line 3, characters 17-20: +Error: ppx_cstruct: [@len] argument should be an integer diff --git a/ppx_test/errors/cstruct_len_not_int.ml b/ppx_test/errors/cstruct_len_not_int.ml new file mode 100644 index 00000000..b373becf --- /dev/null +++ b/ppx_test/errors/cstruct_len_not_int.ml @@ -0,0 +1,6 @@ +[%%cstruct + type t = { + a: uint8_t [@len ""] + } + [@@little_endian] +] diff --git a/ppx_test/errors/cstruct_len_not_int.ml.expected b/ppx_test/errors/cstruct_len_not_int.ml.expected new file mode 100644 index 00000000..5118c334 --- /dev/null +++ b/ppx_test/errors/cstruct_len_not_int.ml.expected @@ -0,0 +1,2 @@ +File "cstruct_len_not_int.ml", line 3, characters 17-20: +Error: ppx_cstruct: [@len] argument should be an integer diff --git a/ppx_test/errors/dune.inc b/ppx_test/errors/dune.inc index d07df7fc..a12e1ba5 100644 --- a/ppx_test/errors/dune.inc +++ b/ppx_test/errors/dune.inc @@ -97,6 +97,34 @@ (action (diff cstruct_duplicate_field.ml.expected cstruct_duplicate_field.ml.errors))) +(rule + (deps pp.exe (:input cstruct_len_int32.ml)) + (targets cstruct_len_int32.ml.errors) + (action + (with-stderr-to + %{targets} + (run ./pp.exe --impl %{input})))) + +(alias + (name runtest) + (package ppx_cstruct) + (action + (diff cstruct_len_int32.ml.expected cstruct_len_int32.ml.errors))) + +(rule + (deps pp.exe (:input cstruct_len_not_int.ml)) + (targets cstruct_len_not_int.ml.errors) + (action + (with-stderr-to + %{targets} + (run ./pp.exe --impl %{input})))) + +(alias + (name runtest) + (package ppx_cstruct) + (action + (diff cstruct_len_not_int.ml.expected cstruct_len_not_int.ml.errors))) + (rule (deps pp.exe (:input cstruct_multiple_len.ml)) (targets cstruct_multiple_len.ml.errors) From 353102f7a188e595d3d708f28aa60f12af6ce6b0 Mon Sep 17 00:00:00 2001 From: Etienne Millon Date: Tue, 3 Sep 2019 17:13:07 +0200 Subject: [PATCH 2/2] Check that len > 0 --- ppx/ppx_cstruct.ml | 11 ++++++++--- ppx_test/errors/cstruct_len_zero.ml | 6 ++++++ ppx_test/errors/cstruct_len_zero.ml.expected | 2 ++ ppx_test/errors/dune.inc | 14 ++++++++++++++ 4 files changed, 30 insertions(+), 3 deletions(-) create mode 100644 ppx_test/errors/cstruct_len_zero.ml create mode 100644 ppx_test/errors/cstruct_len_zero.ml.expected diff --git a/ppx/ppx_cstruct.ml b/ppx/ppx_cstruct.ml index e5a182dc..500c9d24 100644 --- a/ppx/ppx_cstruct.ml +++ b/ppx/ppx_cstruct.ml @@ -527,12 +527,17 @@ let constr_enum = function loc_err loc "invalid cenum variant" let get_len = function - | [ ({txt = "len"; _}, + | [ ({txt = "len"; loc}, PStr [{pstr_desc = Pstr_eval ({pexp_desc = Pexp_constant (Pconst_integer (sz, None)); _}, _) - ; _}])] -> - Some (int_of_string sz) + ; _}])] + -> + let n = int_of_string sz in + if n > 0 then + Some n + else + loc_err loc "[@len] argument should be > 0" | [{txt = "len"; loc}, _ ] -> loc_err loc "[@len] argument should be an integer" | _ -> diff --git a/ppx_test/errors/cstruct_len_zero.ml b/ppx_test/errors/cstruct_len_zero.ml new file mode 100644 index 00000000..a5f3269b --- /dev/null +++ b/ppx_test/errors/cstruct_len_zero.ml @@ -0,0 +1,6 @@ +[%%cstruct + type t = { + a: uint8_t [@len 0] + } + [@@little_endian] +] diff --git a/ppx_test/errors/cstruct_len_zero.ml.expected b/ppx_test/errors/cstruct_len_zero.ml.expected new file mode 100644 index 00000000..aae1fb1c --- /dev/null +++ b/ppx_test/errors/cstruct_len_zero.ml.expected @@ -0,0 +1,2 @@ +File "cstruct_len_zero.ml", line 3, characters 17-20: +Error: ppx_cstruct: [@len] argument should be > 0 diff --git a/ppx_test/errors/dune.inc b/ppx_test/errors/dune.inc index a12e1ba5..426539ba 100644 --- a/ppx_test/errors/dune.inc +++ b/ppx_test/errors/dune.inc @@ -125,6 +125,20 @@ (action (diff cstruct_len_not_int.ml.expected cstruct_len_not_int.ml.errors))) +(rule + (deps pp.exe (:input cstruct_len_zero.ml)) + (targets cstruct_len_zero.ml.errors) + (action + (with-stderr-to + %{targets} + (run ./pp.exe --impl %{input})))) + +(alias + (name runtest) + (package ppx_cstruct) + (action + (diff cstruct_len_zero.ml.expected cstruct_len_zero.ml.errors))) + (rule (deps pp.exe (:input cstruct_multiple_len.ml)) (targets cstruct_multiple_len.ml.errors)