Make -unused-code-warnings also controls warning 34 suppression items
#510
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Update CHANGES.md Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Signed-off-by: Mathieu Barbin <[email protected]>
mbarbin
force-pushed
the
unused-code-warnings-controls-w34
branch
from
0a33b57
to
b041ff1
Compare
This allows disabling the generation of `let _ = fun (_ : t) -> ()` strucutre items for each type using derivers. The feature was meant to automatically disable warning 34 when using `[@@deriving ...]` This is inspired by ocaml-ppx#493 but adapted for ocaml-ppx#510. Signed-off-by: Mathieu Barbin <[email protected]>
mbarbin
requested review from
ceastlund,
NathanReb,
panglesd,
patricoferris and
pitag-ha
as code owners
src/deriving.ml
Outdated
| @ | ||
| match lerr with | ||
| | [] -> [] | ||
| | _ :: _ as lerr -> [ { items = lerr; unused_code_warnings = false } ] |
There was a problem hiding this comment.
@NathanReb my reasoning here was :
-
Inserting
unused_code_warnings=falseimpacts what the new code does since after these items are merged with the rest of the one generated by a ppx,types_used_by_derivingdoesn't distinguish it from ppx generated ones, and believes one of the ppx intends not to allow the warnings. Thus, it is better to omit this extra element in the list if thelerrlist to be inserted is empty (and result in the same generated items overall). -
In case
lerris not empty, (this part I am more fuzzy on), I am guessing this is a mechanism designed in ppxlib to avoid stopping on the first error reported by one ppx. I made the hypothesis that in this case the entire ppxchain is going to be aborted in the end due to processing errors, and thus the impact ofunused_code_warningscan be ignored in this code path (when errors are emitted).
There was a problem hiding this comment.
I would suggest a slightly different approach here. I think it makes things clearer if we determine whether we should silence warning 34 from the list of generators rather than from the wrapped generated items.
Getting it directly from the field that determines whether they opted out is safer than getting it from a field that derives from that. I can imagine a scenario where changes in the code could inadvertently break this behaviour, as suggests this specific change for instance.
Does that sound good to you?
There was a problem hiding this comment.
Oh, of course yes. I missed that. Thanks a lot! Done.
mbarbin
changed the title
-unused-code-warnings also controls warning 34 suppression items.-unused-code-warnings also controls warning 34 suppression items
There was a problem hiding this comment.
Thanks for taking the time to add this feature, really appreciate your contributions!
Except for the way we determine if all derivers opted out, it looks good to me!
CC @ceastlund, you might be interested in taking a look. I kind of changed my mind and decided to control warning 34 with the same flag and Deriving.make argument as you initially suggested. We'll still add a separate flag that allows only disabling the warning 34 silencing as some users (such as @mbarbin) are interested in getting this quick win out of the way without having to fix the other warnings.
| allow_unused_code_warnings | ||
| ~ppx_allows_unused_code_warnings:unused_code_warnings | ||
| in | ||
| if keep_w32_impl () || unused_code_warnings then [] |
There was a problem hiding this comment.
We should probably drop the keep_w32_impl () here, it's clearly a bug to control the unused type warnings via this flag. I'll remove it in a subsequent PR!
There was a problem hiding this comment.
This sounds good to me. After the set of changes we intend to include in the release, their will be a clear replacement for this (-unused-type-warnings). It is not likely to break existing usage, as fixing this bug could only remove warnings rather than adding new ones (right?).
There was a problem hiding this comment.
Yep that sounds about right!
src/deriving.ml
Outdated
| @ | ||
| match lerr with | ||
| | [] -> [] | ||
| | _ :: _ as lerr -> [ { items = lerr; unused_code_warnings = false } ] |
There was a problem hiding this comment.
I would suggest a slightly different approach here. I think it makes things clearer if we determine whether we should silence warning 34 from the list of generators rather than from the wrapped generated items.
Getting it directly from the field that determines whether they opted out is safer than getting it from a field that derives from that. I can imagine a scenario where changes in the code could inadvertently break this behaviour, as suggests this specific change for instance.
Does that sound good to you?
Infer it from the list of generators rather than from the wrapped generated items. This was suggested as review comment by @NathanReb. Signed-off-by: Mathieu Barbin <[email protected]>
This allows disabling the generation of `let _ = fun (_ : t) -> ()` strucutre items for each type using derivers. The feature was meant to automatically disable warning 34 when using `[@@deriving ...]` This is inspired by ocaml-ppx#493 but adapted for ocaml-ppx#510. Signed-off-by: Mathieu Barbin <[email protected]>
This allows disabling the generation of `let _ = fun (_ : t) -> ()` strucutre items for each type using derivers. The feature was meant to automatically disable warning 34 when using `[@@deriving ...]` This is inspired by ocaml-ppx#493 but adapted for ocaml-ppx#510. Signed-off-by: Mathieu Barbin <[email protected]>
- Fix a bug where `Code_path.main_module_name` would not properly remove extensions from the filename and therefore return an invalid module name. (ocaml-ppx/ppxlib#512, @NathanReb) - Add `-unused-type-warnings` flag to the driver to allow users to disable only the generation of warning 34 silencing structure items when using `[@@deriving ...]` on type declarations. (ocaml-ppx/ppxlib#511, @mbarbin, @NathanReb) - Make `-unused-code-warnings` flag to the driver also controls the generation of warning 34 silencing structure items when using `[@@deriving ...]` on type declarations. (ocaml-ppx/ppxlib#510, @mbarbin, @NathanReb) - Driver: Add `-unused-code-warnings=force` command-line flag argument. (ocaml-ppx/ppxlib#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 ]`. (ocaml-ppx/ppxlib#498, ocaml-ppx/ppxlib#502, @v-gb, @NathanReb) - Fix `Longident.parse` so it also handles indexing operators such as `.!()`, `.%(;..)<-`, or `Vec.(.%())` (ocaml-ppx/ppxlib#494, @Octachron) - Add a `special_function'` variant which directly takes a `Longident.t` argument to avoid the issue that `Longident.t` cover distinct syntaxic classes which cannot be easily parsed by a common parser (ocaml-ppx/ppxlib#496, @Octachron). - Keep location ranges consistent when migrating `Pexp_function` nodes from 5.2+ to older versions (ocaml-ppx/ppxlib#504, @jchavarri) - Fix `-locations-check` behaviour so it is no longer required to pass `-check` as well to enable location checks. (ocaml-ppx/ppxlib#506, @NathanReb) Signed-off-by: Nathan Rebours <[email protected]>
- Fix a bug where `Code_path.main_module_name` would not properly remove extensions from the filename and therefore return an invalid module name. (ocaml-ppx/ppxlib#512, @NathanReb) - Add `-unused-type-warnings` flag to the driver to allow users to disable only the generation of warning 34 silencing structure items when using `[@@deriving ...]` on type declarations. (ocaml-ppx/ppxlib#511, @mbarbin, @NathanReb) - Make `-unused-code-warnings` flag to the driver also controls the generation of warning 34 silencing structure items when using `[@@deriving ...]` on type declarations. (ocaml-ppx/ppxlib#510, @mbarbin, @NathanReb) - Driver: Add `-unused-code-warnings=force` command-line flag argument. (ocaml-ppx/ppxlib#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 ]`. (ocaml-ppx/ppxlib#498, ocaml-ppx/ppxlib#502, @v-gb, @NathanReb) - Fix `Longident.parse` so it also handles indexing operators such as `.!()`, `.%(;..)<-`, or `Vec.(.%())` (ocaml-ppx/ppxlib#494, @Octachron) - Add a `special_function'` variant which directly takes a `Longident.t` argument to avoid the issue that `Longident.t` cover distinct syntaxic classes which cannot be easily parsed by a common parser (ocaml-ppx/ppxlib#496, @Octachron). - Keep location ranges consistent when migrating `Pexp_function` nodes from 5.2+ to older versions (ocaml-ppx/ppxlib#504, @jchavarri) - Fix `-locations-check` behaviour so it is no longer required to pass `-check` as well to enable location checks. (ocaml-ppx/ppxlib#506, @NathanReb) Signed-off-by: Nathan Rebours <[email protected]>
Instead of only controlling the generation of items to suppress warnings 32 and 60, makes this flag (and the respective Deriving arg) also control the item generated to suppress warning 34 (unused type declaration).
This is based on a recent discussion with @NathanReb in #490 .