Skip to content

Make deprecation warning non-fatal#1850

Closed
rgrinberg wants to merge 1 commit intoocaml:masterfrom
rgrinberg:nonfatal-deprecation
Closed

Make deprecation warning non-fatal#1850
rgrinberg wants to merge 1 commit intoocaml:masterfrom
rgrinberg:nonfatal-deprecation

Conversation

@rgrinberg
Copy link
Copy Markdown
Member

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

@rgrinberg rgrinberg requested review from a user and emillon February 17, 2019 09:20
@rgrinberg rgrinberg force-pushed the nonfatal-deprecation branch from 0093572 to fb21727 Compare February 17, 2019 09:24
Based on user experience, this is the mode widely used default

Signed-off-by: Rudi Grinberg <rudi.grinberg@gmail.com>
@rgrinberg rgrinberg force-pushed the nonfatal-deprecation branch from fb21727 to 8d9e955 Compare February 17, 2019 09:24
@yminsky
Copy link
Copy Markdown
Contributor

yminsky commented Feb 17, 2019

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.

@avsm
Copy link
Copy Markdown
Member

avsm commented Feb 17, 2019

I agree with @yminsky here. It's not difficult to add a [@@@warning "-3"] for a particular deprecation warning that can't be immediately fixed, and a clear message from dune with a pointer to that might be more useful.

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

@rgrinberg
Copy link
Copy Markdown
Member Author

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.

@yminsky
Copy link
Copy Markdown
Contributor

yminsky commented Feb 18, 2019

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.

@NathanReb
Copy link
Copy Markdown
Collaborator

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.

@ghost
Copy link
Copy Markdown

ghost commented Feb 18, 2019

But for libraries where you're trying to support a wide range of OCaml versions and dependencies,

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 ~/.config/dune/config file.

@emillon
Copy link
Copy Markdown
Collaborator

emillon commented Feb 18, 2019

I believe that the problem here is that "development mode" means different things:

  • one the one hand, it's the only context problems can be fixed, as opposed to when a package in installed through opam. So we want to report problems.
  • on the other hand, when developing, it's common to go through intermediate "invalid" states where some warnings get in the way: for example, temporarily disabling a function call can cause a variable to become unused.

The profile setting has been designed the other way around: release was the default, and --dev was opt-in. When that was the case, the distinction between the two meanings of "development" was less important, as it was fairly easy to just run --dev from time to time to maintain quality.

@yminsky
Copy link
Copy Markdown
Contributor

yminsky commented Feb 19, 2019

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.

@rgrinberg
Copy link
Copy Markdown
Member Author

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:

  • I don't know how to best do it
  • I have more important things to work on

And yet now I can't proceed with my original goal until I fix this issue.

My workaround has been to use the DUNE_PROFILE var and set it to some dummy value for such builds. You can tell that this is not ideal.

@rgrinberg rgrinberg closed this Feb 19, 2019
@ghost
Copy link
Copy Markdown

ghost commented Feb 19, 2019

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 release when we want the loose behavior seems like the right method to me, and dune offers many ways to do it. We could make it even easier with some editor integration for instance: by pressing a key combo, you'd quickly alternate between the different modes.

@bluddy
Copy link
Copy Markdown

bluddy commented Aug 20, 2019

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.

@yminsky
Copy link
Copy Markdown
Contributor

yminsky commented Aug 20, 2019

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.

@ghost
Copy link
Copy Markdown

ghost commented Aug 20, 2019

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 env stanza. Personally, it seems better to me to fix such warnings as soon as possible. Otherwise, they will quickly accumulate and it will be painful for everybody working on the code.

@ghost
Copy link
Copy Markdown

ghost commented Aug 20, 2019

However, there is a possible compromise. Let's say we add a new field to the dune-project file that indicates which version of the compiler developers are supposed to use to work on the project. When using a compiler older or equal to this version, deprecation warnings are treated as error. When using a newer compiler, deprecation warnings are not treated as errors, even in development mode.

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.

@alainfrisch
Copy link
Copy Markdown

Some thoughts about this discussion (and sorry in advance if the points below don't apply to Dune, which I haven't used yet):

  • Non-fatal warnings are mostly useless, since they are lost in the stream of outputs from the build system, so they are usually ignored, they don't break CI build, and if you weren't lucky enough to see them when your files were first compiled, you won't see them later.

  • Fatal warnings are rather aggressive, since they are real errors and they prevent compiling further modules.

  • I believe that while it make sense to have the compiler deal with enabling/disabling warnings (locally, through attributes), Dune should make all warnings non-fatal (for the compiler) and handle warning outputs seriously itself: remember warnings from compiled modules in its cache, always inform users about pending warnings in the entire project (with a sub-command to show them), and perhaps turn some warnings into fatal ones at the project/final target level (i.e. prevent building libraries/programs if some selected warnings remains). Always showing some reminder status about pending warnings is annoying enough to avoid the risk of people not dealing with them, without ever breaking the build unless developers explicitly asked so.

  • In addition to that, there is a clear need to version deprecation warnings. Cf discussion on Generalize the "deprecated" alert ocaml#1804 . As mentioned in Generalize the "deprecated" alert ocaml#1804 (comment), one could already use alerts to specify (in their payload) the version of the stdlib (or another library) in which some component has been deprecated. What's missing is a way to let the user specify version constraints, but this could be handled by the build system. Concretely, assuming that some components in the stdlib is marked as:

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.)

@yminsky
Copy link
Copy Markdown
Contributor

yminsky commented Aug 20, 2019

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.

@ghost
Copy link
Copy Markdown

ghost commented Aug 20, 2019

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.

@hcarty
Copy link
Copy Markdown
Member

hcarty commented Aug 21, 2019

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants