RFC: add command option to remove method redefinition warnings

[KristofferC]

With #265 fixed, more and more workflow seems to move towards redefining methods while keeping Julia running, even methods that exist in modules, e.g. https://github.com/timholy/Revise.jl. Also in interactive sessions like IJulia, redefining methods is very common.

Since this seems to be the way people will develop code, it is nice to be able to turn off the method redefinition warnings globally. This PR adds a command line option to turn them off.

[KristofferC]

Not sure this is the best design though. It would perhaps be better if the method redefinition warning could be opt out at the "eval site". This leaves it up to package developers instead of users to chose if the warning should be shown.

[KristofferC]

Regarding the thumbs down. Not all redefinition warnings are equal. Turning them off globally can hide real errors, but enabling them globally can be annoying and user unfriendly. For a package like Revise, being able to disable method redefintion warnings when it is executing something which sole purpose is to redefine methods makes sense to me.

[tkelman]

Overwriting a method with a modified version of the same thing during development, when that change was made intentionally and willingly by the same person who wrote the overwritten version, is one thing. But I don't think there's any way of distinguishing that from one package deciding it doesn't like how a different package works and maliciously changing its behavior, which has global side effects and is generally acknowledged to be a poor practice. Aside from the interactive development replacement use case, needing to overwrite a method is usually a sign of a missing API that should be done in a different, properly extensible way. I don't think we should add a feature that encourages people to do it more often. (Global command line flag that isn't on by default isn't the end of the world though, we let people ignore depwarns if they really want to and opt into that.)

[KristofferC]

one package deciding it doesn't like how a different package works and maliciously changing its behavior,

I don't understand this argument, if you think the package is "malicious" you are already hosed since you are running the code of that package? You can already redirect STDERR to dev/null so that would just a more fine-grained version of that, so you don't accidentally redirect too much.

Aside from the interactive development replacement use case

Yeah, aside from the use case where method redefinitions are good, method redefinitions are bad.

[tkelman]

You can already redirect STDERR to dev/null

Yes, and library code doing this is a terrible idea as it silently hides all errors. Library code overwriting methods is bad practice as well.

[KristofferC]

Yes, and library code doing this is a terrible idea as it silently hides all errors

Exactly, which is why more fine grained control is needed?

Library code overwriting methods is bad practice as well.

So when IJulia / Juno allows you to re-execute a cell / codeblock with a function definition, that is bad practice? When you can develop a package interactively with Revise, that is bad practice?
Whether it is bad practice or not depends on the context which is why I think a context based option is better than this PR which is global.

If method redefinition is not supported, then #265 could have been closed long ago. If it is supported, there should be a way to use it without warnings. Julia warnings shouldn't be used to indicate "bad API design".

[tkelman]

re-executing a notebook cell or interactively changing something is not what "library code" means. "library code" is a fixed version of a package that users or other packages need to rely on to behave consistently - aside from some eval corner cases, 265 didn't change the behavior of most libraries, it changed interactive REPL workflows. those workflows are where this might be useful, but a command line argument is sufficient for that.

I think it's totally reasonable, and beneficial, to warn if you do a Pkg.update and all of a sudden someone has added a dependency that overwrites an important method in some other package. I don't think we should add a feature that lets packages easily hide that kind of overwriting from their users. You're right that redirecting stderr already exists, but we shouldn't be encouraging it.

[KristofferC]

I think it's totally reasonable, and beneficial, to warn if you do a Pkg.update and all of a sudden someone has added a dependency that overwrites an important method in some other package.

Your scenario is only relevant if you assume that one of the updated packages has gone "rogue" and is trying to deliberately hide stuff from you. And at that point, being able to opt in to remove redef method warnings does not give the rogue package any extra abilities. You are already running its code, any sort of "security" thinking is pointless.
If the package has not opted in to remove redefinition warnings, you will still get them. If the package does opt in, it probably has a good reason and instead of redirecting STDERR, it can now redirect exactly what it wants instead.

[tkelman]

it probably has a good reason

Your "good reason" that seemed harmless to you is going to be "rogue" and harmful for someone else. The command line flag should also have an always option that ignores any suppressions. I'd argue "always" should be default for non interactive use, "sometimes" could respect local annotations, "never" would globally disable them.

[KristofferC]

Your "good reason" that seemed harmless to you is going to be "rogue" and harmful for someone else.

You are running my code so you have already decided that my code is harmless.
I agree with having an always, although I think the sometimes should be default.

[vtjnash]

I haven't read any of the above discussion, but let's just delete that warning from normal usage altogether. It was conceived of in a time where it was assumed to be rare. It's still probably worthwhile to warn (or error?) if this happens during "emitting output" (aka incremental precompilation), and just never warn otherwise.

[martinholters]

Warn only if the redefined method was originally defined in another module?

[KristofferC]

I'm up for that.

[tkelman]

Or re-defining a method within a non-Main module when not running interactively? When this warning was first enabled more broadly it caught a few packages that had exact duplicate definitions of some methods, which was clearly a bug that no one noticed otherwise.

[KristofferC]

Superseeded by #23030

[KristofferC]

Updated, I am semi AWOL so if anyone wants to take over this, it's good by me..

[tkelman]

as mentioned in #23030, Pkg.test and the precompilation process should default this to on. possibly when running the base tests as well if we think doing it accidentally there would be worth avoiding.

[JeffBezanson]

I can fix this up.

[JeffBezanson]

Since it seems likely there will only be more warning options over time, I think we should start using the naming convention --warn-x={yes|no}.

[JeffBezanson]

Done, and turned on for Pkg.test, precompile, and sysimg build. I've left if off for Base tests, since (1) the warning is tested explicitly in a couple places, and (2) this has the nice side effect of making the test output a bit quieter.

[tkelman]

command line flags should also be mentioned in news, the man page, and docs

[fredrikekre]

Since it seems likely there will only be more warning options over time, I think we should start using the naming convention --warn-x={yes|no}.

Should we then change --depwarn={yes|no|error} to --warn-deprecations={yes|no|error}?

[StefanKarpinski]

Wouldn't --deprecation={warn|ignore|error} be clearer?