Lower compat to 1.6

[gdalle]

You wanna see where it breaks, @Seelengrab? I'll show you where it breaks 😈

[Seelengrab]

And off it goes!

[codecov-commenter]

Codecov Report

Patch coverage: 66.66% and project coverage change: +0.60 🎉

Comparison is base (af7bbcf) 60.00% compared to head (5378f9d) 60.60%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main       #4      +/-   ##
==========================================
+ Coverage   60.00%   60.60%   +0.60%     
==========================================
  Files           1        1              
  Lines         100       99       -1     
==========================================
  Hits           60       60              
+ Misses         40       39       -1     

Impacted Files	Coverage Δ
src/RequiredInterfaces.jl	60.60% <66.66%> (+0.60%)	⬆️

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

[Seelengrab]

Oh, these failures due to @testset let are not surprising - maybe I should move these testing functions to an extension and version them there, though the test failures are going to be quite a bit poorer information wise without @testset let 🤔

[gdalle]

By the looks of it the error is the same on all previous versions, and pretty fixable:

LoadError: Expected begin/end block or for loop as argument to @testset
in expression starting at /src/RequiredInterfaces.jl:207

[Seelengrab]

Well yes, but that's just hiding the true failures in the package itself, as those are from a macro expansion that just didn't exist before 1.9. You can try removing that @testset here and we can see what happens afterwards. It's "just" UX, not a functional part of the package.

[Seelengrab]

Ok, cursory local testing with 1.6.7 and 1.8.5 suggests simply removing that UX convenience makes it work on 1.6! So if you add that removal to this PR (possibly behind a version check, to keep the nice printing on 1.9+?) and if CI agrees, we can do this :) Seems like the code is unexpectedly less brittle than I thought.

Once it lands here, I'll make a PR to General to change the compat notice there too.

[gdalle]

So if you add that removal to this PR

What "UX convenience are you referring to?

Once it lands here, I'll make a PR to General to change the compat notice there too.

Why does it require a PR to General? You're gonna tag a new version anyway, right?

[Seelengrab]

What "UX convenience are you referring to?

@testset let the variables passed in that let-block as part of the failure when displaying the testset. I just thought that was a nice convenience, but it's no biggy not to have that.

Why does it require a PR to General? You're gonna tag a new version anyway, right?

Right, that's true - then this will land with fixing #2. Seems like CI still doesn't like the change though - there are trues instead of expected interface violations, meaning the detection of "does this throw the correct error?" failed.. I'll have a look after #2/possibly tomorrow.

[gdalle]

No rush! It's a cool package so I'm happy to lend a hand but I didn't want to pressure you in any way

[Seelengrab]

Yeah this is getting weird - I can't reproduce the failures locally at all, only in CI 🤔 Are you able to get those failures locally?

[gdalle]

Tests pass locally on my Mac with Julia 1.6...

[gdalle]

Although I don't understand why check_interface_implemented doesn't just return true or false

[Seelengrab]

A non-Boolean result in @test shows up in the test summary; just false doesn't give an indication of which parts of the interface are missing. Makes it easier to look at the test result and see exactly what is missing in the implementation :)

The underlying issue would be the same; CI is getting a true when it should get a false (or the Vector). So seems like the first step is replicating what is going wrong in CI...

[Seelengrab]

Ok, got a breadcrumb - I can reproduce the failure locally on 1.6.7 by doing

julia> import Pkg

julia> d = Dict{Symbol, Any}(:coverage => true, :julia_args => ["--check-bounds=yes"]);

julia> Pkg.test(;d...)

but ONLY through that. I can't yet get it to reproduce with a regular julia process..

Hah! Launching julia with

~/Downloads/jl/julia-1.6.7/bin/julia --project=. --check-bounds=yes --code-coverage=all

and running this:

julia> using RequiredInterfaces

julia> const RI = RequiredInterfaces
RequiredInterfaces

julia> abstract type TestInterface end

julia> struct TestViolator <: TestInterface end

julia> @required TestInterface testfunc(::Int, ::TestInterface)
testfunc (generic function with 1 method)

julia> RI.check_interface_implemented(TestInterface, TestViolator)
true

and we got a reproducer!

[Seelengrab]

Minimizing further, the --check-bounds is not needed:

[sukera@tower RequiredInterfaces.jl]$ ~/Downloads/jl/julia-1.6.7/bin/julia --project=. --code-coverage=all tmp.jl 
true
[sukera@tower RequiredInterfaces.jl]$ ~/Downloads/jl/julia-1.6.7/bin/julia --project=. --code-coverage=user tmp.jl 
true
[sukera@tower RequiredInterfaces.jl]$ ~/Downloads/jl/julia-1.6.7/bin/julia --project=. --code-coverage=none tmp.jl 
Tuple{Any, Tuple}[(testfunc, (Int64, TestViolator))]

so only if we don't produce coverage at all, it works as expected. This is fixable though!

[gdalle]

This is why I don't play with macros 🤓

[Seelengrab]

Alright, this is implemented on main now, so I'll close this.

This is why I don't play with macros nerd_face

Oh the macro had nothing to do with this - the underlying issue was that pre this PR in Julia, coverage tracking was done with a Expr(:code_coverage_effect), which shuffeled stuff around in the IR returned by code_typed. I didn't account for that, so the code (mistakenly) thought it didn't end up throwing the RI.NotImplementedError. There's lots of similar issues, like JuliaLang/julia#49978.

The new version is robust to that and works accross 1.6-1.9, so that's good - but it will undoubtedly break in the future again. My gut says that the only proper fix is to use an abstract interpreter for this, but that'll have to wait until the next Julia LTS including the (by then hopefully stable) API is released.

[gdalle]

Awesome, thanks @Seelengrab!
Two questions:

• should we pre-emptively limit compatibility with future Julia versions in order not to overpromise?
• could you register v0.1.3 and earn yourself a first official dependency (HMMs)? 😇

[Seelengrab]

should we pre-emptively limit compatibility with future Julia versions in order not to overpromise?

Well, I know for a fact that the current code works on 1.10, and since I usually run a close-to-nightly build anyway, I'm likely going to notice if it breaks :) Retroactively capping versions seems less hassle than testing old versions for compatibility with new julia versions.

We could have it run nightly CI once per day?

could you register v0.1.3 and earn yourself a first official dependency (HMMs)? innocent

Will do - I'm just waiting on the doc CI finishing up!

[gdalle]

We could have it run nightly CI once per day?

Maybe a good solution

[Seelengrab]

Alright, it's released!