disable method overwrite warnings if both functions are in Main

[KristofferC]

Implements @vtjnash suggestion in #18725 (comment)

julia> f(x) = 3;

julia> f(x) = 3;

julia> module F
       f(x) = 3
       f(x) = 3
       end;
WARNING: Method definition f(Any) in module F at REPL[2]:2 overwritten at REPL[2]:3.

fixes #18725

Stuff like JuliaPlots/Plots.jl#508 or

julia> f(g) = g();

julia> g() = 1;

julia> f(g)
1

julia> g() = 2

julia> f(g)
1

are of course a little bit scary to happen without a warning.

[tkelman]

can we make this only the case when isinteractive ?

[tkelman]

indent is off

[tkelman]

neither path or stream are used here, why the mktemp?

[KristofferC]

Accident from previous test iterations :P

[KristofferC]

How should I test it then @tkelman. Start a new worker with the needed command flag?

[tkelman]

If that works? We don't do all that much testing of warnings, though that doesn't mean we shouldn't try to.

[KristofferC]

Regarding isinteractive there is a bit of a mismatch on the julia side vs c side:

julia> isinteractive()
true

julia> Base.JLOptions().isinteractive
0

because the function that starts the REPL sets the isinteractive variable here:

julia/base/client.jl

Line 346 in 0287a99

global is_interactive = true

.

I could of course just jl_call into julias isinteractive(). Would that be reasonable?

[vtjnash]

If we are going to make this conditional on Base.isinteractive(), it should probably be a push model – something like:

ccall(:jl_suppress_interactive_warnings, Cint, (Cint,), true)

static int suppress_interactive_warnings;
JL_DLLEXPORT int jl_suppress_interactive_warnings(int nowarn)
{
    int last = suppress_interactive_warnings;
    suppress_interactive_warnings = nowarn;
    return last;
}

[JeffBezanson]

I think it would be better to simply add an option to disable the warning, at least until #265 is fixed. It's very hard to make the case that some method overwrites don't need warnings, and others do.

[KristofferC]

I agree.

[vtjnash]

I think it would be better to simply add an option to disable the warning, at least until #265 is fixed.

Either this should be a warning or it shouldn't. This isn't really doing much to warn for #265 occurrences, since it only handles one very specific case, so that seems like a red herring.

It's very hard to make the case that some method overwrites don't need warnings, and others do.

Adding a flag is annoying since it's one more thing that will differ between master and worker processes. Jeff was the one who originally convinced me it's better to avoid options when we can just pick a sane default (such as warning only for code inside a module), or we could move this entirely to a linter.

[JeffBezanson]

We already have an option to disable deprecation warnings, so options of this kind are clearly considered acceptable at this point.

True, it does not catch all cases of #265, but the case in #18725 (comment) is especially bad, and I feel it's very helpful to give some indication of what might be wrong. Unlike other cases of #265, this one is actionable, since you can usually avoid overwriting methods (e.g. use different names instead of repeatedly redefining f(x) = in a test suite).

This warning is perfectly fine except in the marginal case where you reload lots of definitions and find the excess text annoying. An option seems like the right way to handle that, so that you can temporarily silence the warnings to make it easier to see other messages.

[KristofferC]

How about showing it only once per session for Main Main overwrites?