[docs] add style guide on abstract types

[odow]

What is this PR?

This PR adds some recommendations to the JuMP style guide that we think you should follow when working with abstract types and method arguments in Julia.

Why

The not-julia article made some very good points.

There was a long discussion on Discourse:

• https://discourse.julialang.org/t/discussion-on-why-i-no-longer-recommend-julia-by-yuri-vishnevsky/81151
  and also one on HackerNews. Here's the relevant JuMP issue: Discussion/style guide updates on abstraction/correctness/composability #2988

Feedback

We're in no rush to merge this, so please chime in with feedback. This will likely take a few rounds of edits to get right.

Closes #2988

[codecov]

Codecov Report

Merging #2995 (eb29820) into master (dc4a786) will increase coverage by 0.68%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #2995      +/-   ##
==========================================
+ Coverage   95.44%   96.13%   +0.68%     
==========================================
  Files          43       32      -11     
  Lines        5824     4135    -1689     
==========================================
- Hits         5559     3975    -1584     
+ Misses        265      160     -105     

Impacted Files	Coverage Δ
src/macros.jl	96.28% <0.00%> (-0.14%)	⬇️
src/JuMP.jl	95.66% <0.00%> (ø)
src/precompile.jl	0.00% <0.00%> (ø)
src/feasibility_checker.jl	100.00% <0.00%> (ø)
src/optimizer_interface.jl	82.00% <0.00%> (ø)
src/_Derivatives/reverse.jl
src/_Derivatives/topological_sort.jl
src/_Derivatives/conversion.jl
src/_Derivatives/linearity.jl
src/_Derivatives/subexpressions.jl
... and 11 more

---

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 dc4a786...eb29820. Read the comment docs.

[mlubin]

This looks good as a starting point for discussions.

[nickrobinson251]

In case it's of interest, the recently released SciML style also has a section on recommendations e.g. https://github.com/SciML/SciMLStyle#errors-should-be-caught-as-high-as-possible-and-error-messages-should-be-contextualized-for-newcommers

I'm not sure what the name is for these kind of guidelines (principles? / philosophies? / dogmas?!), different from the syntactic style suggestions (things JuliaFormatter could in principle automate, "put space around binary operators" etc), but i think it's interesting that these things are being made more explicit. I suppose, like with syntactic style, it may be interesting in time to see where different large projects agree/disagree on various points.

[odow]

it may be interesting in time to see where different large projects agree/disagree on various points.

The tension is definitely the extent to which abstract interfaces are supported.

I'm also not a major fan of things like https://github.com/SciML/SciMLStyle#closures-should-be-avoided-whenever-possible

We spent a lot of effort in the development of MathOptInterface chasing performance, but readability and maintainability are equally important, if not more so.

[matbesancon]

All code seems a bit extreme. I can understand providing an opinion and the advantage it can provide in some cases, but overgeneralizing this can lead to a lot of code to produce errors, and may require overly specific methods to avoid ambiguities with the error fallback.

@oxinabox detailed it more here: https://www.oxinabox.net/2020/04/19/Julia-Antipatterns.html#notimplemented-exceptions

[odow]

The experience is that beginner Julia users find MethodErrors hard to deal with. They might not even have a good concept of what a type is, let alone specifics of why Vector{Float64} <: Vector{Real} is false.

Is "All public code" better? Admittedly, we don't do this perfectly, so it's a standard to strive for, not a fixed rule.

[matbesancon]

Is "All public code" better?

yes that's already bringing a nuance

Admittedly, we don't do this perfectly, so it's a standard to strive for, not a fixed rule.

Yes that's my point, the phrasing implies it is a fixed rule.

[mlubin]

I'm not sure I agree with distinguishing between public and private code. Private code should still follow the style guide. Instead of "All code must [...]", what about "Code should [...]"? The style guide is a set of recommendations, so "must" is out of character.

[oxinabox]

I didn't have any comment on this when tagged because, I think JuMP has its own philosphy that is strong and well-rounded and targetting a different audience to my blog post.

But actually now I do have an opinion because JuMP's current doing of this is annoying to me.
because I was like shadow_price(my_constraint) just now
and I got

julia> shadow_price.(cons)
ERROR: The shadow price is not defined or not implemented for this type of constraint.
Stacktrace:
 [1] error(s::String)
   @ Base ./error.jl:33
 [2] shadow_price(con_ref::ConstraintRef{Model, MathOptInterface.ConstraintIndex{MathOptInterface.VectorAffineFunction{Float64}, MathOptInterface.NormOneCone}, VectorShape})

and my initial instinct was: "Ok well lets see what types it is defined for".
and so I used methods(shadow_price).

julia> methods(shadow_price)
# 4 methods for generic function "shadow_price":
[1] shadow_price(con_ref::ConstraintRef{<:AbstractModel, MathOptInterface.ConstraintIndex{F, S}}) where {S<:MathOptInterface.LessThan, F} in JuMP at /home/oxinabox/.julia/packages/JuMP/Psd1J/src/constraints.jl:1056
[2] shadow_price(con_ref::ConstraintRef{<:AbstractModel, MathOptInterface.ConstraintIndex{F, S}}) where {S<:MathOptInterface.GreaterThan, F} in JuMP at /home/oxinabox/.julia/packages/JuMP/Psd1J/src/constraints.jl:1069
[3] shadow_price(con_ref::ConstraintRef{<:AbstractModel, MathOptInterface.ConstraintIndex{F, S}}) where {S<:MathOptInterface.EqualTo, F} in JuMP at /home/oxinabox/.julia/packages/JuMP/Psd1J/src/constraints.jl:1082
[4] shadow_price(con_ref::ConstraintRef{<:AbstractModel, <:MathOptInterface.ConstraintIndex}) in JuMP at /home/oxinabox/.julia/packages/JuMP/Psd1J/src/constraints.jl:1017

Which of-course lists there as being a method for this.
The one that manually threw the error.

So while i do think JuMP is probably right in having a strong philosphy around these things,
I will just say it is not without cost in terms of usability to experienced julia programmers.
It is probably worth it to be more friendly to less experienced programmers.
That is a call that should be conciously made however.

[odow]

I will just say it is not without cost in terms of usability to experienced julia programmers.
It is probably worth it to be more friendly to less experienced programmers.

Yes, I think we went for the latter.

[mlubin]

We say, "In some cases, the JuMP style guide diverges from the Julia style guide. All such cases will be explicitly noted and justified." That applies to these recommendations and https://docs.julialang.org/en/v1.0.0/manual/style-guide/#Avoid-writing-overly-specific-types-1.

[ChrisRackauckas]

I'm also not a major fan of things like https://github.com/SciML/SciMLStyle#closures-should-be-avoided-whenever-possible

We spent a lot of effort in the development of MathOptInterface chasing performance, but readability and maintainability are equally important, if not more so.

The suggestion to not do that is a suggestion against maintainability and readable error messages. Anonymous functions and closures give unnamed functions in stack traces and give lots of hidden compile time issues, along with some correctness issues that can come from unintentional captured variables. So that's a direct contradiction of the proposed values.

[odow]

Anonymous functions and closures give unnamed functions in stack traces

So the issue is the #5 instead of Fix2 here?

julia> function foo(vector_of_vectors, i)
           return map(Base.Fix2(getindex, i), vector_of_vectors)
       end
foo (generic function with 1 method)

julia> function bar(vector_of_vectors, i)
           return map(v -> v[i], vector_of_vectors)
       end
bar (generic function with 1 method)

julia> vector_of_vectors = [rand(3) for _ in 1:4]
4-element Vector{Vector{Float64}}:
 [0.2944113044040204, 0.1502238100477964, 0.7719060173197017]
 [0.5503852948486079, 0.12060690924862927, 0.9112641636545447]
 [0.8292675030520171, 0.8359221806182853, 0.04041374985594448]
 [0.08277416126462644, 0.14552667363773697, 0.7186541169561522]

julia> foo(vector_of_vectors, 4)
ERROR: BoundsError: attempt to access 3-element Vector{Float64} at index [4]
Stacktrace:
 [1] getindex
   @ ./array.jl:805 [inlined]
 [2] Fix2
   @ ./operators.jl:1002 [inlined]
 [3] iterate
   @ ./generator.jl:47 [inlined]
 [4] _collect
   @ ./array.jl:695 [inlined]
 [5] collect_similar
   @ ./array.jl:606 [inlined]
 [6] map
   @ ./abstractarray.jl:2294 [inlined]
 [7] foo(vector_of_vectors::Vector{Vector{Float64}}, i::Int64)
   @ Main ./REPL[6]:2
 [8] top-level scope
   @ REPL[9]:1

julia> bar(vector_of_vectors, 4)
ERROR: BoundsError: attempt to access 3-element Vector{Float64} at index [4]
Stacktrace:
 [1] getindex
   @ ./array.jl:805 [inlined]
 [2] #5
   @ ./REPL[7]:2 [inlined]
 [3] iterate
   @ ./generator.jl:47 [inlined]
 [4] _collect
   @ ./array.jl:695 [inlined]
 [5] collect_similar
   @ ./array.jl:606 [inlined]
 [6] map
   @ ./abstractarray.jl:2294 [inlined]
 [7] bar(vector_of_vectors::Vector{Vector{Float64}}, i::Int64)
   @ Main ./REPL[7]:2
 [8] top-level scope
   @ REPL[10]:1

I guess that's a trade-off, but I'd rather code that was easier to read than stack traces that were easier to read.

along with some correctness issues

The compile issues are known and also part of the trade-off, but what are the correctness issues?

[ChrisRackauckas]

So the issue is the #5 instead of Fix2 here?
I guess that's a trade-off, but I'd rather code that was easier to read than stack traces that were easier to read.

Yes, you get names for everything and they can be documented. Though if you do use a anonymous function, one thing you could do is used a named anonymous function. This is part of the ChainRules style rules, as seen here:

https://github.com/JuliaDiff/ChainRules.jl/blob/v1.35.3/src/rulesets/LinearAlgebra/uniformscaling.jl

where if this is seen in a stacktrace, it will use the name plus_back instead of an obscure #5. That's one way to make things a bit nicer (and is very crucial for debuggable AD stacktraces).

The compile issues are known and also part of the trade-off, but what are the correctness issues?

@chriselrod had a bunch of examples of some difficult to debug cases which prompted him to write that rule into the style guide, so I'll let him share those.

[chriselrod]

The compile issues are known and also part of the trade-off, but what are the correctness issues?

Here is a minimal example:

julia> let
         s = Threads.Atomic{Int}(0);
         Threads.@threads for i = 1:10000
           x = i
           sleep(1e-3);
           Threads.atomic_add!(s,x)
         end
         s[]
       end
50005000

julia> sum(1:10_000)
50005000

julia> let
         s = Threads.Atomic{Int}(0);
         Threads.@threads for i = 1:10000
           x = i
           sleep(1e-3);
           Threads.atomic_add!(s,x)
         end
         x = 0
         s[]
       end
50046631

This behavior is also highly non-local, so it can be hard to track down in larger, more complicated, programs.
The non-locality also makes reasoning about the behavior of code more challenging.
Suddenly, you can no longer take the behavior of any piece of code in isolation.
You can have two correct pieces of code that don't interact, and move one in a manner that seems innocuous, and suddenly the behavior changes.

Note that the amount of space between defining x above and the @threads can be arbitrarily large, so long as they're still in the same scope.

Also, note the let block is important. Copy/pate the code (without the let) into your REPL, and you'll get the correct answer.

[chriselrod]

but readability and maintainability are equally important, if not more so.

These are precisely why I don't want closures in a code base.
I want to keep spooky action at a distance to a minimum.

Unless your definition of "readability" is "because local reasoning is impossible, you're always forced to read the entire surrounding context to understand possible non-local effects that", and your definition of "maintainability" has room for heisenbugs introduced by refactoring code, where simply the act of moving one correct piece of code suddenly changes the behavior of another correct piece.

It's too bad that pretty syntax was given such awful semantics.
I dread the day the performance issues get fixed, causing people to start liberally sprinkling their code with sporadic and hard to track down correctness bugs.

To me, easily understanding the behavior of code is more important than literally superficial details like surface syntax.

[odow]

Okay, so to check my understanding, the issue is that x = 0 introduces x into the scope of the let-block, so the for-loop captures x and modifies it in the threading which is not thread-safe. But if you went local x = i, presumably that would work.

It also seems like something that captures x is a closure, but which doesn't lead to the inner x getting confused for it will also work:

julia> function foo(f)
           s = Threads.Atomic{Int}(0)
           Threads.@threads for i = 1:10000
               x = f(i)
               sleep(1e-3)
               Threads.atomic_add!(s, x)
           end
           return s[]
       end
foo (generic function with 1 method)

julia> let
           x = 0
           foo(i -> i + x)
       end
50005000

So the problem isn't really closures, it's dealing with scoping issues and thread-safety in threading code? That seems like a tricky footgun to get right, but not a reason to avoid closures in all cases.

Anyway, we're derailing the conversation from the actual PR, which was about MethodErrors. There's no suggestion in the JuMP style guide at present to use or avoid closures. It was just an off-hand comment about some differences.

[chriselrod]

if you went local x = i, presumably that would work.

Yes. That's what I do in the proprietary code. It's of course easier to add local in front of every line of a big closure than to rewrite the code not to use closures.
Perhaps a softer guideline is "closures are okay so long as every assignment is prefaced by local".
I would still strongly discourage "global", preferring Ref if that's the actually intended behavior (which it of course never is).
That would give the code the property of being able to move it around without changing its meaning, which is IMO a desirable property / makes it much easier to understand by mitigating the significance of context.

So the problem isn't really closures

I would say the problem is that closures capture and mutate bindings, creating global state in code where if you don't look at the entire program, it looks like things are local.

Mutable global state is widely considered bad.
We generally discourage people from defining

const MYOPTIONS = Ref{MyOptionType}()

as means of passing around information / controlling the behavior of their code, suggesting they instead pass their options around as a (keyword) argument to all their functions that need it.
This would especially be the recommendation if it is a variable being actively mutated as part of their computations.

The problem with closures are that they randomly make people's code follow really bad/strongly discouraged practices without anyone even realizing it, and hence introduce the associated bugs that people weren't even looking out for.

[odow]

One place JuMP uses mutating closures is in callbacks https://jump.dev/JuMP.jl/stable/manual/callbacks/#Lazy-constraints. Most of the JuMP API is mutating-by-default, so we already don't work with threading.

The bigger problem I see is that to the general user, it isn't actually obvious that this is a closure, or that it leads to non-local behavior.

julia> let
         s = Threads.Atomic{Int}(0);
         Threads.@threads for i = 1:10000
           x = i
           sleep(1e-3);
           Threads.atomic_add!(s,x)
         end
         x = 0
         s[]
       end

You almost need a Julia flag (or a linter) julia --error-on-modify-captured-variables that catches this at the compiler level. Leaving it to the user to notice ain't going to work.

[chriselrod]

You almost need a Julia flag (or a linter) julia --error-on-modify-captured-variables that catches this at the compiler level.

If the LSP would warn about it reliably, that'd solve most of my griping.
I'm not sure how difficult that'd be. On the one hand, you don't even need type information to figure that out, but on the other you need to be able to expand macros. Threads.@threads as you noted, as well as DSLs introduce a lot of closures.

If it were always obvious what was happening, there wouldn't be much of a problem.

Maybe I should file an issue/feature request at LanguageServer.jl?

[odow]

@matbesancon, I tweaked the wording of the assumption checks.

[matbesancon]

yes this is better. I'm overall not a fan of overly defensive styles but it is definitely helping new comers

[odow]

What about instead of validating assumptions, we say you should provide a function that the user can call in their tests which checks assumptions?

[matbesancon]

that sounds like a good alternative to suggest

[odow]

Okay, I think we're at a point where we can merge this. We can always continue to tweak it going forward.