Conversation
0093572 to
fb21727
Compare
Based on user experience, this is the mode widely used default Signed-off-by: Rudi Grinberg <rudi.grinberg@gmail.com>
fb21727 to
8d9e955
Compare
|
One thing I like about the current world is that it pretty strongly encourages people to keep up with deprecation warnings. I worry that just turning it off will greatly reduce the effectiveness of deprecation warnings. Just because people have the practice of ignoring warnings doesn't mean it's good practice1 For new code, treating the use of deprecated APIs as a straight-up error is likely the right response. The issue instead is when people are adopting new libraries, where you want them to be notified in a clear way about the new deprecations, but they don't necessarily want to get rid of them immediately. Maybe a better in-between would be for Dune to keep the current behavior, but to provide a really clear message that explains how they can temporarily and/or narrowly disable the error. |
|
I agree with @yminsky here. It's not difficult to add a I do think that the problem is worse with vendored code, but external code needs a different set of flags entirely and so could be addressed separately by #1016 |
|
I think we have a different perspective on how the defaults should be selected. To me, defaults are configurations that we should be tweaked as little as possible. Therefore, if we have a default setting that is very well intentioned but needs to be turned off more often than not, that seems to be the wrong default. So it is not a matter of how easy it is to disable this setting (making deprecations fatal would be just as easy as well of course). I often myself having to change this setting on a regular basis, and have user reports confirming this experience. It's not that I would like to ship code that uses deprecated API's, but in practice I find that the ground on which I'm standing on moves at a faster pace than I can keep up with. I think it's also a matter of whether one maintains libraries or applications. For applications, I certainly prefer the current default as well. But for libraries where you're trying to support a wide range of OCaml versions and dependencies, the current default gets in the way more often than not. |
|
Interesting. I guess what this highlights is that there are two modes here: one where you're developing code in a way that tries to eagerly drop deprecated APIs, one one where one purposely maintains them. I'm not sure what the right workflow is for purposely using deprecated APIs, but my instinct would be to want a marker in the code or in the build rules for where I'm using such APIs, so they're easy to keep track of and remove when the use of the deprecated APIs is no longer desired. But for the workflow where one is avoiding deprecated APIs, the current behavior seems clearly preferable. So, you've heard from one subset of users who have complained about the present behavior, but if it's changed, I suspect you'll end up hurting a different set of users. The fact that there are people unhappy with the current state doesn't seem like quite enough to just flip straight to the other behavior. |
|
As I already expressed in #1843 I think the main motivation here is that by making warning 3 fatal by default, deprecation becomes a breaking change to most library users, especially in cases where you don't have full control over the code using deprecated APIs. Just as you, I personally prefer to make it fatal and to use a stricter profile to improve the quality of the code I write. Maybe we need a new profile here. |
That's the root of the problem. Supporting a wide range of dependency versions is a massive pain, but we don't have much choice because the world doesn't move away very fast from older deprecated stuff. I also feel like having warning 3 as fatal by default in development mode is the right choice: the development mode is meant for humans. Developers generally want their code to be as good as possible, and code using deprecated features is not as good as it could be. The tooling is also getting better in general and hopefully one day we won't need to keep supporting a wide range of versions of the compiler and/or of dependencies. For the time being, if this is a real issue in practice, I suggest that we make this behavior configurable via the user configuration file. I.e. allow users to make warning 3 non-fatal by default in development mode by writing something in their |
|
I believe that the problem here is that "development mode" means different things:
The profile setting has been designed the other way around: release was the default, and |
|
FWIW, my personal approach, and the default approach at Jane Street, is to have warnings-as-errors on all the time. You typically transition through "invalid" states very briefly, and when you need to allow turning off a warning in a file, we tend to do it explicitly, through an annotation in a file. In this case in particular, the ability to transition through the invalid state of using deprecated APIs is typically not useful when developing new code. I can well imagine it would be useful in the case that @rgrinberg described, where you're using a new version of a library in a way that continues to work with the old version of that library. But I'd prefer to do that with explicit, source-level markers, rather than turning off the error in its entirety. I do think this highlights the challenge of building an opinionated dev tool -- different people do things different ways, but the defaults have to be chosen just one way. |
|
I'll close this for now as it's too controversial. But it still remains an annoying issue in practice. Here's a recent example: I tried getting dune to work on 4.08 but was met by a fresh batch of deprecation errors. I'm not in a position to fix those errors as:
And yet now I can't proceed with my original goal until I fix this issue. My workaround has been to use the |
|
Just a comment on the rationale behind the current design: the release mode is for machine builds, while the development mode is for humans. Machines are not yet clever enough to process and fix warnings emitted by the compiler, so to increase the lifetime of released code, machine builds have to be as loose as possible. Jbuilder was originally designed for machine builds only. It didn't even support incremental compilation for instance. Once humans started to use jbuilder to develop, we added the development mode. When jbuilder became widely used as a development tools for humans, we swapped the default mode. Setting the profile to |
|
Coming back to this, the decision to make the default warnings-as-errors has now reverberated to the rest of the ecosystem, and deprecation warnings are now seen as breaking builds, causing people in the ecosystem to want to avoid any further deprecations whatsoever. It's also curious to me that a tool that bills itself as the default build system for OCaml chooses to override the compiler's default behavior, effectively changing/bypassing it for the entire ecosystem. I would urge reconsideration of this PR. |
|
I thought there was a pretty good solution for this in flight. First, not treating warnings as errors in vendored libraries. Second, not treating all errors as the baseline, but having dinner have a specified set of warnings for each version. Then, bumping the version of the dune format being used is what gives you the new warnings. |
|
To be clear, opam builds are not affected since in release mode warnings are not treated as errors. It is indeed the same for vendored libraries. The only catch is that the set of deprecated items in a library is not versioned. Either we get all deprecation warnings either we get none. So effectively, in development mode as soon as one switches to 4.08 for their local development they have to immediately fix the deprecation warnings for the newly deprecated items. One can easily change this behaviour for a given project via the |
|
However, there is a possible compromise. Let's say we add a new field to the This way, someone who would just want to play with a new compiler before the team has decided to switch to the new compiler doesn't have to fix all the wanings upfront. Ideally, such a mechanism would be extended to all dependencies, however it is not possible to filter warnings per dependency. |
|
Some thoughts about this discussion (and sorry in advance if the points below don't apply to Dune, which I haven't used yet):
val foo: ... [@@alert deprecated_since "4.08"]One could tell Dune that a given project targets stdlib 4.07, in which case the deprecation alert would be ignored when that component is referenced. (One could also add "since" alerts to specify in which version the component has been introduced, and tell Dune that we don't want to use any component introduced after some target version.) |
|
I like this idea a lot. It would be lovely to be able to see all the errors from a build, and this is a case where you could easily do just that. Preventing the construction of final targets seems prudent, as a way of preventing people from thinking that things are OK when they really aren't. |
|
I like this idea as well. It'd be nice if the compiler could report the warnings/errors in a more structured way so that Dune could easily separate the errors from the warnings, but at worse parsing the compiler output shouldn't be too hard. Regarding the versioning of warnings, the compiler would also need to know the version of the libraries being used in order to decide what alerts to report for each library dependency. |
|
It's not something that exists in the compiler now, so it wouldn't be a near-term thing, but there may be some support for the idea of structured error/warning reporting according to ocaml/ocaml#7123 |
Based on user experience, this is the more widely used default. This is not a breaking change, so we might as well just do it ASAP.
One thing, I'm wondering is if we should make this dafault project-version dependent. That is, set this to non fatal for >= 1.8 only. I'm not yet sure how hard that would be.
cc @NathanReb and @avsm