RFC: always warn on overwritten methods

[JeffBezanson]

Currently we only warn if a method is overwritten in a different module. I now believe we should always print this warning.

First, I was amazed to discover how often this happens in Base. Nearly all cases seem to be mistakes. Bizarrely, several definitions were literally copy & pasted one after the other. It's possible those were rebase artifacts of some kind. This also revealed that when doc strings are applied to @enums, the entire definition (type and all methods) is repeated twice. I have not fixed this yet.

Second, jb/functions makes overwriting methods worse, since code can be specialized on function arguments. A few packages are failing because their tests repeatedly write f(x) = ..., intending to create a different function each time. These should be written as f = x-> ... instead. This warning will help find such cases. (This is an especially bad manifestation of #265.)

The first commit removes many overwritten definitions from Base. The second commit changes the warning to print in all cases.

[yuyichao]

👍 Maybe we could even make it an error? There's at least one test in core.jl that deliberately override method definition to check for segfault. Maybe we can just remove that...

[johnmyleswhite]

+100

[mauro3]

+1 if it can be disable in the REPL as that would add a lot of noise.

[toivoh]

• a lot

[kmsquire]

+1 if it can be disable in the REPL as that would add a lot of noise.

+1 to the idea, but also to this statement. I find myself redefining functions at the REPL frequently during development, and this would get annoying.

(Also would be nice to disable warnings on calls to workspace().)

[StefanKarpinski]

Wouldn't making overwriting a method always warning be sufficient? That would allow redefining things / reloading code, but would make it clear when library code was doing it and shouldn't.

[vtjnash]

+1

[tkelman]

where's the duplicate here?

[JeffBezanson]

It's in libgit2.jl, where there is a 2-argument clone method that collides due to this argument being optional.

[tkelman]

Looks like a few more have slipped onto master were not remedied here

[eschnett]

I see warnings regarding checked_add being overwritten in coreimg. This looks as if it should indeed be overwritten. If so, can the warning be omitted? Or would a comment in the code suffice?

[NickMcNutt]

+1 if it can be disable in the REPL as that would add a lot of noise.

+1 to the idea, but also to this statement. I find myself redefining functions at the REPL frequently during development, and this would get annoying.

+1 to this as well. Now that it issues a warning, my Jupyter notebooks are full of red boxes taking up the entire page. I've resorted to this hack to get rid of them:

macro nowarn(expr)
    quote
        stderr = STDERR
        stream = open("/dev/null", "a")
        redirect_stderr(stream)
        result = $(esc(expr))
        redirect_stderr(stderr)
        close(stream)
        result
    end
end

@nowarn function myfunc()
    ...
end

[timholy]

While on the topic of making TL;DR posts (#11242 (comment)), I've suddenly developed an interest in adding an @overwrite macro that allows one to choose to suppress the warning added here. To clarify, I like the warning as the default behavior, but I've discovered a situation in which I would also like the option of avoiding it.

Background: without dragging everyone through the details, suffice it to say that a real implementation (ArrayIteration.jl) of the ideas discussed in #15648 seems to be coming along quite well: there's still lots to do, but a lot is in place. The best way to think about it is as a set of run-time and compile-time "hooks" (based on a traits-system) for customizing behavior of functions that handle arrays. (At the current time it remains largely undocumented, unfortunately.)

Given the interest in finalizing julia-0.5 soon, deciding what to do with it is a little bit tricky. On one hand, ArrayIteration.jl is still young and inexperienced, and certainly shouldn't be allowed to wander around town (Base) on its own. On the other hand, the way for it to gain experience is for people to use it to achieve new functionality. The problem is that some of its generalizations violate deeply baked-in assumptions in Base; many methods will produce bugs or segfaults if called on new array types. This situation makes it hard to imagine there being a great deal of adoption, even if it has advantages.

Aside from doing nothing and seeing what happens, there seem to be two "active" decisions we could make:

• Despite ArrayIteration's youth, merge at least some of it into Base, so methods can be generalized during 0.5.x.
• Leave it in a package, and create a second package (Base-ng.jl?) that introduces more generic implementations of the necessary methods.

I suspect the second option is by far the better choice. I am uncertain how many methods from Base would need replacing; it could be anywhere from a couple dozen to possibly well over a hundred. To avoid a long scroll of "Method definition...overwritten" warnings, I propose we introduce an @overwrite macro to make it possible to suppress those warnings.

Thoughts? If you're willing, I can put this together.

[Keno]

FWIW, I've also wanted an @overwrite macro for JuliaParser, which can optionally replace base parse/include. For now I simply redirect STDERR while that's happening, so the user doesn't see the warnings ;).

[timholy]

I thought about that, but what I don't like is the fact that other more serious problems might be hidden.

[tkelman]

The potential for abuse here is really high, but I guess that's not a new or unique concern.

Is there a minimum level of indirection where some initial things could be stubbed out? Or a temporary new level in the abstract type hierarchy you could leverage? Actually making use of a heavily Base-overwriting package is just asking for #265 problems, isn't it?

[Keno]

Actually making use of a heavily Base-overwriting package is just asking for #265 problems, isn't it?

Yes, you have to be very careful.

[timholy]

One possibility is to merge just types.jl and core.jl. That might be good enough. It won't give you much in the way of an implementation of, e.g., sparse-array handling, but that could come later (that's the main/only part that's largely unfinished). It does seem to mostly solve the main corner case for ReshapedArray performance, but until we can "allocate" immutables with references on the stack, ArrayIteration will never reach its real potential (and would likely make things much worse for small arrays). That, plus the fact that it hasn't been put to a serious test yet, makes it seem a little premature, esp. just before a release.

With regards to #265, it's hard to say, but I think it might work out. The problems arise only for array types that aren't in Base; presumably that means the only concerns are methods that explicitly disable specialization (ANY arguments). I haven't looked, but I bet we have relatively few of those that do much in the way of array computation.

[nalimilan]

We could have an unexported macro, so that you would write Base.@overwrite, making it obvious this isn't supposed to be used in general.

[kmsquire]

Also would like the @overwrite macro for kmsquire/Logging.jl#27.

[StefanKarpinski]

Wouldn't it make sense to be able to generally turn off/on warnings? This ties into needing the ability to have more sophisticated control of what to do with debug messages, warnings and errors.

[StefanKarpinski]

Of course, you don't want to miss other warnings that might occurs during the overwrite.

[timholy]

Yes, I agree that we need some kind of general mechanism for this.

[ymer]

These warnings are really bothersome with Jupyter notebook. It would be nice with an easy way to turn them off.

[KristofferC]

I agree. The reason for something being a warning and not an error is because there is legitimate use for it. And then it should be possible to do it silently. Like everyone else I have been solving this by temporarily redirect stderr but that hides other stuff so it isn't very good.

I thought about adding another field to JLOptions but that is immutable and you generally want to just disable this for a while and not for the whole session.

[XilinJia]

Allow me to easily turn off these redefinition warnings please! A zillion warning messages at REPL after the inclusion of a file the second time serves no help to anyone. They serve only on hiding meaningful warnings from the user. Please, let me be able to turn them off!

[ghost]

I also would like to be able to turn off warnings. Mostly because I am writing extensive unit-tests and by the nature of unit-tests all kinds of warnings are triggered ( For example hints for the user that some of his input has been altered, because it was not in the right form ). All these warnings now appear in my unit-test output and it is all just noise to me, because these warnings are not for me.

[StefanKarpinski]

Not really the right place for that request – see the Julia discourse discussion forum or somewhere there's an issue about redirecting error output in general.