Per type warn 4 #1071
Per type warn 4 #1071
Conversation
|
Note that I've hesitated to update the manual, as I'd tend to think that the sentence
in the documentation for the |
|
What about cases where you have warning 4 enabled and you pattern match on pairs of a type with -4 and an unannotated type ? It seems to me you would need some sort of analysis for this, which is much more precise than what is currently done. |
|
I'm not sure I understand your point. In this case, I indeed expect a warning. In my opinion, this is not different to the current situation with a pair of |
|
@jberdine thanks for the pointer. I only searched for related PR on github and somehow forgot about mantis 😕 . I guess that gasche's or octachron's suggestion of having different levels for warning 4 annotation at declaration point would probably address @Drup remark. I'll try to make some suggestions in this direction in a not-too-distant future. |
The same issue that was reported for typecore.ml in PR#6394 did occur in fragile pattern-matching detection code. Fix is similar: accept the fact that in presence of recursive module some incoherence may happen.
Implements Octachron's proposal in PR#7310: warnings can now be in 4 states: - Always: they are always active - Implicit: they are active, but can be deactivated through external definitions - Explicit: they are inactive, but can activated through external definitions - Never: they are always inactive. `warn-error` keeps its semantics: if the warning is triggered (regardless of the activation state), a compilation error will follow. Warning modifiers in the warning string are now the following - `+` put the corresponding warnings in Implicit state - `-` in Explicit state - `++` in Always state - `--` in Never state - `@` in Implicit state and triggers an error - `@@` in Always state and triggers an error. as of now, only warning 4 takes advantage of the four states. For any other warning, `Always` and `Implicit` merely indicate that the warning is active, and `Never` and `Explicit` that it is inactive.
vprevosto
force-pushed
the
per-type-warn-4
branch
from
a81ee6b
to
25a131a
Compare
|
I've pushed a proposal implementing octachron's suggestion in the related mantis issue: a warning can now have 4 statuses:
I've also modified the parsing of the warning string to set these various statuses:
Remarks are welcome. |
typing/parmatch.ml
Outdated
| | 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 |
typing/parmatch.ml
Outdated
| Builtin_attributes.with_warning_attribute | ||
| decl.Types.type_attributes | ||
| (fun () -> Warnings.(is_active warn)) | ||
|
|
There was a problem hiding this comment.
nit: indent 2 spaces less?
utils/warnings.ml
Outdated
| | true, true -> Implicit | ||
| | false, true -> Explicit | ||
| | false, false -> Never | ||
| ;; |
There was a problem hiding this comment.
nit: indent 2 spaces less
utils/warnings.ml
Outdated
| 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 |
There was a problem hiding this comment.
Maybe 'rigid' instead of 'strict' if you want an antonym of 'flexible'?
There was a problem hiding this comment.
This renaming makes sense indeed. Thanks for the suggestion.
typing/parmatch.ml
Outdated
| (if extendable_path path && warn_fragile_type p.pat_type p.pat_env | ||
| then add_path path r else r) | ||
| ps | ||
| | None -> r) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 psHere 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.
There was a problem hiding this comment.
@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.
| | 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. *) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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!
There was a problem hiding this comment.
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.
|
Is there a link between this PR and #1248? |
|
|
||
| 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. | ||
| *) |
There was a problem hiding this comment.
It's either PR or MPR, we never used the MR abbreviation.
There was a problem hiding this comment.
Aren't there some GPR as well?
There was a problem hiding this comment.
Yes there is also GPR but it's a different set of PRs. I was just talking about the naming of Mantis Problem Reports.
- fixes indentation and spacing - small code refactoring - renaming of local variables
|
I like the general direction (althrough I'd personnally be more interested to have the equivalent for warning 9). But I don't think one should re-use the existing I'd prefer to introduce an ad hoc attribute on the type declaration (e.g. [@@ocaml.no_fragile_match]). It would only suppress the warning, and there should be no way to force issuing the warning if the "use site" disable the warning explicitly. |
+1, I couldn't agree more. |
|
cc @alainfrisch |
|
Sorry, I missed that as well. Personally, I'm not convinced by the 4-level controls over warning. For many warnings, the finer distinction is not useful, and there could be cases where there could be multiple different "sources" for enabling a specific warning, and one might want to provide different controls depending on the source. For instance, for extensible constructors, one could imagine some warnings enabled from the type declaration or from the constructor definition. (I'm not claiming that it necessarily makes sense to use different settings depending on the source, only that it's not impossible that one might want to do so.) There are already several instances of similar warnings with different numbers, used to distinguish variants of the same situation (e.g. 37/38, 20/26/27/32/35/36, 11/12, 33/66). I believe we could do the same here, and simply introduce a new warning, similar to 9, but triggered by a specific annotation on the type declaration. |
|
I had forgotten about this work. I think it is nice and important, and that we should consider trying to get it merged. In addition, since the time at which it originally appeared, I have gotten much more familiar with the pattern-matching analyses, to the point that I could probably review the implementation myself. On the other hand, I don't like the user interface at all. |
|
Note: Nowadays people can write What is the status of this PR? Reading the discussion again, I think that the main problem is that we don't have a clear consensus on what we want. There were complaints about the previous UI ( What do we want? Clearly people care about the feature itself. So now we need to agree on how to express it. Personally, I'm not so sure anymore that I really want a dedicated attribute, because there are various other warnings that could benefit from the same treatment (Alain mentioned |
…d stubs" (ocaml#1071) This reverts commit 29cabeb.
This MR allows to enable (or disable) warning 4 (fragile pattern-matching) on a per-type basis instead of globally. More specifically, if an attribute
ocaml.warningwith a payload containing+4(resp-4) is associated to the definition of a sum typet, this attribute will be honored for any pattern-matching over a value of typet, regardless of the status of warning 4 at that point.The rationale behind that is that writing non-fragile pattern-matching is pretty cumbersome and leads to code that is difficult to read (as a matter of fact, even the OCaml compiler disables it for its own compilation). On the other hand, knowing which patterns are fragile is extremely useful when one is adding a constructor, but only if only the patterns of the type being extended are reported (otherwise, there will very likely be too much noise from fragile matching over irrelevant types), which is exactly what this PR allows.
Additionally, the fragile matching detection code is now called on every matching, which uncovered an existing issue (just launch current ocamlc -w +4 over the code of PR #6394 to see it happen). This is fixed in the second commit of the series.