Thanks for working on this!

I'm not fond of the name "semi-opaque" which suggests a semantic meaning (while the behavior is very heuristic and under-specified). What about a more explicit literal_pattern_deprecated? (It could make sense to have the symmetric literal_value_deprecated to avoid magic values at construction time as well.)

The fact that, if I understand correctly, it only applies to single-argument constructors so far is a bit hackish. Would it be unreasonably hard to annotate the constructor argument instead?

(Also as always it would be nice to have this feature exercised somewhere in the testsuite.)

Small question, would a warning be emitted in the following case?

type failure = Case1 | Case2
exception Error of failure

try
  <some code>
with
| Error Case1 -> ()
| Error Case2 -> ()

(EDIT: assuming the Error exception is declared as "semi opaque", of course)

@gasche : no problem for changing the name (I don't expect it to be widely used by user code anyway). Annotating arguments is more tricky since the type-checker does not keep track of attribute on type expressions (nor on expressions, patterns, etc), but only on "declarations" (including those on constructors as a whole). It wouldn't be too hard to detect the attribute on each argument and keep this information in the constructor description (perhaps by synthesizing an attribute whose payload identifies the argument number, to avoid changing the data structure for such a rare case) for later inspection by the type-checker (when analyzing the constructor pattern). But I don't think this is worth the trouble. What could be useful and easier would be to trigger the warning as soon as a constant appear under the constructor pattern. So a constructor such as Assert_failure could be annotated and this would trigger the warning when any of the argument is matched with a constant.

@Gbury : no currently, the warning is only triggered when the argument pattern is a constant literal (i.e. basic values only: string, int, ...).

On Mon, 5 Oct 2015 08:19:11 -0700, Alain Frisch wrote:

@gasche : no problem for changing the name (I don't expect it to be widely used by user code anyway).

How about “ocaml.informational_strings”?

How about reusing the “Fragile pattern matching” warning (expanding its definition)?

Annotating arguments is more tricky since the type-checker does not keep track of attribute on type expressions (nor on expressions, patterns, etc), but only on "declarations" [...] What could be useful and easier would be to trigger the warning as soon as a constant appear under the constructor pattern. So a constructor such as Assert_failure could be annotated and this would trigger the wa rning when any of the argument is matched with a constant.

That could be useful.

@Chris00 the idea of reusing "fragile pattern matching" is clever, but it is rather problematic to extend the application area of a warning a posteriori (because users like to assume that, if a particular warning is not raised by their code today, then they can safely mark it as an error -- except for "deprecated" blah which extends its semantics a bit too often, and I suspect that even that is a mistake).

@alainfrisch Sorry for being unclear: I was assuming that annotating each argument would, indeed, unreasonably increase the complexity of the patch, but I thought that maybe there was a hidden miracle (one never knows). I agree raising if one of the arguments is a constant literal (not a constant variant constructor) is a good idea.

it is rather problematic to extend the application area of a warning a posteriori

Do you have evidence of it, i.e. users complaining about the same warning codes producing more warnings with a new release?

If we avoid warning extension, we can provide a strong guarantee that a code that is warning-free today under a given set of warning will remain warning-free tomorrow with the same set of warnings. There is evidence that users are interested (see PR#6873), and this cannot work if we change extend the warning area of existing warnings -- or at least we would need to delimit clearly which warnings are "stable" and which are not.

The issue is that many warnings are not formally defined, and are either based on "implementation details" of the type checker or on heuristics. I don't see this as fundamentally different from warnings produced by C compilers based on their optimization levels (more aggressive optimizations can uncover more defects, and newer versions of the compiler can become better).

Do you have evidence of it, i.e. users complaining about the same warning codes producing more warnings with a new release?

Definitely! Our development versions of Frama-C are compiled with -warn-error, having to patch the Makefile of a 3/4 years-old version because the semantics of a warning has changed is a major inconvenience. The new numbering scheme based on integers has been introduced (IIRC) because few letters remained. It is really a problem to consume a few ints?

How about reusing the “Fragile pattern matching” warning (expanding its definition)?

At the risk of sounding picky, this is a pretty terrible choice of warning to expand. I have the impression that this warning is not often active, because it is too burdensome. Few people would benefit from the new diagnostics.

Definitely! Our development versions of Frama-C are compiled with -warn-error, having to patch the Makefile of a 3/4 years-old version because the semantics of a warning has changed is a major inconvenience.

I don't have an opinion on the point in case, but I'd just like to mention that since you choose to compile with -warn-error you will also be subject to the addition of new default warnings, so I would say that this is not such a good argument, as there are a few ones that*should actually be introduced (e.g. a good one fore the M.() notation).

At the risk of sounding picky, this is a pretty terrible choice of warning to expand. I have the impression that this warning is not often active, because it is too burdensome. Few people would benefit from the new diagnostics.

Yes. It should simply be enabled by default (which will lead to the same problem for you and your -warn-error).

As I already said a lot of times I think that the notion of activable warnings should be killed as it leads to programmers having a different view of the language. I personally never, ever tweak warnings in my project and hope that dev team actually designs me a good default language rather than solve design disputes through the addition of activable warnings.

In the rare cases were unwarranted warnings should be disabled this should be done via attributes on particular expressions (iirc this is now possible).

Note that this doesn't mean that the non-default warnings are not useful but I think that some of them should simply be provided under the form of a separate (and maybe more powerful) static analysis tool or linter.

since you choose to compile with -warn-error you will also be subject to the addition of new default warnings

No, you can explicitly disable the "default warning set" (-a) and enable just a few hand-selected ones.

I personally never, ever tweak warnings in my project and hope that dev team actually designs me a good default language rather than solve design disputes through the addition of activable warnings.

I think it is important to accept relativism here. You have a very personal development methodology with very reactive changes throughout a small, personally-crafted and gardened codebase. This approach may not be applicable to other development methodologies, and a lot of people have the inverse problem of have a codebase too large for them to easily apply style changes as they get proposed/warned upstream. We can, and I think we should, cater for both scenarios.

@alainfrisch

I've re-used the "deprecated" warning for now.

This new thing has nothing to do with deprecation, so you shouldn't reuse this warning (and the name of the annotation shouldn't include the word deprecated).

Likewise, this new check is only very loosely related to the notion of fragile pattern-matching, so you shouldn't reuse that either.

I'm surprised that you, of all people, would hesitate to add a new warning :-)

Do you have evidence of it, i.e. users complaining about the same warning codes producing more warnings with a new release?

I get hurt by it every time. Consider this a complaint from a user.

The issue is that many warnings are not formally defined, and are either based on "implementation details" of the type checker or on heuristics.

You're committing a variant of the Valhalla fallacy. Just because the current situation is not perfect, doesn't mean we should feel free to make it worse. In this case, I think we want to break the old code explicitly, because otherwise it's going to break silently when we change the strings, but in most other cases we should strive for compatibility.

This approach may not be applicable to other development methodologies, and a lot of people have the inverse problem of have a codebase too large for them to easily apply style changes as they get proposed/warned upstream.

Well that's a broken methodology then. Warnings are not introduced for the sake of annoying people, they are introduced to show that your code base has a problem or will become problematic in the future. If your methodology ignores that I wouldn't trust you in producing quality software in the long run.

Besides I rely on default sets of warnings because I believe in modularity which means that I have dozens of independent projects and I can't just spent my time tweaking them on each project, if you want to scale modularity you need to be able to rely on good defaults from the base infrastructure. Note that this is not a "personal development methodology", if you take the MirageOS project they also have in the hundreth of different independent packages to manage and suffer from the same problems.

Besides I rely on default sets of warnings because I believe in modularity which means that I have dozens of independent projects and I can't just spent my time tweaking them on each project, if you want to scale modularity you need to be able to rely on good defaults from the base infrastructure

@dbuenzli oh come on, is it really so hard to write

<src/**/*.ml>: warn(+a), warn(-4), warn(-44), warn(-50)

in a _tags file? They are pretty useful (caught many bugs for me, especially the "unused variable" stuff). If you don't add those warnings from the beginning, it can be very painful to update a project so that it compiles without warnings again.

Of course if you enable warn error and -w +a then you choose to be annoyed by new warnings.

Le jeudi, 8 octobre 2015 à 14:40, Simon Cruanes a écrit :

@dbuenzli oh come on, is it really so hard to write
<src/**/*.ml>: warn(+a), warn(-4), warn(-44), warn(-50)

Yes — and furthermore how do I update the set in my dozens of repos when new OCaml versions are out with interesting new warnings ? Manually ? No thanks. How do I discover which set of warnings is the right one ? Sorry I don't have the time for that.

And looking at this line my only reaction is, what a terrible user interface, what I'm suppose to infer is being checked by this cryptic line ? If at least there were designed classes or levels of warnings rather than this insane micro management interface.

Good design relies on good defaults. I want to work with a language that is designed, i.e. where designers make well thought out choices to liberate me of having to perform these.

Daniel

IMO this should be a new warning, and it should be turned on by default. If we're going to be changing the strings returned by exceptions (which I think is fair), it should definitely be a warning, if not an error, to match on said strings. All projects doing this should be updated.

(I have some design drafts in my garage for a better user-interface where warnings have a name. Later.)

So, can anyone suggest a better name for the attribute? Damien doesn't like the deprecated part of literal_value_deprecated, and informational_strings is not very accurate (the attribute applies to arbitrary constant literals).

Alternatively, we could get rid of the user-facing attribute altogether, since the most important goal is to get the warning on the built-in exception constructors (this can be dealt with an internal attribute).

dont_pattern_match_that_string :-)

How about avoid_matching?

What about fragile_literal_parameters? The meaning is to indicate "the values of this parameter are constant values of a large type, but they are expected to change over the lifetime of the library". As a consequence of this meaning, it is unreasonable to use them in a pattern.

Unrelated remark: it is possible to silence the warning by rewriting Failure "foo" into Failure f when f = "foo" (which does not make the code more robust, but at least is explicit about its fiddliness).

Unrelated remark: it is possible to silence the warning by rewriting Failure "foo" into Failure f when f = "foo" (which does not make the code more robust, but at least is explicit about its fiddliness).

Yes, this works.

I quite like fragile_literal_parameters (or brittle_literal_parameters). A slightly different version that focuses more on the stability level might be unstable_literal_parameters.

Nevertheless, both options inform on a potential ulterior problem without expanding on the cause of this problem. Maybe the attribute should explain that the value of the parameters should be not be relied upon because there are partially arbitrary and are therefore bound to wiggle around their current values.

In this case, a name like noncanonical_literal_parameters might convey better the fact that the parametrers should not be not matched upond due to their arbitrariness.

I prefer avoid_matching, it is straight to the point, easy to understand, and works without having to define new bureaucratic terminology to be able to understand what the warning means (namely the notions of fragile or noncanonical).

How about something along the lines of warn_if_matched? It avoids adding any semantics to the attribute by just describing exactly what will happen.

warn_if_matched would work well if the attribute was on the constructor's argument, but it's technically much easier to have it on the constructor, in which case this wording can be confusing (it's ok to match on the constructor itself).

warn_if_matched would work well if the attribute was on the constructor's argument, but it's technically much easier to have it on the constructor, in which case this wording can be confusing (it's ok to match on the constructor itself).

Then warn_if_arg_matched ?

warn_if_arg_matched_against_literal?

warn_if_arg_matched_against_literal?

warn_if_literal_arg_match ? Whatever.

warn_on_literal_pattern?

warn_on_literal_pattern?

Yes.

warn_on_literal_pattern

I think this is the winner. I've applied the renaming and also introduced a new warning.

Committed to trunk, rev 16502.

Following #379, I have regrets about the current state of the this PR. Due to implementation limitations, we only support constructors with a single argument, but using the attribute on constructors with several arguments will just silently make it meaningless and not warn the user in any way. This is very bad usability.

At the very least it would be nice to have an "implementation limitation" warning where the attribute is misplaced. However, this is a bit problematic in terms of backward-compatibility: if we lift the single-argument limitation in the future, then people will be able to write (correct) that does not warn for their OCaml version, but warns on 4.03, and this is always a bit painful.

I think the real fix would be to have an implementation that is not half-baked: pose the attribute on constructor arguments, and warn if the corresponding argument is a constant literal. If we were still in the proposal discussion stage, I would now be of the opinion to not merge it until this more complete implementation is available. I'm not sure what's best at this point, but I'm considering proposing to postpone the feature until after the release.

Let's not spend too much effort on this. The proper solution is to use proper types for arguments instead of generic strings/ints to carry error payload (for instance an abstract type, with a "to_string" function). The problem is that it is too late to change the definition of existing exceptions in the stdlib, hence this proposal.

I would be perfectly ok with dropping support for adding the attribute on user-defined constructors, and keeping it only on predefined exceptions, if this can avoid over-engineering this stuff. I guess that most third-party libraries avoid the pitfall (or are easier to fix).