Effects on master are influenced by generation of coverage data

[Seelengrab]

Found while debugging the failures of FieldFlags.jl on nightly - the tests run fine on 1.9, haven't yet tried to bisect. Since it's not (yet) through its registration period, you'll have to ]add the github repo directly for reproducing.

Either way, the failures are reproducible with

julia --color=yes --project=. -e 'import Pkg; Pkg.test(;coverage=true)'

but not with

julia --color=yes --project=. -e 'import Pkg; Pkg.test(;coverage=false)'

which make the tests run through. I'll see if I can minimize the failure a bit more.

EDIT: For posteriority, a smaller MWE without dependencies:

[sukera@tower ~]$ julia --code-coverage -q
julia> h(x) = iseven(x) ? "string" : 1
h (generic function with 1 method)

julia> g() = h(2)
g (generic function with 1 method)

julia> Base.infer_effects(g,())
(+c,!e,+n,+t,+s,+m,+i)

[sukera@tower ~]$ julia -q
julia> h(x) = iseven(x) ? "string" : 1
h (generic function with 1 method)

julia> g() = h(2)
g (generic function with 1 method)

julia> Base.infer_effects(g,())
(+c,+e,+n,+t,+s,+m,+i)

[oscardssmith]

this is annoying but intentional. the problem is that asking for code coverage means that code is not effect free (because with code coverage on, deleting a function who's output is not used has the visible effect of changing the coverage number.

[Seelengrab]

:/

Does that mean more or less everyone relying on some effects to be there needs to run their code twice, once with coverage and once without? Or should I not test effects to this extent (would be a shame - I need those guarantees for AVR)?

It's not a long-running testsuite, but here's the smallest reproducer I have for now:

using FieldFlags
using Test

@bitflags struct JETStruct
    a
    _
    b
end

foldableAccess(j::JETStruct) = j.a

effects = Base.infer_effects(foldableAccess, (JETStruct,))
display(effects)
@test Core.Compiler.is_foldable(effects)

with outputs being

[sukera@tower FieldFlags.jl]$ julia --code-coverage -q --project=. repr.jl 
(+c,!e,+n,+t,+s,+m,+i)
Test Failed at [...]
  Expression: Core.Compiler.is_foldable(effects)

ERROR: LoadError: There was an error during testing
[...]
[sukera@tower FieldFlags.jl]$ julia -q --project=. repr.jl 
(+c,+e,+n,+t,+s,+m,+i)

[Seelengrab]

The PR introducing this change is #48598, if I'm not mistaken, which only taints the effect because otherwise the optimizer would remove the dead IR. Is it possible to implement code coverage differently, so that effect testing is not influenced by coverage? Or is there some other way I can test that these functions are foldable?

[Seelengrab]

If anything, I'd argue that tainting :effect_free here is incorrect - the method doesn't have externally visible sideeffects in the program (unless the program is inspecting its own coverage output, that is). Feels a bit similar to the exception that's made for elapsed time - technically observable, but taking it into account means we can't optimize anything.

[oscardssmith]

at the heart of this is a question of what coverage should measure. if coverage is meant to measure code as written, the compiler must not be allowed to change the code run in certain ways. we could probably add a coverage option that allows these options, but it is somewhat unclear what would then be measured.

[Seelengrab]

Here's a smaller MWE, which doesn't need FieldFlags.jl:

[sukera@tower ~]$ julia --code-coverage -q
julia> h(x) = iseven(x) ? "string" : 1
h (generic function with 1 method)

julia> g() = h(2)
g (generic function with 1 method)

julia> Base.infer_effects(g,())
(+c,!e,+n,+t,+s,+m,+i)

[sukera@tower ~]$ julia -q
julia> h(x) = iseven(x) ? "string" : 1
h (generic function with 1 method)

julia> g() = h(2)
g (generic function with 1 method)

julia> Base.infer_effects(g,())
(+c,+e,+n,+t,+s,+m,+i)

This doesn't make sense and is incosistent, as @code_warntype happily reports that this returns a constant, even in the covered mode:

[sukera@tower ~]$ julia --code-coverage -q
julia> h(x) = iseven(x) ? "string" : 1
h (generic function with 1 method)

julia> g() = h(2)
g (generic function with 1 method)

julia> @code_warntype g()
MethodInstance for g()
  from g() @ Main REPL[2]:1
Arguments
  #self#::Core.Const(g)
Body::String
1 ─ %1 = Main.h(2)::Core.Const("string")
└──      return %1

If the tainting were real/proper, this ought to infer Union{Int, String} (which doesn't make sense to do, because the compiler clearly has enough information to propagate the constants).

[KristofferC]

If one of the "effects" of the function is to emit coverage data, then evaluating it at compile time would change that behavior, no?

This doesn't make sense and is incosistent, as @code_warntype happily reports that this returns a constant, even in the covered mode:

This is with coverage:

julia> @code_llvm  g()
;  @ REPL[2]:1 within `g`
define nonnull {}* @julia_g_925() #0 {
top:
  %0 = alloca [8 x i8], align 8
  %lcnt = load volatile i64, i64* inttoptr (i64 105553177974792 to i64*), align 8
  %1 = add i64 %lcnt, 1
  store volatile i64 %1, i64* inttoptr (i64 105553177974792 to i64*), align 8
  %2 = call { {}*, i8 } @j_h_927([8 x i8]* noalias nocapture noundef nonnull %0, i64 signext 2) #0
  %3 = extractvalue { {}*, i8 } %2, 0
  ret {}* %3
}

and without

julia> @code_llvm g()
;  @ REPL[2]:1 within `g`
define nonnull {}* @julia_g_477() #0 {
top:
  %0 = alloca [8 x i8], align 8
  %1 = call { {}*, i8 } @j_h_479([8 x i8]* noalias nocapture noundef nonnull %0, i64 signext 2) #0
  %2 = extractvalue { {}*, i8 } %1, 0
  ret {}* %2
}

[Seelengrab]

"emitting coverage data" is a bad effect in terms of side-effect freeness though, in particular when it puns on "is constant foldable". The reason being that, for the code that we're inspecting with coverage data, the coverage data is not a sideeffect - it's only a behavior changing sideeffect for the testsuite.

[Seelengrab]

In particular, the example I gave in the OP is a longer version of this MWE:

[sukera@tower ~]$ julia --code-coverage -q
julia> struct Foo end

julia> Base.getproperty(f::Foo, s::Symbol) = s === :foo ? "foo!" : getfield(f, s)

julia> f(x::Foo) = x.foo
f (generic function with 1 method)

julia> Base.infer_effects(f,(Foo,))
(+c,!e,+n,+t,+s,+m,+i)

julia> code_warntype(f,(Foo,))
MethodInstance for f(::Foo)
  from f(x::Foo) @ Main REPL[3]:1
Arguments
  #self#::Core.Const(f)
  x::Core.Const(Foo())
Body::String
1 ─ %1 = Base.getproperty(x, :foo)::Core.Const("foo!")
└──      return %1

While the code in the OP doesn't return a Core.Const, this example does - and illustrates the issue nicely. Every part necessary for getproperty to infer a Core.Const is there, and it does do so - yet the function can't constant fold due to the effects introduced by --code-coverage, even though the only thing that could change the effects, the passed in Foo, is not relevant in the branch where the constant is ultimately inferred.

[jakobnissen]

In general, it's not a good idea to have the results of your code rely on certain optimisations that may be disabled when testing. Think of forced boundschecks, and debug mode(s) in some languages. Being able to add additional safety features during testing is a good thing

[KristofferC]

yet the function can't constant fold due to the effects introduced by --code-coverage

With apologies for repeating myself, but if the code is supposed to track every time it is called, how could it be valid to evaluate its result at compile time? Then that would change the output in the coverage files?

[vtjnash]

It is actually an interesting question though, once we expose infer_effects to users in an API, do we try to make it so that the code path taken is not impacted, and so you can still test code coverage without it bypassing the code being tested?

[gbaraldi]

I guess we could make it an invisible effect like a GC allocation? Though that doesn't solve the fact that it shouldn't concrete eval/fold, because it means you might get wrong coverage results

[oscardssmith]

I think a lot of the problems here could be resolved by adding a separate effect for coverage rather than tainting the existing effects. The only reason the change of effects is causing problems is because people want to test that their code has good effects (which for example will be needed for compiling to AVR). If we separated the coverage effect, it would be possible for people to test that their code will work as expected when deployed (without coverage).