From a49bf1eff4491189b2fa1946934afb33c8a28b84 Mon Sep 17 00:00:00 2001
From: Teun van den Brand <49372158+teunbrand@users.noreply.github.com>
Date: Wed, 28 Aug 2024 09:24:09 +0200
Subject: [PATCH] Check ellipses in several places (#6031)

* helper function to throw error demoted to warning

* check `fortify()` dots

* check `get_alt_text()` dots

* check `deprecated_guide_args()` dots

* check for unknown labels

* add news bullet
---
 NEWS.md                         |  2 ++
 R/fortify.R                     |  7 +++++--
 R/guide-legend.R                |  1 +
 R/labels.R                      | 28 ++++++++++++++++++++++++----
 R/utilities.R                   | 12 ++++++++++++
 man/fortify.Rd                  |  2 +-
 man/get_alt_text.Rd             |  2 +-
 tests/testthat/_snaps/guides.md | 11 +++++++++++
 tests/testthat/_snaps/labels.md | 12 ++++++++++++
 tests/testthat/_snaps/plot.md   |  7 +++++++
 tests/testthat/test-guides.R    |  9 +++++++++
 tests/testthat/test-labels.R    |  9 +++++++++
 tests/testthat/test-plot.R      |  1 +
 13 files changed, 95 insertions(+), 8 deletions(-)

diff --git a/NEWS.md b/NEWS.md
index 8c5ca3c555..c527454cdd 100644
--- a/NEWS.md
+++ b/NEWS.md
@@ -168,6 +168,8 @@
 * `theme_classic()` now has black ticks and text instead of dark gray. In 
   addition, `theme_classic()`'s axis line end is `"square"` (@teunbrand, #5978).
 * {tibble} is now suggested instead of imported (@teunbrand, #5986)
+* The ellipsis argument is now checked in `fortify()`, `get_alt_text()`, 
+  `labs()` and several guides (@teunbrand, #3196).
 
 # ggplot2 3.5.1
 
diff --git a/R/fortify.R b/R/fortify.R
index 3a61e3ce49..5b5b7c5171 100644
--- a/R/fortify.R
+++ b/R/fortify.R
@@ -8,9 +8,12 @@
 #' @seealso [fortify.lm()]
 #' @param model model or other R object to convert to data frame
 #' @param data original dataset, if needed
-#' @param ... other arguments passed to methods
+#' @inheritParams rlang::args_dots_used
 #' @export
-fortify <- function(model, data, ...) UseMethod("fortify")
+fortify <- function(model, data, ...) {
+  warn_dots_used()
+  UseMethod("fortify")
+}
 
 #' @export
 fortify.data.frame <- function(model, data, ...) model
diff --git a/R/guide-legend.R b/R/guide-legend.R
index 6f0b98ac33..4dd088f358 100644
--- a/R/guide-legend.R
+++ b/R/guide-legend.R
@@ -688,6 +688,7 @@ deprecated_guide_args <- function(
   default.unit = "line",
   ...,
   .call = caller_call()) {
+  warn_dots_used(call = .call)
 
   args <- names(formals(deprecated_guide_args))
   args <- setdiff(args, c("theme", "default.unit", "...", ".call"))
diff --git a/R/labels.R b/R/labels.R
index 7662153e91..20cc929d37 100644
--- a/R/labels.R
+++ b/R/labels.R
@@ -18,8 +18,8 @@ update_labels <- function(p, labels) {
 
 # Called in `ggplot_build()` to set default labels not specified by user.
 setup_plot_labels <- function(plot, layers, data) {
-  # Initiate from user-defined labels
-  labels <- plot$labels
+  # Initiate empty labels
+  labels <- list()
 
   # Find labels from every layer
   for (i in seq_along(layers)) {
@@ -65,7 +65,26 @@ setup_plot_labels <- function(plot, layers, data) {
       labels <- defaults(labels, current)
     }
   }
-  labels
+
+  # Warn for spurious labels that don't have a mapping.
+  # Note: sometimes, 'x' and 'y' might not have a mapping, like in
+  # `geom_function()`. We can display these labels anyway, so we include them.
+  plot_labels  <- plot$labels
+  known_labels <- c(names(labels), fn_fmls_names(labs), "x", "y")
+  extra_labels <- setdiff(names(plot_labels), known_labels)
+
+  if (length(extra_labels) > 0) {
+    extra_labels <- paste0(
+      "{.code ", extra_labels, " = \"", plot_labels[extra_labels], "\"}"
+    )
+    names(extra_labels) <- rep("*", length(extra_labels))
+    cli::cli_warn(c(
+      "Ignoring unknown labels:",
+      extra_labels
+    ))
+  }
+
+  defaults(plot_labels, labels)
 }
 
 #' Modify axis, legend, and plot labels
@@ -168,7 +187,7 @@ ggtitle <- function(label, subtitle = waiver()) {
 #' text from the information stored in the plot.
 #'
 #' @param p a ggplot object
-#' @param ... Currently ignored
+#' @inheritParams rlang::args_dots_used
 #'
 #' @return A text string
 #'
@@ -191,6 +210,7 @@ ggtitle <- function(label, subtitle = waiver()) {
 #' get_alt_text(p)
 #'
 get_alt_text <- function(p, ...) {
+  warn_dots_used()
   UseMethod("get_alt_text")
 }
 #' @export
diff --git a/R/utilities.R b/R/utilities.R
index 2585de5acc..56325e83d9 100644
--- a/R/utilities.R
+++ b/R/utilities.R
@@ -843,6 +843,18 @@ as_unordered_factor <- function(x) {
   x
 }
 
+warn_dots_used <- function(env = caller_env(), call = caller_env()) {
+  check_dots_used(
+    env = env, call = call,
+    # Demote from error to warning
+    error = function(cnd) {
+      # cli uses \f as newlines, not \n
+      msg <- gsub("\n", "\f", cnd_message(cnd))
+      cli::cli_warn(msg, call = call)
+    }
+  )
+}
+
 # Shim for scales/#424
 col_mix <- function(a, b, amount = 0.5) {
   input <- vec_recycle_common(a = a, b = b, amount = amount)
diff --git a/man/fortify.Rd b/man/fortify.Rd
index 59dcfea9ec..36d1837025 100644
--- a/man/fortify.Rd
+++ b/man/fortify.Rd
@@ -11,7 +11,7 @@ fortify(model, data, ...)
 
 \item{data}{original dataset, if needed}
 
-\item{...}{other arguments passed to methods}
+\item{...}{Arguments passed to methods.}
 }
 \description{
 Rather than using this function, I now recommend using the \pkg{broom}
diff --git a/man/get_alt_text.Rd b/man/get_alt_text.Rd
index b0da28a783..c9f6bdeb1e 100644
--- a/man/get_alt_text.Rd
+++ b/man/get_alt_text.Rd
@@ -10,7 +10,7 @@ get_alt_text(p, ...)
 \arguments{
 \item{p}{a ggplot object}
 
-\item{...}{Currently ignored}
+\item{...}{Arguments passed to methods.}
 }
 \value{
 A text string
diff --git a/tests/testthat/_snaps/guides.md b/tests/testthat/_snaps/guides.md
index 62d1e41d24..be7c84d7f3 100644
--- a/tests/testthat/_snaps/guides.md
+++ b/tests/testthat/_snaps/guides.md
@@ -1,3 +1,14 @@
+# dots are checked when making guides
+
+    Ignoring unknown argument to `guide_axis()`: `foo`.
+
+---
+
+    Arguments in `...` must be used.
+    x Problematic argument:
+    * foo = "bar"
+    i Did you misspell an argument name?
+
 # Using non-position guides for position scales results in an informative error
 
     `guide_legend()` cannot be used for x, xmin, xmax, or xend.
diff --git a/tests/testthat/_snaps/labels.md b/tests/testthat/_snaps/labels.md
index e1f6ed4140..49efe59a72 100644
--- a/tests/testthat/_snaps/labels.md
+++ b/tests/testthat/_snaps/labels.md
@@ -5,6 +5,18 @@
     Output
       [1] "A plot showing class on a discrete x-axis and count on a continuous y-axis using a bar layer."
 
+# get_alt_text checks dots
+
+    Arguments in `...` must be used.
+    x Problematic argument:
+    * foo = "bar"
+    i Did you misspell an argument name?
+
+# warnings are thrown for unknown labels
+
+    Ignoring unknown labels:
+    * `foo = "bar"`
+
 # plot.tag.position rejects invalid input
 
     The `plot.tag.position` theme element must be a <character/numeric/integer> object.
diff --git a/tests/testthat/_snaps/plot.md b/tests/testthat/_snaps/plot.md
index 6035364389..157ebb6635 100644
--- a/tests/testthat/_snaps/plot.md
+++ b/tests/testthat/_snaps/plot.md
@@ -8,6 +8,13 @@
     `data` cannot be a function.
     i Have you misspelled the `data` argument in `ggplot()`
 
+---
+
+    Arguments in `...` must be used.
+    x Problematic argument:
+    * foobar = "nonsense"
+    i Did you misspell an argument name?
+
 # construction have user friendly errors
 
     Cannot use `+` with a single argument.
diff --git a/tests/testthat/test-guides.R b/tests/testthat/test-guides.R
index f0057a7452..23df1f75e2 100644
--- a/tests/testthat/test-guides.R
+++ b/tests/testthat/test-guides.R
@@ -87,6 +87,15 @@ test_that("show.legend handles named vectors", {
   expect_equal(n_legends(p), 1)
 })
 
+test_that("dots are checked when making guides", {
+  expect_snapshot_warning(
+    new_guide(foo = "bar", super = GuideAxis)
+  )
+  expect_snapshot_warning(
+    guide_legend(foo = "bar")
+  )
+})
+
 test_that("axis_label_overlap_priority always returns the correct number of elements", {
   expect_identical(axis_label_priority(0), numeric(0))
   expect_setequal(axis_label_priority(1), seq_len(1))
diff --git a/tests/testthat/test-labels.R b/tests/testthat/test-labels.R
index c659e39cf5..60f5165c1b 100644
--- a/tests/testthat/test-labels.R
+++ b/tests/testthat/test-labels.R
@@ -104,6 +104,15 @@ test_that("alt text can take a function", {
   expect_snapshot(get_alt_text(p))
 })
 
+test_that("get_alt_text checks dots", {
+  expect_snapshot_warning(get_alt_text(ggplot(), foo = "bar"))
+})
+
+test_that("warnings are thrown for unknown labels", {
+  p <- ggplot(mtcars, aes(mpg, disp)) + geom_point() + labs(foo = 'bar')
+  expect_snapshot_warning(ggplot_build(p))
+})
+
 test_that("plot.tag.position rejects invalid input", {
   p <- ggplot(mtcars, aes(mpg, disp)) + geom_point() + labs(tag = "Fig. A)")
 
diff --git a/tests/testthat/test-plot.R b/tests/testthat/test-plot.R
index dc1b507c13..2cccf79034 100644
--- a/tests/testthat/test-plot.R
+++ b/tests/testthat/test-plot.R
@@ -1,6 +1,7 @@
 test_that("ggplot() throws informative errors", {
   expect_snapshot_error(ggplot(mapping = letters))
   expect_snapshot_error(ggplot(data))
+  expect_snapshot_warning(ggplot(foobar = "nonsense"))
 })
 
 test_that("construction have user friendly errors", {