Repository: JuliaPlots/Makie.jl (julia)

Issue:
Wrong error when trying to set axis elements in `hist!` call
Per a discussion on Slack, I'm seeing:

```
f = Figure()
ax1 = Axis(f[1, 1])
hist!(ax1, rand(100), axis = (xlabel = "value", ylabel = "occurences"))

setting axis for scene via plot attribute not supported anymore

Stacktrace:
  [1] error(s::String)
    @ Base .\error.jl:33
  [2] plot!(scene::Scene, P::Type{Combined{Makie.hist, Tuple{Vector{Float64}}}}, attributes::Attributes, input::Tuple{Observable{Vector{Float64}}}, args::Observable{Tuple{Vector{Float64}}})
    @ Makie C:\Users\ngudat\.julia\packages\Makie\fEZv2\src\interfaces.jl:399
  [3] plot!(scene::Scene, P::Type{Combined{Makie.hist}}, attributes::Attributes, args::Vector{Float64}; kw_attributes::Base.Pairs{Symbol, Union{}, Tuple{}, NamedTuple{(), Tuple{}}})
    @ Makie C:\Users\ngudat\.julia\packages\Makie\fEZv2\src\interfaces.jl:320
  [4] plot!
    @ C:\Users\ngudat\.julia\packages\Makie\fEZv2\src\interfaces.jl:288 [inlined]
  [5] plot!(la::Axis, P::Type{Combined{Makie.hist}}, attributes::Attributes, args::Vector{Float64}; kw_attributes::Base.Pairs{Symbol, Union{}, Tuple{}, NamedTuple{(), Tuple{}}})
    @ Makie.MakieLayout C:\Users\ngudat\.julia\packages\Makie\fEZv2\src\makielayout\blocks\axis.jl:723
  [6] plot!
    @ C:\Users\ngudat\.julia\packages\Makie\fEZv2\src\makielayout\blocks\axis.jl:718 [inlined]
  [7] #plot!#379
    @ C:\Users\ngudat\.julia\packages\Makie\fEZv2\src\makielayout\blocks\axis.jl:735 [inlined]
  [8] hist!(::Axis, ::Vararg{Any}; attributes::Base.Pairs{Symbol, NamedTuple{(:xlabel, :ylabel), Tuple{String, String}}, Tuple{Symbol}, NamedTuple{(:axis,), Tuple{NamedTuple{(:xlabel, :ylabel), Tuple{String, String}}}}})
    @ Makie C:\Users\ngudat\.julia\packages\MakieCore\oBlaS\src\recipes.jl:37
  [9] top-level scope
    @ In[137]:3
 [10] eval
    @ .\boot.jl:373 [inlined]
 [11] include_string(mapexpr::typeof(REPL.softscope), mod::Module, code::String, filename::String)
    @ Base .\loading.jl:1196
```

This is on CairoMakie 0.8.3, Julia 1.7.2, Win10

Pull request description:
# Description

Fixes #2005

A more informative error is thrown when `axis` or `figure` keywords end up in the 
plotting functions of Axis, Axis3 and LScene. This will usually have happened 
because a mutating plotting function was used accidentally.

## Type of change

Delete options that do not apply:

- [x] Bug fix (non-breaking change which fixes an issue)

## Checklist

- [x] Added unit tests for new algorithms, conversion methods, etc.

Patch:
diff --git a/src/figureplotting.jl b/src/figureplotting.jl
index 5d3185a2871..2a67443e999 100644
--- a/src/figureplotting.jl
+++ b/src/figureplotting.jl
@@ -22,6 +22,12 @@ function _validate_nt_like_keyword(@nospecialize(kw), name)
     end
 end
 
+function _disallow_keyword(kw, attributes)
+    if haskey(attributes, kw)
+        throw(ArgumentError("You cannot pass `$kw` as a keyword argument to this plotting function. Note that `axis` can only be passed to non-mutating plotting functions (not ending with a `!`) that implicitly create an axis, and `figure` only to those that implicitly create a `Figure`."))
+    end
+end
+
 function plot(P::PlotFunc, args...; axis = NamedTuple(), figure = NamedTuple(), kw_attributes...)
     
     _validate_nt_like_keyword(axis, "axis")
@@ -63,7 +69,7 @@ end
 function plot(P::PlotFunc, gp::GridPosition, args...; axis = NamedTuple(), kwargs...)
 
     _validate_nt_like_keyword(axis, "axis")
-    
+
     f = get_top_parent(gp)
 
     c = contents(gp, exact = true)
@@ -102,8 +108,8 @@ end
 function plot!(P::PlotFunc, gp::GridPosition, args...; kwargs...)
 
     c = contents(gp, exact = true)
-    if !(length(c) == 1 && c[1] isa Union{Axis, LScene})
-        error("There needs to be a single axis at $(gp.span), $(gp.side) to plot into.\nUse a non-mutating plotting command to create an axis implicitly.")
+    if !(length(c) == 1 && can_be_current_axis(c[1]))
+        error("There needs to be a single axis-like object at $(gp.span), $(gp.side) to plot into.\nUse a non-mutating plotting command to create an axis implicitly.")
     end
     ax = first(c)
     plot!(P, ax, args...; kwargs...)
@@ -157,7 +163,7 @@ function plot!(P::PlotFunc, gsp::GridSubposition, args...; kwargs...)
     gp = layout[gsp.rows, gsp.cols, gsp.side]
 
     c = contents(gp, exact = true)
-    if !(length(c) == 1 && c[1] isa Union{Axis, LScene})
+    if !(length(c) == 1 && can_be_current_axis(c[1]))
         error("There is not just one axis at $(gp).")
     end
     ax = first(c)
diff --git a/src/makielayout/blocks/axis.jl b/src/makielayout/blocks/axis.jl
index db16690c5c3..dc8bdcfd01b 100644
--- a/src/makielayout/blocks/axis.jl
+++ b/src/makielayout/blocks/axis.jl
@@ -726,6 +726,9 @@ function Makie.plot!(
 
     allattrs = merge(attributes, Attributes(kw_attributes))
 
+    _disallow_keyword(:axis, allattrs)
+    _disallow_keyword(:figure, allattrs)
+
     cycle = get_cycle_for_plottype(allattrs, P)
     add_cycle_attributes!(allattrs, P, cycle, la.cycler, la.palette)
 
diff --git a/src/makielayout/blocks/axis3d.jl b/src/makielayout/blocks/axis3d.jl
index 6b63ba16ef8..a8680318f7e 100644
--- a/src/makielayout/blocks/axis3d.jl
+++ b/src/makielayout/blocks/axis3d.jl
@@ -274,6 +274,9 @@ function Makie.plot!(
 
     allattrs = merge(attributes, Attributes(kw_attributes))
 
+    _disallow_keyword(:axis, allattrs)
+    _disallow_keyword(:figure, allattrs)
+
     cycle = get_cycle_for_plottype(allattrs, P)
     add_cycle_attributes!(allattrs, P, cycle, ax.cycler, ax.palette)
 
diff --git a/src/makielayout/blocks/scene.jl b/src/makielayout/blocks/scene.jl
index e4b0c282d74..f1c05989190 100644
--- a/src/makielayout/blocks/scene.jl
+++ b/src/makielayout/blocks/scene.jl
@@ -11,6 +11,8 @@ end
 
 function Makie.plot!(P::Makie.PlotFunc, ls::LScene, args...; kw_attributes...)
     attributes = Makie.Attributes(kw_attributes)
+    _disallow_keyword(:axis, attributes)
+    _disallow_keyword(:figure, attributes)
     Makie.plot!(ls, P, attributes, args...)
 end
 


Test patch:
diff --git a/test/makielayout.jl b/test/makielayout.jl
index 2ae0451090f..6e46bdd5d0b 100644
--- a/test/makielayout.jl
+++ b/test/makielayout.jl
@@ -186,6 +186,27 @@ end
     @test leg.valign[] == 0.8
 end
 
+# issue 2005
+@testset "invalid plotting function keyword arguments" begin
+    for T in [Axis, Axis3, LScene]
+        f = Figure()
+        kw = (; backgroundcolor = :red)
+        @test_throws ArgumentError lines(f[1, 1], 1:10, figure = kw)
+        @test_nowarn               lines(f[1, 2], 1:10, axis = kw)
+        @test_throws ArgumentError lines(f[1, 3][1, 1], 1:10, figure = kw)
+        @test_nowarn               lines(f[1, 4][1, 2], 1:10, axis = kw)
+        ax = T(f[1, 5])
+        @test_throws ArgumentError lines!(ax, 1:10, axis = kw)
+        @test_throws ArgumentError lines!(ax, 1:10, axis = kw)
+        @test_throws ArgumentError lines!(1:10, axis = kw)
+        @test_throws ArgumentError lines!(1:10, figure = kw)
+        @test_nowarn               lines!(1:10)
+        @test_throws ArgumentError lines!(f[1, 5], 1:10, figure = kw)
+        @test_throws ArgumentError lines!(f[1, 5], 1:10, axis = kw)
+        @test_nowarn               lines!(f[1, 5], 1:10)
+    end
+end
+
 @testset "Linked axes" begin
     # this tests a bug in 0.17.4 where the first axis targetlimits
     # don't change because the second axis has limits contained inside those