Replace N field with result_index

[matbesancon]

Closes #960

[matbesancon]

one question is do we want to add a deprecation on:

• attr.N
• _result_index_field(attr)

[matbesancon]

@blegat same as #979, if we add the @deprecate, we don't need to wait for v0.10

[codecov-io]

Codecov Report

Merging #999 into master will decrease coverage by 0.01%.
The diff coverage is 93.6%.

@@            Coverage Diff             @@
##           master     #999      +/-   ##
==========================================
- Coverage   95.22%   95.21%   -0.02%     
==========================================
  Files          99       99              
  Lines       11159    11233      +74     
==========================================
+ Hits        10626    10695      +69     
- Misses        533      538       +5

Impacted Files	Coverage Δ
src/Bridges/graph.jl	100% <100%> (ø)	⬆️
src/Utilities/functions.jl	93.15% <100%> (ø)	⬆️
src/FileFormats/LP/LP.jl	97.8% <100%> (+0.15%)	⬆️
src/constraints.jl	89.28% <100%> (+0.39%)	⬆️
src/Test/contconic.jl	99.26% <100%> (+0.01%)	⬆️
src/variables.jl	57.89% <33.33%> (-4.61%)	⬇️
src/sets.jl	95.28% <89.47%> (-1.31%)	⬇️
src/Bridges/Constraint/interval.jl	86.66% <91.11%> (-4.83%)	⬇️
src/Bridges/bridge_optimizer.jl	96.08% <0%> (+0.65%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5a5ac6f...98121fa. Read the comment docs.

[matbesancon]

not sure the coverage makes sense here

[blegat]

@blegat same as #979, if we add the @deprecate, we don't need to wait for v0.10

Still don't understand why. For instance, between Julia v0.x.y and v0.x.y+1, no deprecation where added, deprecation where only added between v0.x and v0.x+1. This allowed packages to write v0.x in their REQUIRE file and be sure that they users won't see any warnings. Then they will update their package for v0.x+1 using warnings to help them.
If you add deprecations in a patch release then the packages depending on you cannot guarantee their users that they will see warnings that are not due to them (since it's due to a dependency for which one of it's dependency added deprecations in a patch release).

[mlubin]

Agreed. We don't want a patch release of MOI that causes all of the solver interfaces to start printing warnings. That's a bad user experience.

[matbesancon]

sure but at least it creates a signal for the packages depending on MOI to fix the deprecations, which is much easier than looking for breaking changes in a changelog

[matbesancon]

so would we prefer having deprecation warnings in a v0.10 or directly breaking changes?

[blegat]

so would we prefer having deprecation warnings in a v0.10 or directly breaking changes?

I prefer deprecation warnings in v0.10 rather than breaking changes when possible.

[mlubin]

I think the deprecation warnings will be useful to help update the wrappers before the first version compatible with MOI 0.10 is tagged. Secondarily, in the rare case where some part of the wrapper isn't updated, I'd rather a user see a deprecation warning than an error.

[matbesancon]

Regarding a deprecation warning, it can be trickier than I thought. We would want to deprecate getproperty(attr::A, f::Symbol) for A the attribute types that had an N before. To add a deprecation warning, we would need to

function Base.getproperty(attr:A, f::Symbol)
    f2 = if f == :N 
        @warn "N has been deprecated, using attr.result_index instead"
        :result_index
    else
        f
    end
    getfield(attr, f)
end

The problem is that we add this level of indirection for all calls to attr.N which people were normally not using in the first place, since there was the dedicated _result_index_field(attr) before. I would say we can change the field name and leave only one of the two deprecations, possibly giving a warning in the NEWS.md

Opinions?

[odow]

The _result_index_field was an internal MOI function. It shouldn't have been used by external solvers.

I would prefer to see:

function Base.getproperty(attr::Union{VariableValue}, field::Symbol)
    if field == :N 
        @warn("The `.N` field has been deprecated. Use `.result_index` instead")
        return getfield(attr, :result_index)
    end
    return getfield(attr, field)
end

Where the union includes all the structs being changed. I don't think we can merge this before 0.10, however.

[matbesancon]

I don't think we can merge this before 0.10, however.

yes I think this is the plan. This implies we have the warning and indirection staying for the whole 0.10 series and then being removed in 0.11 / 1.0

[odow]

Julia uses constant propagation, so the if field == :N check is completely removed.

struct Foo
    b::Int
end

function Base.getproperty(f::Foo, field::Symbol)
    if field == :a
        @warn("Use `.b` instead.")
        return getfield(f, :b)
    end
    return getfield(f, field)
end

struct Bar
    b::Int
end

test_b(x) = x.b

f = Foo(1)
b = Bar(1)

julia> @code_llvm test_b(f)

;  @ REPL[58]:1 within `test_b'
define i64 @julia_test_b_975([1 x i64]* nocapture nonnull readonly dereferenceable(8)) {
top:
; ┌ @ REPL[56]:6 within `getproperty'
   %1 = getelementptr inbounds [1 x i64], [1 x i64]* %0, i64 0, i64 0
; └
  %2 = load i64, i64* %1, align 8
  ret i64 %2
}

julia> @code_llvm test_b(b)

;  @ REPL[58]:1 within `test_b'
define i64 @julia_test_b_976([1 x i64]* nocapture nonnull readonly dereferenceable(8)) {
top:
; ┌ @ Base.jl:33 within `getproperty'
   %1 = getelementptr inbounds [1 x i64], [1 x i64]* %0, i64 0, i64 0
; └
  %2 = load i64, i64* %1, align 8
  ret i64 %2
}

[matbesancon]

The deprecations had to come after the definition of all attributes with this property, including NLPBlockDual so I placed them in a separate file. This file can be used to add all deprecations, that makes it easier to know what can / should be removed on breaking changes

[matbesancon]

CI will keep failing on nightly while this odd upstream problem remains