Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add driver flag to force opting in to unused code warnings in derived code #490

Merged
merged 1 commit into from
Jul 15, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions CHANGES.md
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,8 @@ details.

### Other changes

- Driver: Add `-unused-code-warnings=force` command-line flag argument. (#490, @mbarbin)

- new functions `Ast_builder.{e,p}list_tail` that take an extra tail
expression/pattern argument parameter compared to `Ast_builder.{e,p}list`, so
they can build ASTs like `a :: b :: c` instead of only `[ a; b ]`.
Expand Down
17 changes: 15 additions & 2 deletions doc/driver.mld
Original file line number Diff line number Diff line change
Expand Up @@ -91,8 +91,21 @@ driver.exe [extra_args] [<files>]
-no-merge Do not merge context free transformations (better for debugging rewriters). As a result, the context-free transformations are not all applied before all impl and intf.
-cookie NAME=EXPR Set the cookie NAME to EXPR
--cookie Same as -cookie
-help Display this list of options
--help Display this list of options
-deriving-keep-w32 {impl|intf|both}
Do not try to disable warning 32 for the generated code
-deriving-disable-w32-method {code|attribute}
How to disable warning 32 for the generated code
-type-conv-keep-w32 {impl|intf|both}
Deprecated, use -deriving-keep-w32
-type-conv-w32 {code|attribute}
Deprecated, use -deriving-disable-w32-method
-deriving-keep-w60 {impl|intf|both}
Do not try to disable warning 60 for the generated code
-unused-code-warnings {true|false|force}
Allow ppx derivers to enable unused code warnings (default: false)
-help Display this list of options
--help Display this list of options

v}
{%html: </details>%}

Expand Down
24 changes: 18 additions & 6 deletions src/deriving.ml
Original file line number Diff line number Diff line change
Expand Up @@ -63,10 +63,20 @@ let allow_unused_code_warnings = ref Options.default_allow_unused_code_warnings

let () =
Driver.add_arg "-unused-code-warnings"
(Bool (( := ) allow_unused_code_warnings))
~doc:"_ Allow ppx derivers to enable unused code warnings"

let allow_unused_code_warnings () = !allow_unused_code_warnings
(Symbol
( [ "true"; "false"; "force" ],
function
| "true" -> allow_unused_code_warnings := True
| "false" -> allow_unused_code_warnings := False
| "force" -> allow_unused_code_warnings := Force
| _ -> assert false ))
~doc:" Allow ppx derivers to enable unused code warnings (default: false)"

let allow_unused_code_warnings ~ppx_allows_unused_code_warnings =
match !allow_unused_code_warnings with
| Force -> true
| False -> false
| True -> ppx_allows_unused_code_warnings

module Args = struct
include (
Expand Down Expand Up @@ -631,7 +641,8 @@ let wrap_str ~loc ~hide st =
let wrap_str ~loc ~hide ~unused_code_warnings st =
let loc = { loc with loc_ghost = true } in
let unused_code_warnings =
unused_code_warnings && allow_unused_code_warnings ()
allow_unused_code_warnings
~ppx_allows_unused_code_warnings:unused_code_warnings
in
let warnings, st =
if keep_w32_impl () || unused_code_warnings then ([], st)
Expand Down Expand Up @@ -671,7 +682,8 @@ let wrap_sig ~loc ~hide st =
let wrap_sig ~loc ~hide ~unused_code_warnings sg =
let loc = { loc with loc_ghost = true } in
let unused_code_warnings =
unused_code_warnings && allow_unused_code_warnings ()
allow_unused_code_warnings
~ppx_allows_unused_code_warnings:unused_code_warnings
in
let warnings =
if keep_w32_intf () || unused_code_warnings then [] else [ 32 ]
Expand Down
6 changes: 5 additions & 1 deletion src/options.ml
Original file line number Diff line number Diff line change
@@ -1,4 +1,8 @@
let default_allow_unused_code_warnings = false
module Forcable_bool = struct
type t = True | False | Force
end

let default_allow_unused_code_warnings : Forcable_bool.t = False
let perform_checks = false

(* The checks on extensions are only to get better error messages
Expand Down
77 changes: 69 additions & 8 deletions test/deriving_warning/run.t
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,9 @@ triggering warnings, we also added optional arguments to `Deriving.make` so that
the ppx themselves can declare whether they need the driver to disable warnings
or not.

The following tests describe the behaviour of flags and features used to control
the emission of such warning silencing features.

One such flag and optional argument pair is the `-unused-code-warnings` flag and
`?unused_code_warning` `Deriving.V2.make` argument. Both of those default to
`false` and control whether we disable warnings 32 and 60 for generated code
Expand All @@ -15,8 +18,10 @@ and behave as described by the following table:
-------------|-------------|----------------------
true | true | Enabled
true | false | Disabled*
true | force | Enabled
false | true | Disabled
false | false | Disabled
false | force | Enabled
* By adding warning silencers like [@@@ocaml.waring "-60"] or producing code like
`let _ = zero in...`

Expand All @@ -26,15 +31,15 @@ We have a driver with 4 derivers linked in:
- two_do_warn
- alias_warn

----------------------------------------
--------------------------------------------------------------------------------

Let's consider the following ocaml source file using the zero_do_warn deriver

$ cat > zero_do_warn.ml << EOF
> type t = int [@@deriving zero_do_warn]
> EOF

Zero_do_warn is registered with unused_code_warning set true meaning it allows
Zero_do_warn is registered with unused_code_warning set to true meaning it allows
the driver not to silence unused code and unused module warnings if the
-unused-code-warning flag is set to true.

Expand Down Expand Up @@ -96,7 +101,7 @@ and compare the result of both driver invocations:
"@inline"]
[@@merlin.hide ]

-----------------------------------------------
--------------------------------------------------------------------------------

The default value of the -unused-code-warnings should be false:

Expand All @@ -116,7 +121,24 @@ The default value of the -unused-code-warnings should be false:
As we can see here, the warnings were disabled by the driver, as is expected
with -unused-code-warnings=false.

-----------------------------------------------
--------------------------------------------------------------------------------

There is another value possible for the -unused-code-warnings flag: "force".
This allows the warnings to be enabled even if the deriver does not allow it. In
this example though, using `force` or `true` results in the same output, since
the deriver `zero_do_warn` already allows the warning to be enabled.

$ ./driver.exe -unused-code-warnings=force -impl zero_do_warn.ml
type t = int[@@deriving zero_do_warn]
include struct let _ = fun (_ : t) -> () end[@@ocaml.doc "@inline"][@@merlin.hide
]
include struct module Zero = struct type t =
| T0 end
let zero = Zero.T0 end[@@ocaml.doc "@inline"][@@merlin.hide ]

We'll see below other examples where the `force` flag is actually useful.

--------------------------------------------------------------------------------

Let's consider the following ocaml source file using the one_no_warn deriver

Expand All @@ -125,9 +147,9 @@ Let's consider the following ocaml source file using the one_no_warn deriver
> EOF

One_no_warn is registered with unused_code_warning set to false, meaning the driver
should always disable warnings for the generated code, regardless of the value of
the -unused-code-warning flag. The following driver invocations have the same
output:
should disable warnings for the generated code, even when the value of the
-unused-code-warning is set to true. The following driver invocations have the
same output:

$ ./driver.exe -unused-code-warnings=false -impl one_no_warn.ml
type t = int[@@deriving one_no_warn]
Expand Down Expand Up @@ -177,7 +199,30 @@ Same goes for .mli files:
val one : Zero.t One.t
end[@@ocaml.doc "@inline"][@@merlin.hide ]

-------------------------------------------------
--------------------------------------------------------------------------------

When using a deriving that does not allow the warning to be enabled (such as
`one_no_warn` here), it is still possible to force it from the user side. That's
what the `force` argument for the driver flag is for. See below:

$ ./driver.exe -unused-code-warnings=force -impl one_no_warn.ml
type t = int[@@deriving one_no_warn]
include
struct
let _ = fun (_ : t) -> ()
module One = struct type 'a t =
| T1 of 'a end
let one = One.T1 zero
end[@@ocaml.doc "@inline"][@@merlin.hide ]

Same goes for .mli files:

$ ./driver.exe -unused-code-warnings=force -intf one_no_warn.mli
type t = int[@@deriving one_no_warn]
include sig module One : sig type 'a t end val one : Zero.t One.t end
[@@ocaml.doc "@inline"][@@merlin.hide ]

--------------------------------------------------------------------------------

The alias_warn deriver is in fact an alias for two derivers:
- alias_do_warn, which is registered with unused_code_warnings=true
Expand Down Expand Up @@ -214,6 +259,17 @@ the unused-code-warnings flag, there will be one for both:
include struct let unit_two = unit_one
let _ = unit_two end[@@ocaml.doc "@inline"][@@merlin.hide ]

As expected, if we force the unused-code-warnings, there will be no let _ for
any of the two values.

$ ./driver.exe -unused-code-warnings=force -impl alias_warn.ml
type t = int[@@deriving alias_warn]
include struct let _ = fun (_ : t) -> () end[@@ocaml.doc "@inline"][@@merlin.hide
]
include struct let unit_one = () end[@@ocaml.doc "@inline"][@@merlin.hide ]
include struct let unit_two = unit_one end[@@ocaml.doc "@inline"][@@merlin.hide
]

Same goes for .mli:

$ cat > alias_warn.mli << EOF
Expand All @@ -235,3 +291,8 @@ Same goes for .mli:
include sig [@@@ocaml.warning "-32"] val unit_two : unit end[@@ocaml.doc
"@inline"]
[@@merlin.hide ]

$ ./driver.exe -unused-code-warnings=force -intf alias_warn.mli
type t = int[@@deriving alias_warn]
include sig val unit_one : unit end[@@ocaml.doc "@inline"][@@merlin.hide ]
include sig val unit_two : unit end[@@ocaml.doc "@inline"][@@merlin.hide ]
Loading