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

Fix PR6638: "unused open" warning was incorrectly suppressed by "open!" #1110

Merged
merged 8 commits into from Nov 9, 2018

Conversation

Projects
None yet
@alainfrisch
Copy link
Contributor

commented Mar 16, 2017

Cf https://caml.inria.fr/mantis/view.php?id=6638

The open! variant of the open statement is intended to silence the "shadowing" warning.

It also silenced the "unused open" warning, which was unintended. This PR fixes this behavior, and adds tests for the two kinds of open-related warnings.

Note: it would be easy to report cases where open! is used but there is no shadowing. Is it worth adding a new warning for that?

@lpw25

This comment has been minimized.

Copy link
Contributor

commented Mar 16, 2017

I basically agree that silencing the "unused open" warning is not really the job of "open!", but I'm a bit concerned about the affect this will have on Jane Street's code. Most of our code starts with something like:

open! Core
open! Async

regardless of whether the code actually uses things from those libraries. Without the current behaviour there doesn't seem to be a succinct way of saying that we don't mind if these opens are unused.

@alainfrisch

This comment has been minimized.

Copy link
Contributor Author

commented Mar 16, 2017

Wouldn't it make sense to use the -open CLI option for such cases?

And if a module doesn't actually use things from the libraries, it would seem appropriate to remove the unused open statements, if only to reduce the dependency graph and potentially improve incremental build times.

You can also of course locally disable the warning 33 explicitly around those declarations (assuming the lines are some kind of standard boilerplate). Or perhaps more compactly add include (Core: sig end).

@yminsky

This comment has been minimized.

Copy link

commented Mar 16, 2017

We have patterns where we do opens in a per-module basis to set up the standard environment, and we want to do this even if the modules don't happen to be used. For example, this is a pretty standard beginning of an ML file:

open! Core
open! Async
open! Import

where Import sets up the rest of the environment. The opening stanza is a guarantee that you have the right starting environment, and it's better to have that guarantee in the source rather than buried in the build system. It also may be different between different files in the same project.

The fact that open! is concise is a big win, because we use this thousands of times in our codebase.

If we're going to fix this in any way, I would make a different syntax for disabling the shadowing warning, which for us at least comes up very rarely, where disabling the unused open warning is entirely routine.

@bluddy

This comment has been minimized.

Copy link

commented Mar 16, 2017

Seems to me like unused open should be an opt-in rather than an opt-out feature. Unused open isn't similar to unused variable, which sometimes indicates a bug in the code. Unused opens, on the other hand, could exist for a multitude of reasons, few(none?) of which have to do with lurking bugs. It almost fits better in a linting tool.

@lpw25

This comment has been minimized.

Copy link
Contributor

commented Mar 16, 2017

A possible solution would be to add a new warning for unused open! separate from the unused open warning.

@alainfrisch

This comment has been minimized.

Copy link
Contributor Author

commented Mar 16, 2017

The fact that open! is concise is a big win, because we use this thousands of times in our codebase.

How much do you rely on the unused open warning at all? Would disabling it globally be option on your side? As @bluddy wrote, the warning is not so important to spot actual bugs, but rather defects of different kinds (including useless dependencies), and this information might not be so important to you (considering that you are ok to live with useless opens on Core/Async/Import anyway).

@lpw25

This comment has been minimized.

Copy link
Contributor

commented Mar 16, 2017

I think it is quite a useful warning and would be sad to have to turn it off. The open!s at the top of the files tend to be of a quite different nature to the other opens in the files (especially local opens), so whilst the warning is not useful for our open!s it is still useful in general.

@alainfrisch

This comment has been minimized.

Copy link
Contributor Author

commented Mar 16, 2017

A possible solution would be to add a new warning for unused open! separate from the unused open warning.

That's of course trivial to implement, but it seems a bit weird to me to somehow correlate the two kinds of warnings. I guess you need to use open! also for other (local?) opens to explicitly allow shadowing, but in that case you would loose the "unused open" warning (in case the open becomes useless after some refactoring, for instance). Also, you don't really want the "allow shadowing" aspect on open! Core (right?). So it seems you would end up using the same syntax open! for two unrelated purposes.

Wouldn't (another) short syntax to silence the "unused open" warning work better in your case?

@lpw25

This comment has been minimized.

Copy link
Contributor

commented Mar 16, 2017

Wouldn't (another) short syntax to silence the "unused open" warning work better in your case?

Yeah that would be ideal. Although I fear "Wadler's law" will drag the discussion into a bottomless pit.

@yminsky

This comment has been minimized.

Copy link

commented Mar 16, 2017

@alainfrisch

This comment has been minimized.

Copy link
Contributor Author

commented Mar 17, 2017

Would open[@if_needed] List be short enough?

@yminsky

This comment has been minimized.

Copy link

commented Mar 17, 2017

@alainfrisch

This comment has been minimized.

Copy link
Contributor Author

commented Mar 18, 2017

I don't think it is fair to draw a parallel between the two warnings. There are definitely cases where shadowing is ok, we just want it to be intentional; hence the warning and the very compact way to silence it. I still don't understand why you want to be explicit about opening very generic modules (not relying on the CLI) and still accept some unused such opens, while insisting on being notified about other unused opens.

So I still believe the bug needs to be fixed. All the following solutions seem reasonnable for your situation, in my opinion:

  • Disable the "unused open" warning. Anyway, you already disable it implicitly today for all shadowing opens; and you are ok with some unused opens.

  • Remove unused opens, including on Core, etc. This can even be automated quite easily if needed.

  • Use a ppx that rewrite "open!" into something that disables the warning locally for that declaration. (Either with the existing @@@ocaml.warning attribute, or by using a new ad hoc attribute.) Alternatively, you could do that for all opens on specific module names which you know should not be subject to the unused warning stuff.

  • If your idea is that some specific modules should be allowed to be uselessly opened without triggering the warning, the right approach might be to mark such modules explicitly (e.g. with a toplevel floating attribute in the interface, similar to the "deprecated" annotation on modules). Are there cases where you want to open Core but be notified when this open becomes useless?

@yminsky

This comment has been minimized.

Copy link

commented Mar 18, 2017

I still don't understand why you want to be explicit about opening very generic modules (not relying on the CLI) and still accept some unused such opens, while insisting on being notified about other unused opens.

We want the namespace that a piece of code is operating to be explicit in the source. Doing it in the build system is distinctly second class from the point of view of making the meaning of a given bit of code clear to the reader. Note that the choice of where to open Async materially changes what's going on, shadowing a large collection of blocking operations, and replacing them with operations that represent their blocking in the Deferred monad. This context is important, and you may want to declare it even if you don't explicitly use any of the operations in that module.

Amusingly, part of what you're buying in these cases is shadowing! Another example: we might open Int.Replace_polymorphic_compare in a codebase where we want to hide the polymorphic comparison operators. You don't want to remove that open just because there aren't any comparisons in the source yet. If you do, then you open up the possibility of polymorphic comparisons being added later, by mistake.

Part of what's going on is that there are different visions of what good idiomatic OCaml programming looks like. I think of shadowing as fairly benign, and you think of it as highly problematic. (It would be interesting, in a different venue, to learn more about why. I haven't seen many problems due to unintended shadowing in our codebase.) I think of explicit management of scopes as important to the comprehension of code. This leads to two different views as to what the defaults should be.

It seems like we have three choices:

  1. Pick one of the two as the more important warning to skip, and make the other one secondary. I'd obviously argue for the unused-module warning, and you'd argue for the shadowing warning. I doubt either one of us will convince the other on this point. Maybe we should look for other voices?
  2. Find a concise syntax for both. I've not heard any proposals on this, but I'd be interested.
  3. Keep the current behavior as the default (disable both warnings), and have more verbose warnings for the individual cases. This would suit us just fine because we only care about one of the warnings, but it's obviously not ideal.

(1) is going to disenfranchise either your desired use cases or ours, and you're not excited about (3). Is there any hope for (2)?

As for your proposed solutions:

  • disabling the unused open warning is a disaster. We rely on it heavily in the cases where we don't use the exception. It's just like exhaustive record matches: the desire to have a local exception in no way suggests that you don't want the check in general.
  • Simply removing all unused opens is counter-productive for the reasons I explained above: it simply reduces the readability of the files in question to not be explicit about the environment they're intended to be written in.
  • Using a PPX rewriter to create the semantics we desire is certainly available. It's worth noting that it's equally available to support your use case. In either case, I'm not excited about forking the language in this way, making the in-our-walls semantics diverge from that outside-our-walls.
  • Similarly, doing this at the module level is not acceptable either. It's simply not the case that all uses of a given module are supposed to be immune to the check. Also, we want people to know when reading the code whether the check applies to a given open. Having it be a non-local property makes the system less predictable.

With all that, Alain, do you feel like you understand our use-case better? It seems to me like you couldn't up until now see any utility in purposefully keeping unused opens in a source file. Do you better understand now why we want this?

@yminsky

This comment has been minimized.

Copy link

commented Mar 18, 2017

Just to get the ball rolling, here are some ideas for having a short syntax for disabling both warnings. I'm really not sure if any of these are reasonable.

  • We could use open$ for allowing shadowing ($ looking something like an s)
  • We could use ! as an indicator that we're overriding all warnings, and then allow a syntax for specializing this. i.e., open! drops all warnings, open!s drops shadowing, open!u drops unused opens.
  • Maybe use underscore somehow? That's used elsewhere for turning off unused opens. We can't grab open_ or _open, because they're used in too much real code. I guess one could do something like open!_, though it's starting to seem like line noise.
@sweeks

This comment has been minimized.

Copy link

commented Mar 19, 2017

As possible syntax, how about:

open! == disable warning 44
open!! == disable warnings 33, 44

@bluddy

This comment has been minimized.

Copy link

commented Mar 19, 2017

Since open!'s entire purpose is to demonstrate conscious behavior ie. to suppress warnings because the programmer knows what he's doing, I think it makes sense to suppress the unused open warning as well.

However, I think that given the limited bandwidth out of the compiler, we want to separate between things that can actually cause bugs and things that are just... useful. For example, it'd be nice to have a warning when a small function cannot be inlined by Flambda, but I wouldn't want to pollute the compilation stream with it unless it's requested by the user. Unused opens seem to lie in the category of things that are nice but far from necessary, and therefore I stand by my notion that this warning should be opt-in.

Shadowing, on the other hand, can easily cause unexpected behavior, especially in the case where you add a function to a module that is opened elsewhere, causing 'spooky bugs at a distance'. This places shadowing in a different category of warnings altogether, and justifies having it be opt-out, and even dedicating syntax for overriding it, as yucky as that is IMO.

@alainfrisch

This comment has been minimized.

Copy link
Contributor Author

commented Mar 20, 2017

(It would be interesting, in a different venue, to learn more about why. I haven't seen many problems due to unintended shadowing in our codebase.)

open generally makes it more difficult to read code because the origin of used identifiers is not explicit. When shadowing is used, this becomes even more difficult, since you don't only need to find an opened module where the identifier is declared, you also need to ensure that the same identifier is not shadowed by another open. Similarly, the risk of shadowing makes it almost impossible to ensure backward compatibility of interfaces: adding a new exported identifier can break client code (or silently change its semantics).

(It would be interesting, in a different venue, to learn more about why. I haven't seen many problems due to unintended shadowing in our codebase.)

Yes, thanks for the explanation. I'm not sure I fully understand everything, but this is a good basis for discussion.

Note that the choice of where to open Async materially changes what's going on, shadowing a large collection of blocking operations, and replacing them with operations that represent their blocking in the Deferred monad. This context is important, and you may want to declare it even if you don't explicitly use any of the operations in that module.

I would find it more robust to group all blocking operations in a dedicated module. So, instead of relying on shadowing, you would set up the "context" either with: open Core;; open Blocking or open Core;; open Async. This way would even make it explicit when a given piece of code is "agnostic" to the choice between Async vs blocking operations.

I can see that a useless open Async is a way to prevent future evolutions of the code to introduce calls to blocking operations. But this is a bit fragile: nothing prevents someone from deleting this useless line (after realizing it is useless), or moving it above open Core. Being explicit about when blocking operations are used (by moving them to a dedicated module) makes it a deliberate choice to use such operations.

Another example: we might open Int.Replace_polymorphic_compare in a codebase where we want to hide the polymorphic comparison operators.

Some thing here. The way I see it: if you consider polymorphic compare to be dangerous as a general tool, you should arrange to make it explicit when it is to be used (e.g. by open Core.Polymorphic_compare) instead of relying on shadowing to opt-out from it. (Since you control your own standard library, I assume that Pervasives is not opened on your code base. Is that right?)

I'd obviously argue for the unused-module warning, and you'd argue for the shadowing warning. I doubt either one of us will convince the other on this point.

I have the documentation on my side 😄 :

Since OCaml 4.01, open statements shadowing an existing identifier (which is later used) trigger the warning 44. Adding a ! character after the open keyword indicates that such a shadowing is intentional and should not trigger the warning.

It's not like there was any ambiguity about the role of open!; the current behavior is clearly a bug. It is unfortunate that you now rely intensively on something which was obviously an unintended behavior (considering the documentation). I'm open to discussing ways to accommodate your approach, but please don't make it sound like there is a symmetry here.

Similarly, doing this at the module level is not acceptable either. It's simply not the case that all uses of a given module are supposed to be immune to the check. Also, we want people to know when reading the code whether the check applies to a given open. Having it be a non-local property makes the system less predictable.

I don't understand this point. Concretely, are there cases where you open Async without the bang, currently? Is not always intended to be opened "after Core" and shadow some of its identifiers? Similarly, considering the intended use of Int.Replace_polymorphic_compare, does't it always make sense to assume you want to open it while disabling both warnings?

In either case, I'm not excited about forking the language in this way, making the in-our-walls semantics diverge from that outside-our-walls.

It would be interesting to hear from non-JS people to see if the style you use to set up the context (relying on shadowing explicitly) is something which is widespread or even considered a good idea, outside your walls.

If we are looking for a short syntax, I would propose: open? to silence the unused open warning and open!? or open?! to silence both warnings. This ? could be supported purely at the parser level by inserting an appropriate longer attribute form.

@yminsky

This comment has been minimized.

Copy link

commented Mar 20, 2017

Long story short: open? sounds fine to me. Resolving this bug by adding first-class support for ? as a way to support suppressing this error individually seems like a lovely solution.

More details:

Your points about how we avoid shadowing are sensible, but don't really help. For one thing, even if we had a nice way of breaking up the stdlib into units that could be opened separately without shadowing (and we don't yet), we still need a way that's natural for both internal and external users of libraries like Base, Core and Async.

Given that OCaml doesn't provide a good way of dealing with this presently out of the box, shadowing is kind of the only option. I do agree that it would be cleaner if it wasn't necessary.

I have the documentation on my side 😄 :

Since OCaml 4.01, open statements shadowing an existing identifier (which is later used) trigger the warning 44. Adding a ! character after the open keyword indicates that such a shadowing is intentional and should not trigger the warning.

Fair. I do have a big line codebase that uses this idiom extensively: a quick grep found 23k uses of open! for the purpose I described, if that's compelling...

It's not like there was any ambiguity about the role of open!; the current behavior is clearly a bug. It is unfortunate that you now rely intensively on something which was obviously an unintended behavior (considering the documentation). I'm open to discussing ways to accommodate your approach, but please don't make it sound like there is a symmetry here.

I agree the documentation does introduce an asymmetry. Other than that, the situation seems to me quite summetric: two warnings were added (in the same release). I think they both deserve a concise syntax for disabling them. That symmetry I stand by.

Similarly, doing this at the module level is not acceptable either. It's simply not the case that all uses of a given module are supposed to be immune to the check. Also, we want people to know when reading the code whether the check applies to a given open. Having it be a non-local property makes the system less predictable.

I don't understand this point. Concretely, are there cases where you open Async without the bang, currently? Is not always intended to be opened "after Core" and shadow some of its identifiers? Similarly, considering the intended use of Int.Replace_polymorphic_compare, does't it always make sense to assume you want to open it while disabling both warnings?

I guess the primary thing to understand is that this isn't just about Core and Async. Lots of libraries define their own little environments which are intended to be used broadly in certain contexts. That means that authors have a bunch of different choices to make about where they do and don't want to open modules that might not be used yet. In order for a reader to understand these choices, whether the test in question is being run should be visibly apparent to the reader. If I see open Foo, I know the open is in fact used in this module. If I see open! Foo, then I know it might not be.

In general, we strongly prefer the behavior of the compiler and checks to be uniform, which is to say deviations from ordinary behavior should be visible in the source.

It would be interesting to hear from non-JS people to see if the style you use to set up the context (relying on shadowing explicitly) is something which is widespread or even considered a good idea, outside your walls.

It's a good question, but it's worth noting there's basically no other option for people who use Core. The way in which they get their own new modules in place is by shadowing existing ones. I just picked a random external project that uses Core_kernel, and you see open Core_kernel.Std at the top, which of course works by shadowing.

https://github.com/frenetic-lang/ocaml-topology/blob/master/lib/Network.ml

I'm not saying I think shadowing is ideal, but the tooling doen't really provide another option.

If we are looking for a short syntax, I would propose: open? to silence the unused open warning and open!? or open?! to silence both warnings. This ? could be supported purely at the parser level by inserting an appropriate longer attribute form.

open? seems fine to me. Even with our 23k uses, it's not hard for us to mechanically smash them from ! to ?.

@alainfrisch

This comment has been minimized.

Copy link
Contributor Author

commented Mar 20, 2017

I agree the documentation does introduce an asymmetry. Other than that, the situation seems to me quite summetric: two warnings were added (in the same release).

A difference is that there is no easy way to "silence" the warning about shadowing without open! (or locally disabling the warning). There is an easy way to get rid of an "unused open": remove it. I now understand that you make a distinction between "unused open" and "useless open", but still, I believe most people (outside Core users) would be happy without a way to disable "unused open" warnings locally, while having no way to "accept" shadowing would force them to disable the warning altogether.

For one thing, even if we had a nice way of breaking up the stdlib into units that could be opened separately without shadowing (and we don't yet)

I'd be interested to understand this point better. Isn't it possible to move all blocking operations (which are being shadowed by Async) into a different module? Same for polymorphic comparison.

I'd also be interested to know if you have occurrences of open Async or open Int.Replace_polymorphic_compare without the bang in your codebase.

@bluddy

This comment has been minimized.

Copy link

commented Mar 20, 2017

For what it's worth, I think adding syntax like open? is just adding more confusion to the language. I don't think we should be oriented towards adding more syntax for warnings, as at some point we'll come up with another module-related warning and need a syntax for overriding that, too. If it's about disabling warnings, then we should have annotations that deal with that at a generic level. Instead, I would propose to look at open! as a 'deliberate' open, an open showing full intent and having the programmer take full responsibility. As such, it completely fits in with the current behavior of the compiler, including disabling the 'unused open' warning.

@jberdine

This comment has been minimized.

Copy link

commented Mar 20, 2017

FWIW, we have code at facebook which uses shadowing to protect against using various parts of the standard library and Core that are dangerous in our context. We definitely rely on open! suppressing the unused open warning. I personally also agree with @bluddy that adding extra syntax is suboptimal and not a maintainable approach over the long-term.

@alainfrisch

This comment has been minimized.

Copy link
Contributor Author

commented Mar 20, 2017

@jberdine Are the same modules sometimes used with open and sometimes with open!. I can imagine that modules whose purpose is specifically to shadow components of other modules would always be opened with open! with the intention of disabling both warnings. Is that right?

@bobzhang

This comment has been minimized.

Copy link
Member

commented Mar 20, 2017

I think introduce open! already complicates the language unnecessarily, having open? will be more confusing

@jberdine

This comment has been minimized.

Copy link

commented Mar 20, 2017

@alainfrisch Yes, in our case, any given module is either always opened with open, or with open! to disable both warnings.

@alainfrisch

This comment has been minimized.

Copy link
Contributor Author

commented Mar 20, 2017

@jberdine So do you think my proposal about marking modules would work on your side? Or do you insist on making the choice explicit on the open site?

@bobzhang What is your position, then? That all open-related warnings should be removed?

@bobzhang

This comment has been minimized.

Copy link
Member

commented Mar 20, 2017

I think we can have an attribute to suppress such warning, it may not be that bad for core to have another ppx to rewrite open! to suppress such warnings.

@jberdine

This comment has been minimized.

Copy link

commented Mar 20, 2017

@alainfrisch Just to be clear, I commented not because I have a particularly strong position, but because there was a question about whether anyone outside Jane Street also used the shadowing behavior. It wouldn't be the end of the world for us if we had to use an attribute instead of/in addition to open!. I'd consider it definitely suboptimal if the "marking" of modules intended to cast shadows had to be hidden in a ppx or the build system, etc. (I solidly agree with @yminsky that it's much clearer when this information is in the code itself). I also think that while Core is a clear example usage of shadowing, it's definitely not alone. I mean, can anyone get away with having a code base worked on by a team, especially where some members are reasonably inexperienced, and not shadow e.g. @ and List.map?

@yminsky

This comment has been minimized.

Copy link

commented Mar 20, 2017

One thing this conversation highlights is that shadowing and wanting to ignore unused open checks do go together naturally. If you're opening a module in order to shadow some calls, that's precisely the kind of case where you want the module in place, whether or not it's being used. This makes the accidental behavior of open! disabling both warnings seem in retrospect fairly natural.

I do think my favorite solution is to leave open! as is, and add readable annotations for the two more narrow ways of disabling warnings. So, the three cases would be:

open! Core
open Core [@@allow_unused]
open Core [@@allow_shadowing]
@alainfrisch

This comment has been minimized.

Copy link
Contributor Author

commented Mar 21, 2017

I'd consider it definitely suboptimal if the "marking" of modules intended to cast shadows had to be hidden in a ppx or the build system, etc. (I solidly agree with @yminsky that it's much clearer when this information is in the code itself).

I was not referring to such solutions, but to the idea of marking module in the code (as an attribute in their interface, similar to the way modules can be marked as deprecated). There are clearly cases where module are intended to shadow/hide previous definitions, and it seems fair to assume that when opening such modules, both kinds of warnings should always be disabled.

@bobzhang

This comment has been minimized.

Copy link
Member

commented Mar 22, 2017

@yminsky imagine your 12k use-cases of let%bind turns into let!, async program will looks much more beautiful
@hcarty I think override this definition are just customizable warning configurations which really should not be taken into the core language

@hcarty

This comment has been minimized.

Copy link
Contributor

commented Mar 22, 2017

@bobzhang That brings up a potentially relevant point around consistency.

According to http://caml.inria.fr/pub/docs/manual-ocaml/extn.html#s%3Aexplicit-overriding val!, method! and inherit! all require that they shadow/override something. The current state of open! conflicts with those other ! uses since it silences the equivalent warning.

@bobzhang

This comment has been minimized.

Copy link
Member

commented Mar 22, 2017

Yeah, it looks weird, and val! predates attributes

@bluddy

This comment has been minimized.

Copy link

commented Mar 24, 2017

@yminsky imagine your 12k use-cases of let%bind turns into let!, async program will looks much more beautiful

Continuing on this tangent for a bit, would let.bind work? A record projection on a keyword cannot exist in the language, and . provides far less visual noise than % as a separator.

@alainfrisch

This comment has been minimized.

Copy link
Contributor Author

commented Mar 27, 2017

The current state of open! conflicts with those other ! uses since it silences the equivalent warning.

method! (and co) also silence an "override" warning (7). The difference is that for open!, it is not considered an error when the ! is actually useless. In the PR description, I hinted at the possibility of reporting this situation (as a warning); but this would be at odds with the use of open! reported at Jane Street.

@lpw25

This comment has been minimized.

Copy link
Contributor

commented Apr 3, 2017

Now that the inevitable syntax suggestions seem to have died down, can I reiterate my earlier suggestion:

A possible solution would be to add a new warning for unused open! separate from the unused open warning.

To me, this is the simplest solution. It has a few things in its favour:

  • The various "unused" warnings are already split by the form of the construct that introduces them (e.g. let-bound variables, argument variables, for indices, and open all have different warnings, rather than a single unused value warning). This is merely introducing another distinction.
  • The ! is only directly silencing the shadowing warning: consistent with the other uses of ! in the language.
  • It leaves what is really a style-issue rather than a soundness issue up to the user's control, which I in general prefer.

@damiendoligez damiendoligez added this to the 4.07-or-later milestone Sep 29, 2017

@damiendoligez damiendoligez removed this from the consider-for-4.07 milestone Jun 5, 2018

@alainfrisch alainfrisch force-pushed the alainfrisch:fix_6638 branch from f09f327 to e2388e6 Nov 8, 2018

@alainfrisch

This comment has been minimized.

Copy link
Contributor Author

commented Nov 8, 2018

@lpw25 I've implemented your proposal of creating a new warning to report unused open! statements.

@alainfrisch

This comment has been minimized.

Copy link
Contributor Author

commented Nov 8, 2018

@lpw25 Would you like to review this PR?

@lpw25

lpw25 approved these changes Nov 8, 2018

Copy link
Contributor

left a comment

One harmless typo, other than that this looks good to go.

@@ -14,7 +14,7 @@
(* *)
(**************************************************************************)

[@@@ocaml.warning "+a-4-9-30-40-41-42"]
[@@@ocaml.warning "+a-4-9-30-40-41-42-66-66"]

This comment has been minimized.

Copy link
@lpw25

lpw25 Nov 8, 2018

Contributor

Repetition

alainfrisch added some commits Nov 8, 2018

@alainfrisch

This comment has been minimized.

Copy link
Contributor Author

commented Nov 8, 2018

Thanks! Typo fixed and changelog entry adjusted and moved from "Bug fix" to "Compiler user-interface and warnings".

@alainfrisch

This comment has been minimized.

Copy link
Contributor Author

commented Nov 8, 2018

(Ok, I'll need to fix tests/warnings/w33.ml)

@alainfrisch alainfrisch merged commit 7baf33d into ocaml:trunk Nov 9, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@yminsky

This comment has been minimized.

Copy link

commented Feb 15, 2019

This seems like an unfortunate outcome. Adding the warning seems right, enabling it by default seems like a mistake, since it disables a useful and now common idiom by default, without a reasonable alternative syntax.

@alainfrisch

This comment has been minimized.

Copy link
Contributor Author

commented Feb 15, 2019

I don't understand : the new warning is disabled by default ( https://github.com/ocaml/ocaml/pull/1110/files#diff-b8211d09f1ad362c785bd8171576ce5fR306 ).

@yminsky

This comment has been minimized.

Copy link

commented Feb 15, 2019

My apologies! I heard a report that indicated it was enabled by default. I'll figure out what happened there.

@yminsky

This comment has been minimized.

Copy link

commented Feb 15, 2019

FWIW, I was confused by the fact that Dune in dev mode enables all warnings, except for a specific list of exceptions. I'm not sure that's the right default policy, but I think this one should just go in the exception list.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.