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

Per type warn 4 #1071

Open
wants to merge 6 commits into
base: trunk
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
31 changes: 31 additions & 0 deletions testsuite/tests/warnings/w04.ml
Expand Up @@ -10,3 +10,34 @@ type t = A | B
let g x = match x with
| A -> 0
| _ -> 1

type u = C | D [@@ ocaml.warning "-4"]
(* should not warn. *)
let h x = match x with
| C -> 0
| _ -> 1

[@@@ocaml.warning "-4"]

type v = F | G [@@ ocaml.warning "+4"]

let k x = match x with
| F -> 0
| _ -> 1

(* should not warn. *)
let l x = match x with
| A -> 0
| _ -> 1

[@@@ocaml.warning "--4"]
(* should not warn. *)
let m x = match x with
| F -> 0
| _ -> 1

[@@@ocaml.warning "++4"]
(*should warn. *)
let n x = match x with
| C -> 0
| _ -> 1
6 changes: 6 additions & 0 deletions testsuite/tests/warnings/w04.reference
@@ -1,3 +1,9 @@
File "w04.ml", line 10, characters 10-40:
Warning 4: this pattern-matching is fragile.
It will remain exhaustive when constructors are added to type t.
File "w04.ml", line 24, characters 10-40:
Warning 4: this pattern-matching is fragile.
It will remain exhaustive when constructors are added to type v.
File "w04.ml", line 41, characters 10-40:
Warning 4: this pattern-matching is fragile.
It will remain exhaustive when constructors are added to type u.
50 changes: 37 additions & 13 deletions typing/parmatch.ml
Expand Up @@ -127,8 +127,10 @@ let clean_copy ty =
let get_type_path ty tenv =
let ty = Ctype.repr (Ctype.expand_head tenv (clean_copy ty)) in
match ty.desc with
| Tconstr (path,_,_) -> path
| _ -> fatal_error "Parmatch.get_type_path"
| Tconstr (path,_,_) -> Some path
| _ -> None
(* Same as PR#6394: we have an incoherence due to recursive module. It
will result in a type error later on. *)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One does indeed get a fatal error here on the small example from MPR#6394, however I feel like this change should be proposed independently of the current GPR.
The change is indeed somewhat unrelated from the rest of the PR and should IMO be discussed independently.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To make my point a bit more convincing I'd like to point out that this bugfix change was written in february, so could have been included in 4.04.X and 4.05.0!

Thanks!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, I've to admit that I'd hoped this GPR to be reviewed a bit more quickly 😄 . That said, I'm a bit reluctant to extract the corresponding commit (albeit tiny, there will be a conflict somewhere), as I'm pretty sure that current usage of warning 4 is extremely limited due its impracticality on any large scale development as long as you can only enable it at global level.


(*************************************)
(* Values as patterns pretty printer *)
Expand Down Expand Up @@ -670,7 +672,9 @@ let should_extend ext env = match ext with
| Tpat_construct
(_, {cstr_tag=(Cstr_constant _|Cstr_block _|Cstr_unboxed)},_) ->
let path = get_type_path p.pat_type p.pat_env in
Path.same path ext
(match path with
| Some path -> Path.same path ext
| None -> false)
| Tpat_construct
(_, {cstr_tag=(Cstr_extension _)},_) -> false
| Tpat_constant _|Tpat_tuple _|Tpat_variant _
Expand Down Expand Up @@ -802,8 +806,11 @@ let build_other ext env = match env with
{lid with txt="*extension*"})) Ctype.none Env.empty
| ({pat_desc = Tpat_construct _} as p,_) :: _ ->
begin match ext with
| Some ext when Path.same ext (get_type_path p.pat_type p.pat_env) ->
extra_pat
| Some ext ->
let path = (get_type_path p.pat_type p.pat_env) in
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: extra space after =

(match path with
| Some path when Path.same ext path -> extra_pat
| _ -> build_other_constrs env p)
| _ ->
build_other_constrs env p
end
Expand Down Expand Up @@ -1869,14 +1876,28 @@ let extendable_path path =
Path.same path Predef.path_unit ||
Path.same path Predef.path_option)

let warn_fragile_type ty env =
let _,_,decl = Ctype.extract_concrete_typedecl env ty in
let warn = Warnings.Fragile_match "" in
match Warnings.status warn with
| Warnings.Always -> true
| Warnings.Never -> false
| Warnings.Implicit | Warnings.Explicit ->
Builtin_attributes.with_warning_attribute
decl.Types.type_attributes
(fun () -> Warnings.(is_active warn))

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: indent 2 spaces less?

let rec collect_paths_from_pat r p = match p.pat_desc with
| Tpat_construct(_, {cstr_tag=(Cstr_constant _|Cstr_block _|Cstr_unboxed)},ps)
->
let path = get_type_path p.pat_type p.pat_env in
List.fold_left
collect_paths_from_pat
(if extendable_path path then add_path path r else r)
ps
(match get_type_path p.pat_type p.pat_env with
| Some path ->
List.fold_left
collect_paths_from_pat
(if extendable_path path && warn_fragile_type p.pat_type p.pat_env
then add_path path r else r)
ps
| None -> r)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any hint about the behavior of this line? In particular, it's not obvious to me why to return r instead of ps, or even error already.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

r is the accumulator (the list of paths we've seen so far), while ps is a list of patterns : the arguments of the constructor, i.e. if p is Foo (a, _, None) then ps is [Tpat_var "a"; Tpat_any; Tpat_construct "None"].
Returning ps wouldn't make sense (it wouldn't type), while returning r does.

One a similar note one can wonder why the code is not instead:

let r =
  match get_type_path ... with
  | Some p when extendable_path p -> add_path p r
  | _ -> r
in
List.fold_left collect_paths_from_pat r ps

Here my guess would be "well, we already know we're in a nonsensical situation, so we might as well not do any extra work". It would be nice if the author could add a comment in the code clarifying that though.

As for the last question "why not error", get_type_path was previously erroring but was modified not to anymore (and return an option instead), it would be of little benefit if we failed when looking at the result.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@trefis you're right. The None case is here to avoid a fatal error in a particular corner case (type error within a recursive module, as described in Mantis issue 6394). A proper type error will be generated somewhat later, and we can mostly ignore the patterns related to that type.

Regarding the way the code is written, your guess is wrong 😄 . It's just that the modification done for supporting per-type warn 4 and fixing PR 6394 were done in two separate commits and I forgot to refactor the code at that time. Your proposal is indeed clearer, and I'll fix up the code.

| Tpat_any|Tpat_var _|Tpat_constant _| Tpat_variant (_,None,_) -> r
| Tpat_tuple ps | Tpat_array ps
| Tpat_construct (_, {cstr_tag=Cstr_extension _}, ps)->
Expand All @@ -1896,7 +1917,7 @@ let rec collect_paths_from_pat r p = match p.pat_desc with
(*
Actual fragile check
1. Collect data types in the patterns of the match.
2. One exhautivity check per datatype, considering that
2. One exhaustivity check per datatype, considering that
the type is extended.
*)

Expand All @@ -1910,6 +1931,8 @@ let do_check_fragile_param exhaust loc casel pss =
| _ -> match pss with
| [] -> ()
| ps::_ ->
let old_status = Warnings.is_active (Warnings.Fragile_match "") in
Warnings.parse_options false "+4";
List.iter
(fun ext ->
match exhaust (Some ext) pss (List.length ps) with
Expand All @@ -1918,7 +1941,8 @@ let do_check_fragile_param exhaust loc casel pss =
loc
(Warnings.Fragile_match (Path.name ext))
| Rsome _ -> ())
exts
exts;
if not old_status then Warnings.parse_options false "-4"

(*let do_check_fragile_normal = do_check_fragile_param exhaust*)
let do_check_fragile_gadt = do_check_fragile_param exhaust_gadt
Expand Down Expand Up @@ -2034,7 +2058,7 @@ let check_partial_param do_check_partial do_check_fragile loc casel =
let pss = get_mins le_pats pss in
let total = do_check_partial loc casel pss in
if
total = Total && Warnings.is_active (Warnings.Fragile_match "")
total = Total
then begin
do_check_fragile loc casel pss
end ;
Expand Down
48 changes: 39 additions & 9 deletions utils/warnings.ml
Expand Up @@ -193,16 +193,20 @@ let letter = function
| _ -> assert false
;;

type status = Always | Implicit | Explicit | Never

type state =
{
active: bool array;
flexible: bool array;
error: bool array;
}

let current =
ref
{
active = Array.make (last_warning_number + 1) true;
flexible = Array.make (last_warning_number + 1) true;
error = Array.make (last_warning_number + 1) false;
}

Expand All @@ -211,12 +215,27 @@ let backup () = !current
let restore x = current := x

let is_active x = (!current).active.(number x);;

let status x =
let n = number x in
match (!current).active.(n), (!current).flexible.(n) with
| true, false -> Always
| true, true -> Implicit
| false, true -> Explicit
| false, false -> Never
;;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: indent 2 spaces less


let is_error x = (!current).error.(number x);;

let parse_opt error active flags s =
let set i = flags.(i) <- true in
let clear i = flags.(i) <- false in
let set_all i = active.(i) <- true; error.(i) <- true in
let parse_opt error active flexible flags s =
let set i = flags.(i) <- true; flexible.(i) <- true in
let set_strict i = flags.(i) <- true; flexible.(i) <- false in
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe 'rigid' instead of 'strict' if you want an antonym of 'flexible'?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This renaming makes sense indeed. Thanks for the suggestion.

let clear i = flags.(i) <- false; flexible.(i) <- true in
let clear_strict i = flags.(i) <- false; flexible.(i) <- false in
let set_all i = active.(i) <- true; error.(i) <- true; flexible.(i) <- true in
let set_all_strict i =
active.(i) <- true; error.(i) <- true; flexible.(i) <- false
in
let error () = raise (Arg.Bad "Ill-formed list of warnings") in
let rec get_num n i =
if i >= String.length s then i, n
Expand All @@ -233,6 +252,7 @@ let parse_opt error active flags s =
else
i, n1, n1
in
let need_char i = if i + 1 >= String.length s then error () in
let rec loop i =
if i >= String.length s then () else
match s.[i] with
Expand All @@ -242,9 +262,18 @@ let parse_opt error active flags s =
| 'a' .. 'z' ->
List.iter clear (letter s.[i]);
loop (i+1)
| '+' -> loop_letter_num set (i+1)
| '-' -> loop_letter_num clear (i+1)
| '@' -> loop_letter_num set_all (i+1)
| '+' ->
need_char i;
if s.[i+1] = '+' then loop_letter_num set_strict (i+2)
else loop_letter_num set (i+1)
| '-' ->
need_char i;
if s.[i+1] = '-' then loop_letter_num clear_strict (i+2)
else loop_letter_num clear (i+1)
| '@' ->
need_char i;
if s.[i+1] = '@' then loop_letter_num set_all_strict (i+2)
else loop_letter_num set_all (i+1)
| _ -> error ()
and loop_letter_num myset i =
if i >= String.length s then error () else
Expand All @@ -267,8 +296,9 @@ let parse_opt error active flags s =
let parse_options errflag s =
let error = Array.copy (!current).error in
let active = Array.copy (!current).active in
parse_opt error active (if errflag then error else active) s;
current := {error; active}
let flexible = Array.copy (!current).flexible in
parse_opt error active flexible (if errflag then error else active) s;
current := {error; active; flexible}

(* If you change these, don't forget to change them in man/ocamlc.m *)
let defaults_w = "+a-4-6-7-9-27-29-32..39-41..42-44-45-48-50-60";;
Expand Down
15 changes: 15 additions & 0 deletions utils/warnings.mli
Expand Up @@ -78,9 +78,24 @@ type t =
| Constraint_on_gadt (* 62 *)
;;

(* For warnings that can be enabled by attributes on external declarations,
the status indicates whether we should follow the current state of the
warning or the one given by those external declarations.

An example of such warning is Fragile_match, which can be (de)activated on a
per-type basis. See PR#7310 for an extended discussion.
*)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

MR#7310 instead of PR#7310?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's either PR or MPR, we never used the MR abbreviation.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Aren't there some GPR as well?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes there is also GPR but it's a different set of PRs. I was just talking about the naming of Mantis Problem Reports.

type status =
| Always (* warning always active. *)
| Implicit (* warning active, but external declarations may deactivate it.*)
| Explicit (* warning not active, external declaration may activate it.*)
| Never (* warning never active. *)
;;

val parse_options : bool -> string -> unit;;

val is_active : t -> bool;;
val status : t -> status;;
val is_error : t -> bool;;

val defaults_w : string;;
Expand Down