From 5324933eafda152e4186c720fd3f0844761138ad Mon Sep 17 00:00:00 2001
From: Dewey Dunnington <dewey@fishandwhistle.net>
Date: Thu, 30 May 2019 13:16:54 -0400
Subject: [PATCH 1/8] add function to check for improper extract usage in aes()

---
 R/aes.r                   | 41 +++++++++++++++++++++++++++++++++++++++
 tests/testthat/test-aes.r | 31 +++++++++++++++++++++++++++++
 2 files changed, 72 insertions(+)

diff --git a/R/aes.r b/R/aes.r
index d5a3996049..2a71342b18 100644
--- a/R/aes.r
+++ b/R/aes.r
@@ -334,3 +334,44 @@ mapped_aesthetics <- function(x) {
   is_null <- vapply(x, is.null, logical(1))
   names(x)[!is_null]
 }
+
+check_aes_extract_usage <- function(mapping, data) {
+  lapply(mapping, check_aes_extract_usage_quo, data)
+  invisible(mapping)
+}
+
+check_aes_extract_usage_quo <- function(quosure, data) {
+  check_aes_extract_usage_expr(rlang::get_expr(quosure), data, rlang::get_env(quosure))
+}
+
+check_aes_extract_usage_expr <- function(x, data, env = emptyenv()) {
+  if (rlang::is_call(x, "[[") || rlang::is_call(x, "$")) {
+    data_eval <- eval_tidy(x[[2]], data, env)
+    if(rlang::is_reference(data_eval, data)) {
+      good_usage <- check_aes_get_alternative_usage(x)
+      warning(
+        "Use of `", format(x), "` is discouraged. ",
+        "Use `", good_usage,  "` instead.",
+        call. = FALSE
+      )
+    }
+  } else if (is.call(x)) {
+    lapply(x, check_aes_extract_usage_expr, data, env)
+  } else if (is.pairlist(x)) {
+    lapply(x, check_aes_extract_usage_expr, data, env)
+  }
+
+  invisible()
+}
+
+check_aes_get_alternative_usage <- function(x) {
+  if(rlang::is_call(x, "[[")) {
+    good_call <- x
+    good_call[[2]] <- quote(.data)
+    format(good_call)
+  } else if(rlang::is_call(x, "$")) {
+    as.character(x[[3]])
+  } else {
+    stop("Don't know how to get alternative usage for `", format(x), "`")
+  }
+}
diff --git a/tests/testthat/test-aes.r b/tests/testthat/test-aes.r
index be8bd96c51..303e0fc5a4 100644
--- a/tests/testthat/test-aes.r
+++ b/tests/testthat/test-aes.r
@@ -111,6 +111,37 @@ test_that("aes standardises aesthetic names", {
   expect_warning(aes(color = x, colour = y), "Duplicated aesthetics")
 })
 
+test_that("Improper use of $ is detected by check_aes_extract_usage()", {
+  check_aes_extract_usage <- ggplot2:::check_aes_extract_usage
+
+  returns_x <- function() "x"
+  df <- tibble::tibble(x = 1:5, nested_df = tibble::tibble(x = 6:10))
+
+  # valid extraction in aes()
+  expect_silent(check_aes_extract_usage(aes(x), df))
+  expect_silent(check_aes_extract_usage(aes(.data$x), df))
+  expect_silent(check_aes_extract_usage(aes(.data[["x"]]), df))
+  expect_silent(check_aes_extract_usage(aes(.data[[!!quo("x")]]), df))
+  expect_silent(check_aes_extract_usage(aes(.data[[returns_x()]]), df))
+  expect_silent(check_aes_extract_usage(aes(!!sym("x")), df))
+  expect_silent(check_aes_extract_usage(aes(x * 10), df))
+  expect_silent(check_aes_extract_usage(aes(nested_df$x), df))
+  expect_silent(check_aes_extract_usage(aes(nested_df[["x"]]), df))
+  expect_silent(check_aes_extract_usage(aes(.data[[c("nested_df", "x")]]), df))
+  expect_silent(check_aes_extract_usage(aes(.data[[c(2, 1)]]), df))
+  expect_silent(check_aes_extract_usage(aes(.data[[1]]), df))
+
+  # bad: use of extraction
+  expect_warning(
+    check_aes_extract_usage(aes(df$x), df),
+    "Use of `df\\$x` is discouraged"
+  )
+  expect_warning(
+    check_aes_extract_usage(aes(df[["x"]]), df),
+    'Use of `df\\[\\["x"\\]\\]` is discouraged'
+  )
+})
+
 
 # Visual tests ------------------------------------------------------------
 

From 8dcd9a2e09585eb6a302c495b6927fa8f0a27686 Mon Sep 17 00:00:00 2001
From: Dewey Dunnington <dewey@fishandwhistle.net>
Date: Thu, 30 May 2019 16:26:15 -0400
Subject: [PATCH 2/8] add function to find column references in a quosure

---
 NAMESPACE                 |  1 +
 R/aes.r                   | 71 ++++++++++++++++++++++++++++++++++++---
 tests/testthat/test-aes.r | 38 ++++++++++++++++++++-
 3 files changed, 104 insertions(+), 6 deletions(-)

diff --git a/NAMESPACE b/NAMESPACE
index 41405a260e..83f0d65759 100644
--- a/NAMESPACE
+++ b/NAMESPACE
@@ -579,6 +579,7 @@ import(gtable)
 import(rlang)
 import(scales)
 importFrom(lazyeval,f_eval)
+importFrom(rlang,is_call)
 importFrom(stats,setNames)
 importFrom(tibble,tibble)
 importFrom(utils,.DollarNames)
diff --git a/R/aes.r b/R/aes.r
index 2a71342b18..a4732f9003 100644
--- a/R/aes.r
+++ b/R/aes.r
@@ -335,19 +335,31 @@ mapped_aesthetics <- function(x) {
   names(x)[!is_null]
 }
 
+
 check_aes_extract_usage <- function(mapping, data) {
   lapply(mapping, check_aes_extract_usage_quo, data)
   invisible(mapping)
 }
 
+check_aes_column_refs <- function(mapping, data) {
+  data_name <- as_label(enquo(data))
+  cols_in_mapping <- unlist(lapply(mapping, quo_column_refs, data))
+
+  if (length(cols_in_mapping) == 0) {
+    warning("Mapping contains zero mapped columns from data", call. = FALSE)
+  }
+
+  invisible(mapping)
+}
+
 check_aes_extract_usage_quo <- function(quosure, data) {
   check_aes_extract_usage_expr(rlang::get_expr(quosure), data, rlang::get_env(quosure))
 }
 
+#' @importFrom rlang is_call
 check_aes_extract_usage_expr <- function(x, data, env = emptyenv()) {
-  if (rlang::is_call(x, "[[") || rlang::is_call(x, "$")) {
-    data_eval <- eval_tidy(x[[2]], data, env)
-    if(rlang::is_reference(data_eval, data)) {
+  if (is_call(x, "[[") || is_call(x, "$")) {
+    if(extract_target_is_data(x, data, env)) {
       good_usage <- check_aes_get_alternative_usage(x)
       warning(
         "Use of `", format(x), "` is discouraged. ",
@@ -365,13 +377,62 @@ check_aes_extract_usage_expr <- function(x, data, env = emptyenv()) {
 }
 
 check_aes_get_alternative_usage <- function(x) {
-  if(rlang::is_call(x, "[[")) {
+  if (is_call(x, "[[")) {
     good_call <- x
     good_call[[2]] <- quote(.data)
     format(good_call)
-  } else if(rlang::is_call(x, "$")) {
+  } else if (is_call(x, "$")) {
     as.character(x[[3]])
   } else {
     stop("Don't know how to get alternative usage for `", format(x), "`")
   }
 }
+
+quo_column_refs <- function(quosure, data) {
+  expr_column_refs(rlang::get_expr(quosure), data, rlang::get_env(quosure))
+}
+
+expr_column_refs <- function(x, data, env = emptyenv()) {
+  if (is.name(x) && (as.character(x) %in% names(data))) {
+    as.character(x)
+  } else if (is_call(x, "[[") && extract_target_is_quo_data(x, data, env)) {
+    # in extract calls from .data, the index is not overscoped with the data
+    index_value <- try(eval_tidy(x[[3]], data = NULL, env), silent = TRUE)
+    if (inherits(index_value, "try-error")) {
+      character(0)
+    } else {
+      column_ref_from_index(index_value, data)
+    }
+  } else if (is_call(x, "$") && extract_target_is_quo_data(x, data, env)) {
+    as.character(x[[3]])
+  } else if(is_call(x, "$")) {
+    expr_column_refs(x[[2]], data, env)
+  } else if (is.call(x)) {
+    new_names <- lapply(x, expr_column_refs, data, env)
+    unlist(new_names)
+  } else if (is.pairlist(x)) {
+    new_names <- lapply(x, expr_column_refs, data, env)
+    unlist(new_names)
+  } else {
+    character(0)
+  }
+}
+
+column_ref_from_index <- function(index, data) {
+  if (is.character(index)) {
+    index[1]
+  } else if (is.numeric(index)) {
+    names(data)[index[1]]
+  } else {
+    character(0)
+  }
+}
+
+extract_target_is_data <- function(x, data, env) {
+  data_eval <- try(eval_tidy(x[[2]], data, env), silent = TRUE)
+  rlang::is_reference(data_eval, data)
+}
+
+extract_target_is_quo_data <- function(x, data, env) {
+  identical(x[[2]], quote(.data)) || extract_target_is_data(x, data, env)
+}
diff --git a/tests/testthat/test-aes.r b/tests/testthat/test-aes.r
index 303e0fc5a4..4f50dc008f 100644
--- a/tests/testthat/test-aes.r
+++ b/tests/testthat/test-aes.r
@@ -112,7 +112,6 @@ test_that("aes standardises aesthetic names", {
 })
 
 test_that("Improper use of $ is detected by check_aes_extract_usage()", {
-  check_aes_extract_usage <- ggplot2:::check_aes_extract_usage
 
   returns_x <- function() "x"
   df <- tibble::tibble(x = 1:5, nested_df = tibble::tibble(x = 6:10))
@@ -142,6 +141,43 @@ test_that("Improper use of $ is detected by check_aes_extract_usage()", {
   )
 })
 
+test_that("Column names are correctly extracted from quosures", {
+
+  returns_x <- function() "x"
+  df <- tibble::tibble(x = 1:5, y = 12, nested_df = tibble::tibble(x = 6:10))
+  returns_df <- function() df
+  not_df <- tibble::tibble(x = 1:5)
+
+  # valid ways to map a column
+  expect_setequal(quo_column_refs(quo(x), df), "x")
+  expect_setequal(quo_column_refs(quo(x * y), df), c("x", "y"))
+  expect_setequal(quo_column_refs(quo(.data$x), df), "x")
+  expect_setequal(quo_column_refs(quo(.data[["x"]]), df), "x")
+  expect_setequal(quo_column_refs(quo(.data[[!!quo("x")]]), df), "x")
+  expect_setequal(quo_column_refs(quo(.data[[returns_x()]]), df), "x")
+  expect_setequal(quo_column_refs(quo(!!sym("x")), df), "x")
+  expect_setequal(quo_column_refs(quo(x * 10), df), "x")
+  expect_setequal(quo_column_refs(quo(nested_df$x), df), "nested_df")
+  expect_setequal(quo_column_refs(quo(nested_df[["x"]]), df), "nested_df")
+  expect_setequal(quo_column_refs(quo(.data[[c("nested_df", "x")]]), df), "nested_df")
+  expect_setequal(quo_column_refs(quo(.data[[c(3, 1)]]), df), "nested_df")
+  expect_setequal(quo_column_refs(quo(.data[[1]]), df), "x")
+
+  # spurious ways to map a column that don't currently fail
+  expect_setequal(quo_column_refs(quo(df$x), df), "x")
+  expect_setequal(quo_column_refs(quo(returns_df()$x), df), "x")
+  expect_setequal(quo_column_refs(quo(df[["x"]]), df), "x")
+
+  # no columns mapped
+  expect_identical(quo_column_refs(quo(), df), character(0))
+  expect_identical(quo_column_refs(quo(not_a_column), df), character(0))
+  expect_identical(quo_column_refs(quo(not_a_column * also_not_a_column), df), character(0))
+
+  # evaluation errors should result in zero mapped columns
+  expect_identical(quo_column_refs(quo(not_a_column$x), df), character(0))
+  expect_identical(quo_column_refs(quo(not_df$x), df), character(0))
+  expect_identical(quo_column_refs(quo(not_a_function()), df), character(0))
+})
 
 # Visual tests ------------------------------------------------------------
 

From a344721ab763da1467b8455f033d84e3a4903fc1 Mon Sep 17 00:00:00 2001
From: Dewey Dunnington <dewey@fishandwhistle.net>
Date: Thu, 30 May 2019 17:20:46 -0400
Subject: [PATCH 3/8] implement mapping checks in Layer

---
 R/aes.r                   | 26 ++++++++++++++++++--------
 R/layer.r                 |  6 +++++-
 tests/testthat/test-aes.r | 20 ++++++++++++++++----
 3 files changed, 39 insertions(+), 13 deletions(-)

diff --git a/R/aes.r b/R/aes.r
index a4732f9003..48c7ad54e9 100644
--- a/R/aes.r
+++ b/R/aes.r
@@ -336,20 +336,32 @@ mapped_aesthetics <- function(x) {
 }
 
 
+#' Check a mapping for discouraged usage
+#'
+#' @param mapping A mapping created with [aes()]
+#' @param data The data to be mapped from
+#'
+#' @noRd
+check_aes <- function(mapping, data) {
+  check_aes_extract_usage(mapping, data)
+  check_aes_column_refs(mapping, data)
+}
+
+# Checks that $ and [[ are not used when the target *is* the data
 check_aes_extract_usage <- function(mapping, data) {
   lapply(mapping, check_aes_extract_usage_quo, data)
-  invisible(mapping)
 }
 
+# Checks that mapping refers to at least one column in data
 check_aes_column_refs <- function(mapping, data) {
+  if (empty(data) || length(mapping) == 0) return()
+
   data_name <- as_label(enquo(data))
   cols_in_mapping <- unlist(lapply(mapping, quo_column_refs, data))
 
   if (length(cols_in_mapping) == 0) {
     warning("Mapping contains zero mapped columns from data", call. = FALSE)
   }
-
-  invisible(mapping)
 }
 
 check_aes_extract_usage_quo <- function(quosure, data) {
@@ -359,7 +371,7 @@ check_aes_extract_usage_quo <- function(quosure, data) {
 #' @importFrom rlang is_call
 check_aes_extract_usage_expr <- function(x, data, env = emptyenv()) {
   if (is_call(x, "[[") || is_call(x, "$")) {
-    if(extract_target_is_data(x, data, env)) {
+    if (extract_target_is_data(x, data, env)) {
       good_usage <- check_aes_get_alternative_usage(x)
       warning(
         "Use of `", format(x), "` is discouraged. ",
@@ -372,8 +384,6 @@ check_aes_extract_usage_expr <- function(x, data, env = emptyenv()) {
   } else if (is.pairlist(x)) {
     lapply(x, check_aes_extract_usage_expr, data, env)
   }
-
-  invisible()
 }
 
 check_aes_get_alternative_usage <- function(x) {
@@ -405,7 +415,7 @@ expr_column_refs <- function(x, data, env = emptyenv()) {
     }
   } else if (is_call(x, "$") && extract_target_is_quo_data(x, data, env)) {
     as.character(x[[3]])
-  } else if(is_call(x, "$")) {
+  } else if (is_call(x, "$")) {
     expr_column_refs(x[[2]], data, env)
   } else if (is.call(x)) {
     new_names <- lapply(x, expr_column_refs, data, env)
@@ -430,7 +440,7 @@ column_ref_from_index <- function(index, data) {
 
 extract_target_is_data <- function(x, data, env) {
   data_eval <- try(eval_tidy(x[[2]], data, env), silent = TRUE)
-  rlang::is_reference(data_eval, data)
+  identical(data_eval, data)
 }
 
 extract_target_is_quo_data <- function(x, data, env) {
diff --git a/R/layer.r b/R/layer.r
index f0427c16eb..816e303c16 100644
--- a/R/layer.r
+++ b/R/layer.r
@@ -238,10 +238,14 @@ Layer <- ggproto("Layer", NULL,
 
     scales_add_defaults(plot$scales, data, aesthetics, plot$plot_env)
 
-    # Evaluate and check aesthetics
+    # Evaluate aesthetics
     evaled <- lapply(aesthetics, eval_tidy, data = data)
     evaled <- compact(evaled)
 
+    # Check for discouraged usage in mapping
+    check_aes(aesthetics, data[setdiff(names(data), "PANEL")])
+
+    # Check aesthetic values
     nondata_cols <- check_nondata_cols(evaled)
     if (length(nondata_cols) > 0) {
       msg <- paste0(
diff --git a/tests/testthat/test-aes.r b/tests/testthat/test-aes.r
index 4f50dc008f..2b95f01efb 100644
--- a/tests/testthat/test-aes.r
+++ b/tests/testthat/test-aes.r
@@ -111,10 +111,10 @@ test_that("aes standardises aesthetic names", {
   expect_warning(aes(color = x, colour = y), "Duplicated aesthetics")
 })
 
-test_that("Improper use of $ is detected by check_aes_extract_usage()", {
+test_that("Improper use of $ and [[ is detected by check_aes_extract_usage()", {
 
   returns_x <- function() "x"
-  df <- tibble::tibble(x = 1:5, nested_df = tibble::tibble(x = 6:10))
+  df <- data_frame(x = 1:5, nested_df = data_frame(x = 6:10))
 
   # valid extraction in aes()
   expect_silent(check_aes_extract_usage(aes(x), df))
@@ -141,12 +141,18 @@ test_that("Improper use of $ is detected by check_aes_extract_usage()", {
   )
 })
 
+test_that("Warnings are issued for improper use of $ and [[ in plots", {
+  df <- data_frame(x = 1:3, y = 3:1)
+  p <- ggplot(df, aes(df$x, df$y)) + geom_point()
+  expect_warning(ggplot_build(p), "Use of `df\\$x` is discouraged")
+})
+
 test_that("Column names are correctly extracted from quosures", {
 
   returns_x <- function() "x"
-  df <- tibble::tibble(x = 1:5, y = 12, nested_df = tibble::tibble(x = 6:10))
+  df <- data_frame(x = 1:5, y = 12, nested_df = data_frame(x = 6:10))
   returns_df <- function() df
-  not_df <- tibble::tibble(x = 1:5)
+  not_df <- data_frame(x = 1:5)
 
   # valid ways to map a column
   expect_setequal(quo_column_refs(quo(x), df), "x")
@@ -179,6 +185,12 @@ test_that("Column names are correctly extracted from quosures", {
   expect_identical(quo_column_refs(quo(not_a_function()), df), character(0))
 })
 
+test_that("Warnings are issued when zero columns from data are mapped", {
+  df <- data_frame(x = 1:3, y = 3:1)
+  p <- ggplot(df, aes(x, y)) + geom_hline(aes(yintercept = 1.5))
+  expect_warning(ggplot_build(p), "zero mapped columns")
+})
+
 # Visual tests ------------------------------------------------------------
 
 test_that("aesthetics are drawn correctly", {

From cff8c40dbdadf1de56269fe66e1bb1915f417199 Mon Sep 17 00:00:00 2001
From: Dewey Dunnington <dewey@fishandwhistle.net>
Date: Thu, 30 May 2019 20:13:22 -0400
Subject: [PATCH 4/8] remove redundant rlang imports

---
 R/aes.r | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/R/aes.r b/R/aes.r
index 48c7ad54e9..1a2f5c7b06 100644
--- a/R/aes.r
+++ b/R/aes.r
@@ -365,10 +365,9 @@ check_aes_column_refs <- function(mapping, data) {
 }
 
 check_aes_extract_usage_quo <- function(quosure, data) {
-  check_aes_extract_usage_expr(rlang::get_expr(quosure), data, rlang::get_env(quosure))
+  check_aes_extract_usage_expr(get_expr(quosure), data, get_env(quosure))
 }
 
-#' @importFrom rlang is_call
 check_aes_extract_usage_expr <- function(x, data, env = emptyenv()) {
   if (is_call(x, "[[") || is_call(x, "$")) {
     if (extract_target_is_data(x, data, env)) {
@@ -399,7 +398,7 @@ check_aes_get_alternative_usage <- function(x) {
 }
 
 quo_column_refs <- function(quosure, data) {
-  expr_column_refs(rlang::get_expr(quosure), data, rlang::get_env(quosure))
+  expr_column_refs(get_expr(quosure), data, get_env(quosure))
 }
 
 expr_column_refs <- function(x, data, env = emptyenv()) {

From e00b8df29ce0712495baf6aee2a79b8a6aca3f32 Mon Sep 17 00:00:00 2001
From: Dewey Dunnington <dewey@fishandwhistle.net>
Date: Fri, 31 May 2019 08:04:10 -0400
Subject: [PATCH 5/8] finish removing redundant rlang imports

---
 NAMESPACE | 1 -
 1 file changed, 1 deletion(-)

diff --git a/NAMESPACE b/NAMESPACE
index 83f0d65759..41405a260e 100644
--- a/NAMESPACE
+++ b/NAMESPACE
@@ -579,7 +579,6 @@ import(gtable)
 import(rlang)
 import(scales)
 importFrom(lazyeval,f_eval)
-importFrom(rlang,is_call)
 importFrom(stats,setNames)
 importFrom(tibble,tibble)
 importFrom(utils,.DollarNames)

From be3ec015c96ea6db88696545df4194f01a1e1084 Mon Sep 17 00:00:00 2001
From: Dewey Dunnington <dewey@fishandwhistle.net>
Date: Mon, 3 Jun 2019 10:52:02 -0400
Subject: [PATCH 6/8] remove aes() no column reference warning, improve error
 message code for extract usage

---
 R/aes.r                   | 71 +++------------------------------------
 R/layer.r                 |  2 +-
 tests/testthat/test-aes.r | 46 +------------------------
 3 files changed, 6 insertions(+), 113 deletions(-)

diff --git a/R/aes.r b/R/aes.r
index 1a2f5c7b06..01f06dfa32 100644
--- a/R/aes.r
+++ b/R/aes.r
@@ -338,32 +338,16 @@ mapped_aesthetics <- function(x) {
 
 #' Check a mapping for discouraged usage
 #'
+#' Checks that `$` and `[[` are not used when the target *is* the data
+#'
 #' @param mapping A mapping created with [aes()]
 #' @param data The data to be mapped from
 #'
 #' @noRd
-check_aes <- function(mapping, data) {
-  check_aes_extract_usage(mapping, data)
-  check_aes_column_refs(mapping, data)
-}
-
-# Checks that $ and [[ are not used when the target *is* the data
 check_aes_extract_usage <- function(mapping, data) {
   lapply(mapping, check_aes_extract_usage_quo, data)
 }
 
-# Checks that mapping refers to at least one column in data
-check_aes_column_refs <- function(mapping, data) {
-  if (empty(data) || length(mapping) == 0) return()
-
-  data_name <- as_label(enquo(data))
-  cols_in_mapping <- unlist(lapply(mapping, quo_column_refs, data))
-
-  if (length(cols_in_mapping) == 0) {
-    warning("Mapping contains zero mapped columns from data", call. = FALSE)
-  }
-}
-
 check_aes_extract_usage_quo <- function(quosure, data) {
   check_aes_extract_usage_expr(get_expr(quosure), data, get_env(quosure))
 }
@@ -380,60 +364,17 @@ check_aes_extract_usage_expr <- function(x, data, env = emptyenv()) {
     }
   } else if (is.call(x)) {
     lapply(x, check_aes_extract_usage_expr, data, env)
-  } else if (is.pairlist(x)) {
-    lapply(x, check_aes_extract_usage_expr, data, env)
   }
 }
 
 check_aes_get_alternative_usage <- function(x) {
   if (is_call(x, "[[")) {
-    good_call <- x
-    good_call[[2]] <- quote(.data)
+    good_call <- call2("[[", quote(.data), x[[3]])
     format(good_call)
   } else if (is_call(x, "$")) {
     as.character(x[[3]])
   } else {
-    stop("Don't know how to get alternative usage for `", format(x), "`")
-  }
-}
-
-quo_column_refs <- function(quosure, data) {
-  expr_column_refs(get_expr(quosure), data, get_env(quosure))
-}
-
-expr_column_refs <- function(x, data, env = emptyenv()) {
-  if (is.name(x) && (as.character(x) %in% names(data))) {
-    as.character(x)
-  } else if (is_call(x, "[[") && extract_target_is_quo_data(x, data, env)) {
-    # in extract calls from .data, the index is not overscoped with the data
-    index_value <- try(eval_tidy(x[[3]], data = NULL, env), silent = TRUE)
-    if (inherits(index_value, "try-error")) {
-      character(0)
-    } else {
-      column_ref_from_index(index_value, data)
-    }
-  } else if (is_call(x, "$") && extract_target_is_quo_data(x, data, env)) {
-    as.character(x[[3]])
-  } else if (is_call(x, "$")) {
-    expr_column_refs(x[[2]], data, env)
-  } else if (is.call(x)) {
-    new_names <- lapply(x, expr_column_refs, data, env)
-    unlist(new_names)
-  } else if (is.pairlist(x)) {
-    new_names <- lapply(x, expr_column_refs, data, env)
-    unlist(new_names)
-  } else {
-    character(0)
-  }
-}
-
-column_ref_from_index <- function(index, data) {
-  if (is.character(index)) {
-    index[1]
-  } else if (is.numeric(index)) {
-    names(data)[index[1]]
-  } else {
-    character(0)
+    stop("Don't know how to get alternative usage for `", format(x), "`", call. = FALSE)
   }
 }
 
@@ -441,7 +382,3 @@ extract_target_is_data <- function(x, data, env) {
   data_eval <- try(eval_tidy(x[[2]], data, env), silent = TRUE)
   identical(data_eval, data)
 }
-
-extract_target_is_quo_data <- function(x, data, env) {
-  identical(x[[2]], quote(.data)) || extract_target_is_data(x, data, env)
-}
diff --git a/R/layer.r b/R/layer.r
index 816e303c16..6ff9dd900b 100644
--- a/R/layer.r
+++ b/R/layer.r
@@ -243,7 +243,7 @@ Layer <- ggproto("Layer", NULL,
     evaled <- compact(evaled)
 
     # Check for discouraged usage in mapping
-    check_aes(aesthetics, data[setdiff(names(data), "PANEL")])
+    check_aes_extract_usage(aesthetics, data[setdiff(names(data), "PANEL")])
 
     # Check aesthetic values
     nondata_cols <- check_nondata_cols(evaled)
diff --git a/tests/testthat/test-aes.r b/tests/testthat/test-aes.r
index 2b95f01efb..5b5e2ec1fe 100644
--- a/tests/testthat/test-aes.r
+++ b/tests/testthat/test-aes.r
@@ -141,56 +141,12 @@ test_that("Improper use of $ and [[ is detected by check_aes_extract_usage()", {
   )
 })
 
-test_that("Warnings are issued for improper use of $ and [[ in plots", {
+test_that("Warnings are issued for extract usage in plots", {
   df <- data_frame(x = 1:3, y = 3:1)
   p <- ggplot(df, aes(df$x, df$y)) + geom_point()
   expect_warning(ggplot_build(p), "Use of `df\\$x` is discouraged")
 })
 
-test_that("Column names are correctly extracted from quosures", {
-
-  returns_x <- function() "x"
-  df <- data_frame(x = 1:5, y = 12, nested_df = data_frame(x = 6:10))
-  returns_df <- function() df
-  not_df <- data_frame(x = 1:5)
-
-  # valid ways to map a column
-  expect_setequal(quo_column_refs(quo(x), df), "x")
-  expect_setequal(quo_column_refs(quo(x * y), df), c("x", "y"))
-  expect_setequal(quo_column_refs(quo(.data$x), df), "x")
-  expect_setequal(quo_column_refs(quo(.data[["x"]]), df), "x")
-  expect_setequal(quo_column_refs(quo(.data[[!!quo("x")]]), df), "x")
-  expect_setequal(quo_column_refs(quo(.data[[returns_x()]]), df), "x")
-  expect_setequal(quo_column_refs(quo(!!sym("x")), df), "x")
-  expect_setequal(quo_column_refs(quo(x * 10), df), "x")
-  expect_setequal(quo_column_refs(quo(nested_df$x), df), "nested_df")
-  expect_setequal(quo_column_refs(quo(nested_df[["x"]]), df), "nested_df")
-  expect_setequal(quo_column_refs(quo(.data[[c("nested_df", "x")]]), df), "nested_df")
-  expect_setequal(quo_column_refs(quo(.data[[c(3, 1)]]), df), "nested_df")
-  expect_setequal(quo_column_refs(quo(.data[[1]]), df), "x")
-
-  # spurious ways to map a column that don't currently fail
-  expect_setequal(quo_column_refs(quo(df$x), df), "x")
-  expect_setequal(quo_column_refs(quo(returns_df()$x), df), "x")
-  expect_setequal(quo_column_refs(quo(df[["x"]]), df), "x")
-
-  # no columns mapped
-  expect_identical(quo_column_refs(quo(), df), character(0))
-  expect_identical(quo_column_refs(quo(not_a_column), df), character(0))
-  expect_identical(quo_column_refs(quo(not_a_column * also_not_a_column), df), character(0))
-
-  # evaluation errors should result in zero mapped columns
-  expect_identical(quo_column_refs(quo(not_a_column$x), df), character(0))
-  expect_identical(quo_column_refs(quo(not_df$x), df), character(0))
-  expect_identical(quo_column_refs(quo(not_a_function()), df), character(0))
-})
-
-test_that("Warnings are issued when zero columns from data are mapped", {
-  df <- data_frame(x = 1:3, y = 3:1)
-  p <- ggplot(df, aes(x, y)) + geom_hline(aes(yintercept = 1.5))
-  expect_warning(ggplot_build(p), "zero mapped columns")
-})
-
 # Visual tests ------------------------------------------------------------
 
 test_that("aesthetics are drawn correctly", {

From 49973d43431e38dd0bb897b13b36f53bdb2e0e13 Mon Sep 17 00:00:00 2001
From: Dewey Dunnington <dewey@fishandwhistle.net>
Date: Tue, 4 Jun 2019 14:37:09 -0500
Subject: [PATCH 7/8] implement changes from review: better organized tests,
 improved argument documentation in aes(), improved the name of the warning
 functions, improved function detecting whether or not an expression refers to
 the plot data

---
 R/aes.r                   | 43 +++++++++++++++++++---------------
 R/layer.r                 |  2 +-
 man/aes.Rd                | 11 +++++----
 tests/testthat/test-aes.r | 49 +++++++++++++++++++++------------------
 4 files changed, 58 insertions(+), 47 deletions(-)

diff --git a/R/aes.r b/R/aes.r
index 01f06dfa32..f6a28b388c 100644
--- a/R/aes.r
+++ b/R/aes.r
@@ -8,8 +8,8 @@ NULL
 #' [ggplot2()] and in individual layers.
 #'
 #' This function also standardises aesthetic names by converting `color` to `colour`
-#' (also in substrings, e.g. `point_color` to `point_colour`) and translating old style
-#' R names to ggplot names (eg. `pch` to `shape`, `cex` to `size`).
+#' (also in substrings, e.g., `point_color` to `point_colour`) and translating old style
+#' R names to ggplot names (e.g., `pch` to `shape` and `cex` to `size`).
 #'
 #' @section Quasiquotation:
 #'
@@ -22,9 +22,10 @@ NULL
 #' programming vignette](http://dplyr.tidyverse.org/articles/programming.html)
 #' to learn more about these techniques.
 #'
-#' @param x,y,... List of name value pairs giving aesthetics to map to
-#'   variables. The names for x and y aesthetics are typically omitted because
-#'   they are so common; all other aesthetics must be named.
+#' @param x,y,... List of name-value pairs in the form `aesthetic = column_name`
+#'   describing which variables in the layer data should be mapped to which
+#'   aesthetics used by paired geom/stat. The names for x and y aesthetics are typically
+#'   omitted because they are so common; all other aesthetics must be named.
 #' @seealso [vars()] for another quoting function designed for
 #'   faceting specifications.
 #' @return A list with class `uneval`. Components of the list are either
@@ -344,18 +345,16 @@ mapped_aesthetics <- function(x) {
 #' @param data The data to be mapped from
 #'
 #' @noRd
-check_aes_extract_usage <- function(mapping, data) {
-  lapply(mapping, check_aes_extract_usage_quo, data)
-}
-
-check_aes_extract_usage_quo <- function(quosure, data) {
-  check_aes_extract_usage_expr(get_expr(quosure), data, get_env(quosure))
+warn_for_aes_extract_usage <- function(mapping, data) {
+  lapply(mapping, function(quosure) {
+    warn_for_aes_extract_usage_expr(get_expr(quosure), data, get_env(quosure))
+  })
 }
 
-check_aes_extract_usage_expr <- function(x, data, env = emptyenv()) {
+warn_for_aes_extract_usage_expr <- function(x, data, env = emptyenv()) {
   if (is_call(x, "[[") || is_call(x, "$")) {
-    if (extract_target_is_data(x, data, env)) {
-      good_usage <- check_aes_get_alternative_usage(x)
+    if (extract_target_is_likely_data(x, data, env)) {
+      good_usage <- alternative_aes_extract_usage(x)
       warning(
         "Use of `", format(x), "` is discouraged. ",
         "Use `", good_usage,  "` instead.",
@@ -363,11 +362,11 @@ check_aes_extract_usage_expr <- function(x, data, env = emptyenv()) {
       )
     }
   } else if (is.call(x)) {
-    lapply(x, check_aes_extract_usage_expr, data, env)
+    lapply(x, warn_for_aes_extract_usage_expr, data, env)
   }
 }
 
-check_aes_get_alternative_usage <- function(x) {
+alternative_aes_extract_usage <- function(x) {
   if (is_call(x, "[[")) {
     good_call <- call2("[[", quote(.data), x[[3]])
     format(good_call)
@@ -378,7 +377,13 @@ check_aes_get_alternative_usage <- function(x) {
   }
 }
 
-extract_target_is_data <- function(x, data, env) {
-  data_eval <- try(eval_tidy(x[[2]], data, env), silent = TRUE)
-  identical(data_eval, data)
+extract_target_is_likely_data <- function(x, data, env) {
+  if(!is.name(x[[2]])) {
+    return(FALSE)
+  }
+
+  tryCatch({
+    data_eval <- eval_tidy(x[[2]], data, env)
+    identical(data_eval, data)
+  }, error = function(err) FALSE)
 }
diff --git a/R/layer.r b/R/layer.r
index 6ff9dd900b..cf0cfd15fd 100644
--- a/R/layer.r
+++ b/R/layer.r
@@ -243,7 +243,7 @@ Layer <- ggproto("Layer", NULL,
     evaled <- compact(evaled)
 
     # Check for discouraged usage in mapping
-    check_aes_extract_usage(aesthetics, data[setdiff(names(data), "PANEL")])
+    warn_for_aes_extract_usage(aesthetics, data[setdiff(names(data), "PANEL")])
 
     # Check aesthetic values
     nondata_cols <- check_nondata_cols(evaled)
diff --git a/man/aes.Rd b/man/aes.Rd
index 0f207c4fef..8c087269cc 100644
--- a/man/aes.Rd
+++ b/man/aes.Rd
@@ -7,9 +7,10 @@
 aes(x, y, ...)
 }
 \arguments{
-\item{x, y, ...}{List of name value pairs giving aesthetics to map to
-variables. The names for x and y aesthetics are typically omitted because
-they are so common; all other aesthetics must be named.}
+\item{x, y, ...}{List of name-value pairs in the form \code{aesthetic = column_name}
+describing which variables in the layer data should be mapped to which
+aesthetics used by paired geom/stat. The names for x and y aesthetics are typically
+omitted because they are so common; all other aesthetics must be named.}
 }
 \value{
 A list with class \code{uneval}. Components of the list are either
@@ -22,8 +23,8 @@ properties (aesthetics) of geoms. Aesthetic mappings can be set in
 }
 \details{
 This function also standardises aesthetic names by converting \code{color} to \code{colour}
-(also in substrings, e.g. \code{point_color} to \code{point_colour}) and translating old style
-R names to ggplot names (eg. \code{pch} to \code{shape}, \code{cex} to \code{size}).
+(also in substrings, e.g., \code{point_color} to \code{point_colour}) and translating old style
+R names to ggplot names (e.g., \code{pch} to \code{shape} and \code{cex} to \code{size}).
 }
 \section{Quasiquotation}{
 
diff --git a/tests/testthat/test-aes.r b/tests/testthat/test-aes.r
index 5b5e2ec1fe..043ed8f641 100644
--- a/tests/testthat/test-aes.r
+++ b/tests/testthat/test-aes.r
@@ -111,39 +111,44 @@ test_that("aes standardises aesthetic names", {
   expect_warning(aes(color = x, colour = y), "Duplicated aesthetics")
 })
 
-test_that("Improper use of $ and [[ is detected by check_aes_extract_usage()", {
+test_that("warn_for_aes_extract_usage() warns for discouraged uses of $ and [[ within aes()", {
 
-  returns_x <- function() "x"
   df <- data_frame(x = 1:5, nested_df = data_frame(x = 6:10))
 
-  # valid extraction in aes()
-  expect_silent(check_aes_extract_usage(aes(x), df))
-  expect_silent(check_aes_extract_usage(aes(.data$x), df))
-  expect_silent(check_aes_extract_usage(aes(.data[["x"]]), df))
-  expect_silent(check_aes_extract_usage(aes(.data[[!!quo("x")]]), df))
-  expect_silent(check_aes_extract_usage(aes(.data[[returns_x()]]), df))
-  expect_silent(check_aes_extract_usage(aes(!!sym("x")), df))
-  expect_silent(check_aes_extract_usage(aes(x * 10), df))
-  expect_silent(check_aes_extract_usage(aes(nested_df$x), df))
-  expect_silent(check_aes_extract_usage(aes(nested_df[["x"]]), df))
-  expect_silent(check_aes_extract_usage(aes(.data[[c("nested_df", "x")]]), df))
-  expect_silent(check_aes_extract_usage(aes(.data[[c(2, 1)]]), df))
-  expect_silent(check_aes_extract_usage(aes(.data[[1]]), df))
-
-  # bad: use of extraction
   expect_warning(
-    check_aes_extract_usage(aes(df$x), df),
+    warn_for_aes_extract_usage(aes(df$x), df),
     "Use of `df\\$x` is discouraged"
   )
+
   expect_warning(
-    check_aes_extract_usage(aes(df[["x"]]), df),
+    warn_for_aes_extract_usage(aes(df[["x"]]), df),
     'Use of `df\\[\\["x"\\]\\]` is discouraged'
   )
 })
 
-test_that("Warnings are issued for extract usage in plots", {
-  df <- data_frame(x = 1:3, y = 3:1)
-  p <- ggplot(df, aes(df$x, df$y)) + geom_point()
+test_that("warn_for_aes_extract_usage() does not evaluate function calls", {
+  df <- data_frame(x = 1:5, nested_df = data_frame(x = 6:10))
+  returns_df <- function() df
+
+  expect_warning(warn_for_aes_extract_usage(aes(df$x), df))
+  expect_silent(warn_for_aes_extract_usage(aes(returns_df()$x), df))
+})
+
+test_that("warn_for_aes_extract_usage() does not warn for valid uses of $ and [[ within aes()", {
+  df <- data_frame(x = 1:5, nested_df = data_frame(x = 6:10))
+
+  # use of .data
+  expect_silent(warn_for_aes_extract_usage(aes(.data$x), df))
+  expect_silent(warn_for_aes_extract_usage(aes(.data[["x"]]), df))
+
+  # use of $ for a nested data frame column
+  expect_silent(warn_for_aes_extract_usage(aes(nested_df$x), df))
+  expect_silent(warn_for_aes_extract_usage(aes(nested_df[["x"]]), df))
+})
+
+test_that("Warnings are issued when plots use discouraged extract usage within aes()", {
+  df <- data_frame(x = 1:3, y = 1:3)
+  p <- ggplot(df, aes(df$x, y)) + geom_point()
   expect_warning(ggplot_build(p), "Use of `df\\$x` is discouraged")
 })
 

From 918103b100009f9d66598bb4b01c03e9eb7d94c4 Mon Sep 17 00:00:00 2001
From: Dewey Dunnington <dewey@fishandwhistle.net>
Date: Tue, 4 Jun 2019 18:14:51 -0500
Subject: [PATCH 8/8] fix tidy styling in extract_target_is_likey_data(),
 improve parameter documentation for aes()

---
 R/aes.r    | 11 +++++++----
 man/aes.Rd |  9 ++++++---
 2 files changed, 13 insertions(+), 7 deletions(-)

diff --git a/R/aes.r b/R/aes.r
index f6a28b388c..ef6f9dd602 100644
--- a/R/aes.r
+++ b/R/aes.r
@@ -22,10 +22,13 @@ NULL
 #' programming vignette](http://dplyr.tidyverse.org/articles/programming.html)
 #' to learn more about these techniques.
 #'
-#' @param x,y,... List of name-value pairs in the form `aesthetic = column_name`
+#' @param x,y,... List of name-value pairs in the form `aesthetic = variable`
 #'   describing which variables in the layer data should be mapped to which
-#'   aesthetics used by paired geom/stat. The names for x and y aesthetics are typically
-#'   omitted because they are so common; all other aesthetics must be named.
+#'   aesthetics used by the paired geom/stat. The expression `variable` is
+#'   evaluated within the layer data, so there is no need to refer to
+#'   the original dataset (i.e., use `ggplot(df, aes(variable))`
+#'   instead of `ggplot(df, aes(df$variable))`). The names for x and y aesthetics
+#'   are typically omitted because they are so common; all other aesthetics must be named.
 #' @seealso [vars()] for another quoting function designed for
 #'   faceting specifications.
 #' @return A list with class `uneval`. Components of the list are either
@@ -378,7 +381,7 @@ alternative_aes_extract_usage <- function(x) {
 }
 
 extract_target_is_likely_data <- function(x, data, env) {
-  if(!is.name(x[[2]])) {
+  if (!is.name(x[[2]])) {
     return(FALSE)
   }
 
diff --git a/man/aes.Rd b/man/aes.Rd
index 8c087269cc..5b1c80daab 100644
--- a/man/aes.Rd
+++ b/man/aes.Rd
@@ -7,10 +7,13 @@
 aes(x, y, ...)
 }
 \arguments{
-\item{x, y, ...}{List of name-value pairs in the form \code{aesthetic = column_name}
+\item{x, y, ...}{List of name-value pairs in the form \code{aesthetic = variable}
 describing which variables in the layer data should be mapped to which
-aesthetics used by paired geom/stat. The names for x and y aesthetics are typically
-omitted because they are so common; all other aesthetics must be named.}
+aesthetics used by the paired geom/stat. The expression \code{variable} is
+evaluated within the layer data, so there is no need to refer to
+the original dataset (i.e., use \code{ggplot(df, aes(variable))}
+instead of \code{ggplot(df, aes(df$variable))}). The names for x and y aesthetics
+are typically omitted because they are so common; all other aesthetics must be named.}
 }
 \value{
 A list with class \code{uneval}. Components of the list are either