Shadow warnings: 44 refined into 52,53 for alphanumeric identifiers and symbolic operators

[gasche]

For more details on warnings when open-shadowed identifiers are used, see the discussion and pointers in PR#6951.

In this proposed PR, warning 44 is preserved exactly as is (the goal is to help forward- and backward-compatibility), and two new warnings are added that refine its behavior: 52 fires on alphanumeric shadowing when 44 is disabled, and 53 fires on symbolic shadowing when 44 is disabled.

A consequence of this compatibility strategy (up for discussion) is that people using +A-44 will see new warnings appearing (one must now use +A-44-52-53 for the same effect).

I think we could consider enabling +52 (shadowing of alphanumeric identifier) by default in 4.03, but this patch does not implement it.

Note that with the proposed implementation, (asr), (mod) etc. are considered symbolic operators.

[Drup]

Same as before, not completely convinced by the distinction operator/identifier but it's at least better than before.

Do you think we should have M.!(...) too ? I feel like it still serves a purpose, even with the new warning. Most people that cares will probably do -44-53, but might occasionally want to shadow an identifier.

Also, -44 is basically useless in your scheme, why not just say "-44 is equivalent to -53-52" ?

[gasche]

I have mixed feelings about M.!(...), so right now I'm trying to focus my (limited) efforts on finer-grained shadowing criterions to make any use of open! less necessary.

The warnings work in a very simple way today, in particular enabling/disabling a warning never acts over other warning settings. I thought about schemes were (un)selecting 44 would implicitly impact 52/53, but ultimately I think that the simplicity of the current system is more important. It is also rather difficult to reason about backward- and forward- compatibility, and I think it is best to make sure that 44 behaves exactly as before on non-erroneous¹ programs.

¹: The first version of my PR guaranteed that 44 would work exactly as before on all programs, in particular a program with A@52 would warn on 44 and then error on 52. The current version does not warn on 44 in this case (it feels a bit superfluous), so the compatibility guarantee is that 44 behaves as before on non-erroneous programs (only).

Note that 44 is not enabled by default today, so the only people using -44 are those using A, and those should be used to micro-manage their warnings. I expect more people to add warnings to the default set (so they now have the choice to replace +44 into +52 if they wish -- and we could discuss adding +52 by default), or explicitly list all warnings they want to enable (the most compatible solution).

[Drup]

I have mixed feelings about M.!(...), so right now I'm trying to focus my (limited) efforts on finer-grained shadowing criterions to make any use of open! less necessary.

"Less" doesn't mean "not", and having only open! is inconsistent.

[dbuenzli]

I think we could consider enabling +52 (shadowing of alphanumeric identifier) by default in 4.03,

I have little interest in tweaking warning flags in each of my projects. Provide the good defaults as this seems to be the right one.

[gasche]

@dbuenzli ok, but adding a new warning by default need to be discussed (compatibility concerns etc.), and I would rather dissociate this discussion from the present PR itself. Also, you are an opinionated person, so I don't think it is realistic to hope that you will always be satisfied with the set of warnings by default. (how would a person of taste accept not to enable 40 as an error?)

It is bad to change the semantics of a warning after it has been released, which means that the semantics of 52/53 will be set in stone after the 4.03 release (no idea when that will be). I might try to refine their behavior a bit before that -- for example maybe try to implement the idea of not warning if the shadowing and shadowed bindings are alias of each other. I have thought a bit about it, but it is not as simple to implement.

[gasche]

We discussed this at the developer meeting yesterday, and the consensus was to not include the change yet and postpone a decision. Some were not convinced that the identifier/symbol split is the right way to split or refine this warning.

(I'm not sure what to do with the PR. If there was a "postponed" label I would use it.)

[agarwal]

If there was a "postponed" label I would use it.

Why not create it.

[gasche]

I'm probably not in the right group (or I don't know how to do that). In the label list I can write arbitrary names in the "filter" input form, but not create a label from it. @damiendoligez , can you create a postponed/delayed/suspended label?

[agarwal]

See here. You have write access to the repo, so I'm pretty sure you should be allowed to do this.

[gasche]

Thanks @agarwal ! This is the worst user interface I've seen in a long time (it's FusionForge-level obscurity), there are 8 steps described to add a label (for something which should be one click away from the "label" button visible in the right), and some of them are wrong or confusing (clicking the "ocaml" repository name in my Repositories list in profile goes to my personal clone, which is not what we want here; step 4 asks to click "Labels" on a page where two different buttons are named "Labels" and the more visible one is the wrong one), but I managed to create a "suspended" label. Yay.

[agarwal]

I agree this is not too intuitive, but comparing to FusionForge is a low blow. Perhaps you'll like their new interface. It's opt-in for less than 2 weeks, and then they'll switch you over.

[damiendoligez]

but adding a new warning by default need to be discussed (compatibility concerns etc.),

There are no compatibility concerns. We clearly have the right to break programs that use -warn-error A.

[xavierleroy]

The latest comment on this PR goes back to January 2016, so I feel somewhat safe in closing this PR. Reopen if you still care about it.