From f56cf08a02c3b39a94573083dc1b2b1d94539f26 Mon Sep 17 00:00:00 2001
From: Hadley Wickham <h.wickham@gmail.com>
Date: Wed, 30 Mar 2022 11:31:48 -0500
Subject: [PATCH 1/5] Move tweak_links with other inherits files

---
 R/rd-inherit.R | 38 ++++++++++++++++++++++++++++++++++++++
 R/utils-rd.R   | 37 -------------------------------------
 2 files changed, 38 insertions(+), 37 deletions(-)

diff --git a/R/rd-inherit.R b/R/rd-inherit.R
index 391fbcff..9f12da98 100644
--- a/R/rd-inherit.R
+++ b/R/rd-inherit.R
@@ -358,6 +358,44 @@ find_field <- function(topic, field_name) {
   }
 }
 
+tweak_links <- function(x, package) {
+  tag <- attr(x, "Rd_tag")
+
+  if (is.list(x)) {
+    if (!is.null(tag) && tag == "\\link") {
+      opt <- attr(x, "Rd_option")
+      if (is.null(opt)) {
+        if (has_topic(x[[1]], package)) {
+          attr(x, "Rd_option") <- structure(package, Rd_tag = "TEXT")
+        }
+      } else {
+        if (is.character(opt) && length(opt) == 1 && substr(opt, 1, 1) == "=") {
+          topic <- substr(opt, 2, nchar(opt))
+
+          if (has_topic(topic, package)) {
+            file <- find_topic_in_package(package, topic)
+            attr(x, "Rd_option") <- structure(paste0(package, ":", file), Rd_tag = "TEXT")
+          }
+        } else if (grepl(":", opt)) {
+          # need to fix the link to point to a file
+          target <- str_split_fixed(opt, ":", n = 2)
+          file <- try_find_topic_in_package(
+            target[1],
+            target[2],
+            where = " in inherited text"
+          )
+          attr(x, "Rd_option") <- structure(paste0(target[1], ":", file), Rd_tag = "TEXT")
+        }
+      }
+    } else if (length(x) > 0) {
+      x[] <- map(x, tweak_links, package = package)
+    }
+  }
+
+  x
+}
+
+
 # Find info in Rd or topic ------------------------------------------------
 
 get_rd <- function(name, topics) {
diff --git a/R/utils-rd.R b/R/utils-rd.R
index 3931af73..d7cdc69c 100644
--- a/R/utils-rd.R
+++ b/R/utils-rd.R
@@ -73,43 +73,6 @@ rd2text <- function(x) {
   paste(chr, collapse = "")
 }
 
-tweak_links <- function(x, package) {
-  tag <- attr(x, "Rd_tag")
-
-  if (is.list(x)) {
-    if (!is.null(tag) && tag == "\\link") {
-      opt <- attr(x, "Rd_option")
-      if (is.null(opt)) {
-        if (has_topic(x[[1]], package)) {
-          attr(x, "Rd_option") <- structure(package, Rd_tag = "TEXT")
-        }
-      } else {
-        if (is.character(opt) && length(opt) == 1 && substr(opt, 1, 1) == "=") {
-          topic <- substr(opt, 2, nchar(opt))
-
-          if (has_topic(topic, package)) {
-            file <- find_topic_in_package(package, topic)
-            attr(x, "Rd_option") <- structure(paste0(package, ":", file), Rd_tag = "TEXT")
-          }
-        } else if (grepl(":", opt)) {
-          # need to fix the link to point to a file
-          target <- str_split_fixed(opt, ":", n = 2)
-          file <- try_find_topic_in_package(
-            target[1],
-            target[2],
-            where = " in inherited text"
-          )
-          attr(x, "Rd_option") <- structure(paste0(target[1], ":", file), Rd_tag = "TEXT")
-        }
-      }
-    } else if (length(x) > 0) {
-      x[] <- map(x, tweak_links, package = package)
-    }
-  }
-
-  x
-}
-
 # helpers -----------------------------------------------------------------
 
 parse_rd <- function(x) {

From aa003713b009499180ff5ba11f3d39fb58c12ccb Mon Sep 17 00:00:00 2001
From: Hadley Wickham <h.wickham@gmail.com>
Date: Thu, 31 Mar 2022 08:13:12 -0500
Subject: [PATCH 2/5] Minimise link tweaking

* Only do it for bare links
* Only do it for the bits of the Rd we're actually using
---
 R/rd-inherit.R                   | 71 +++++++++++++++-----------------
 R/utils-rd.R                     |  5 ---
 tests/testthat/test-rd-inherit.R | 33 +++++++++++++++
 tests/testthat/test-utils-rd.R   | 18 --------
 4 files changed, 66 insertions(+), 61 deletions(-)

diff --git a/R/rd-inherit.R b/R/rd-inherit.R
index 9f12da98..41abb288 100644
--- a/R/rd-inherit.R
+++ b/R/rd-inherit.R
@@ -239,24 +239,23 @@ find_params <- function(name, topics) {
   setNames(rep.int(params, reps), unlist(individual_names))
 }
 
-topic_params <- function(x) UseMethod("topic_params")
-topic_params.Rd <- function(x) {
-  arguments <- get_tags(x, "\\arguments")
-  if (length(arguments) != 1) {
-    return(list())
-  }
-  items <- get_tags(arguments[[1]], "\\item")
+topic_params <- function(x) {
+  if (inherits(x, "Rd")) {
+    arguments <- get_tags(x, "\\arguments")
+    if (length(arguments) != 1) {
+      return(list())
+    }
+    items <- get_tags(arguments[[1]], "\\item")
 
-  values <- map_chr(items, function(x) rd2text(x[[2]]))
-  params <- map_chr(items, function(x) rd2text(x[[1]]))
+    values <- map_chr(items, function(y) rd2text(y[[2]], attr(x, "package")))
+    params <- map_chr(items, function(y) rd2text(y[[1]], attr(x, "package")))
 
-  setNames(values, params)
-}
-topic_params.RoxyTopic <- function(x) {
-  x$get_value("param")
+    setNames(values, params)
+  } else {
+    x$get_value("param")
+  }
 }
 
-
 # Inherit sections --------------------------------------------------------
 
 inherit_sections <- function(topic, topics) {
@@ -310,8 +309,8 @@ find_sections <- function(topic) {
   if (inherits(topic, "Rd")) {
     tag <- get_tags(topic, "\\section")
 
-    titles <- map_chr(map(tag, 1), rd2text)
-    contents <- map_chr(map(tag, 2), rd2text)
+    titles <- map_chr(map(tag, 1), rd2text, package = attr(topic, "package"))
+    contents <- map_chr(map(tag, 2), rd2text, package = attr(topic, "package"))
 
     list(title = titles, content = contents)
   } else {
@@ -352,12 +351,19 @@ find_field <- function(topic, field_name) {
     value <- tag[[1]]
     attr(value, "Rd_tag") <- NULL
 
-    str_trim(rd2text(value))
+    str_trim(rd2text(value, attr(topic, "package")))
   } else {
     topic$get_value(field_name)
   }
 }
 
+rd2text <- function(x, package) {
+  x <- tweak_links(x, package)
+  chr <- as_character_rd(structure(x, class = "Rd"), deparse = TRUE)
+  paste(chr, collapse = "")
+}
+
+# Convert relative to absolute links
 tweak_links <- function(x, package) {
   tag <- attr(x, "Rd_tag")
 
@@ -368,23 +374,11 @@ tweak_links <- function(x, package) {
         if (has_topic(x[[1]], package)) {
           attr(x, "Rd_option") <- structure(package, Rd_tag = "TEXT")
         }
-      } else {
-        if (is.character(opt) && length(opt) == 1 && substr(opt, 1, 1) == "=") {
-          topic <- substr(opt, 2, nchar(opt))
-
-          if (has_topic(topic, package)) {
-            file <- find_topic_in_package(package, topic)
-            attr(x, "Rd_option") <- structure(paste0(package, ":", file), Rd_tag = "TEXT")
-          }
-        } else if (grepl(":", opt)) {
-          # need to fix the link to point to a file
-          target <- str_split_fixed(opt, ":", n = 2)
-          file <- try_find_topic_in_package(
-            target[1],
-            target[2],
-            where = " in inherited text"
-          )
-          attr(x, "Rd_option") <- structure(paste0(target[1], ":", file), Rd_tag = "TEXT")
+      } else if (is_string(opt) && substr(opt, 1, 1) == "=") {
+        topic <- substr(opt, 2, nchar(opt))
+
+        if (has_topic(topic, package)) {
+          attr(x, "Rd_option") <- structure(paste0(package, ":", topic), Rd_tag = "TEXT")
         }
       }
     } else if (length(x) > 0) {
@@ -405,7 +399,7 @@ get_rd <- function(name, topics) {
     pkg <- as.character(parsed[[2]])
     fun <- as.character(parsed[[3]])
 
-    tweak_links(get_rd_from_help(pkg, fun), package = pkg)
+    get_rd_from_help(pkg, fun)
   } else {
     # Current package
     rd_name <- topics$find_filename(name)
@@ -422,16 +416,17 @@ get_rd_from_help <- function(package, alias) {
     return()
   }
 
-  help <- eval(expr(help(!!alias, !!package)))
+  help <- utils::help((alias), (package))
   if (length(help) == 0) {
     warn(paste0("Can't find help topic '", alias, "' in '", package, "' package"))
     return()
   }
 
-  internal_f("utils", ".getHelpFile")(help)
+  out <- internal_f("utils", ".getHelpFile")(help)
+  attr(out, "package") <- package
+  out
 }
 
-
 # helpers -----------------------------------------------------------------
 
 # Returns matching parameter name in haystack
diff --git a/R/utils-rd.R b/R/utils-rd.R
index d7cdc69c..be167d61 100644
--- a/R/utils-rd.R
+++ b/R/utils-rd.R
@@ -68,11 +68,6 @@ get_tags <- function(rd, tag) {
   Filter(function(x) identical(attr(x, "Rd_tag"), tag), rd)
 }
 
-rd2text <- function(x) {
-  chr <- as_character_rd(structure(x, class = "Rd"), deparse = TRUE)
-  paste(chr, collapse = "")
-}
-
 # helpers -----------------------------------------------------------------
 
 parse_rd <- function(x) {
diff --git a/tests/testthat/test-rd-inherit.R b/tests/testthat/test-rd-inherit.R
index 1421b02e..cc1179e9 100644
--- a/tests/testthat/test-rd-inherit.R
+++ b/tests/testthat/test-rd-inherit.R
@@ -26,6 +26,39 @@ test_that("\\links are transformed", {
   expect_snapshot_output(out$get_section("param"))
 })
 
+test_that("href doesn't get extra parens", {
+  expect_equal(rd2text(parse_rd("\\href{a}{b}")), "\\href{a}{b}\n")
+})
+
+test_that("ifelse doesn't get extra parens", {
+  expect_equal(rd2text(parse_rd("\\ifelse{a}{b}{c}")), "\\ifelse{a}{b}{c}\n")
+})
+
+test_that("relative links converted to absolute", {
+  link_to_base <- function(x) {
+    rd2text(parse_rd(x), package = "base")
+  }
+
+  expect_equal(
+    link_to_base("\\link{abbreviate}"),
+    "\\link[base]{abbreviate}\n"
+  )
+  expect_equal(
+    link_to_base("\\link[=abbreviate]{abbr}"),
+    "\\link[base:abbreviate]{abbr}\n"
+  )
+
+  # Doesn't affect links that already have
+  expect_equal(
+    link_to_base("\\link[foo]{abbreviate}"),
+    "\\link[foo]{abbreviate}\n"
+  )
+  expect_equal(
+    link_to_base("\\link[foo::abbreviate]{abbr}"),
+    "\\link[foo::abbreviate]{abbr}\n"
+  )
+})
+
 # tag parsing -------------------------------------------------------------
 
 test_that("warns on unknown inherit type", {
diff --git a/tests/testthat/test-utils-rd.R b/tests/testthat/test-utils-rd.R
index 753eaf9c..865cd5c0 100644
--- a/tests/testthat/test-utils-rd.R
+++ b/tests/testthat/test-utils-rd.R
@@ -1,24 +1,6 @@
-test_that("href doesn't get extra parens", {
-  expect_equal(rd2text(parse_rd("\\href{a}{b}")), "\\href{a}{b}\n")
-})
-
-test_that("ifelse doesn't get extra parens", {
-  expect_equal(rd2text(parse_rd("\\ifelse{a}{b}{c}")), "\\ifelse{a}{b}{c}\n")
-})
-
 test_that("has_topic() works as you'd expect", {
   expect_equal(has_topic("abbreviate", "base"), TRUE)
   expect_equal(has_topic("abbreviateXXXX", "base"), FALSE)
 
   expect_equal(has_topic("foo", "PACKAGEDOESNOTEXIST"), FALSE)
 })
-
-test_that("can tweak links to point to new package", {
-  rd1 <- tweak_links(parse_rd("\\link{abbreviate}"), package = "base")
-  rd2 <- tweak_links(parse_rd("\\link[base]{abbreviate}"), package = "base")
-  rd3 <- tweak_links(parse_rd("\\link[=abbreviate]{abbr}"), package = "base")
-
-  expect_equal(rd2text(rd1), "\\link[base]{abbreviate}\n")
-  expect_equal(rd2text(rd2), "\\link[base]{abbreviate}\n")
-  expect_equal(rd2text(rd3), "\\link[base:abbreviate]{abbr}\n")
-})

From e8735fe9a293ce002e4c0bf6003fed4cc3bdf052 Mon Sep 17 00:00:00 2001
From: Hadley Wickham <h.wickham@gmail.com>
Date: Thu, 31 Mar 2022 08:40:39 -0500
Subject: [PATCH 3/5] Improve warning messages

---
 NEWS.md                             |  4 ++
 R/rd-inherit.R                      | 59 ++++++++++++++++++++---------
 tests/testthat/_snaps/rd-inherit.md | 37 ++++++++++++++++++
 tests/testthat/test-rd-inherit.R    | 24 ++++++++++--
 4 files changed, 103 insertions(+), 21 deletions(-)

diff --git a/NEWS.md b/NEWS.md
index 2e2c75d2..615a22c5 100644
--- a/NEWS.md
+++ b/NEWS.md
@@ -1,5 +1,9 @@
 # roxygen2 (development version)
 
+* `@inherit` and friends perform less aggressive link tweaking, eliminating
+  many spurious warnings. Additionally, when you do get a warning, you'll 
+  now always learn which topic it's coming from (#1135).
+
 * Add support for inheriting 'note' fields via `@inherit pkg::fun note` (@pat-s, #1218)
 
 * Problems with the first tag in each block are now reported with the
diff --git a/R/rd-inherit.R b/R/rd-inherit.R
index 41abb288..a1f26c4b 100644
--- a/R/rd-inherit.R
+++ b/R/rd-inherit.R
@@ -133,15 +133,18 @@ inherit_params <- function(topic, topics) {
   needed <- topic$get_value("formals")
   missing <- setdiff(needed, documented)
   if (length(missing) == 0) {
-    warn(paste0(
-      "Topic '", topic$get_name(), "': ",
-      "no parameters to inherit with @inheritParams"
-    ))
+    cli::cli_warn(
+      c(
+        "@inheritParams failed in topic {.str {topic$get_name()}}.",
+        x = "No parameters remain to be inherited from with @inheritParams."
+      ),
+      call = NULL
+    )
     return()
   }
 
   for (inheritor in inheritors) {
-    inherited <- find_params(inheritor, topics)
+    inherited <- find_params(inheritor, topics, source = topic$get_name())
 
     matches <- map_chr(missing, match_param, names(inherited))
     new_match <- !is.na(matches)
@@ -219,8 +222,8 @@ get_documented_params <- function(topic, only_first = FALSE) {
   documented
 }
 
-find_params <- function(name, topics) {
-  topic <- get_rd(name, topics)
+find_params <- function(name, topics, source) {
+  topic <- get_rd(name, topics, source = source)
   if (is.null(topic)) {
     return()
   }
@@ -293,10 +296,14 @@ inherit_section <- function(topic, topics) {
     selected <- new_section$title %in% titles[[i]]
 
     if (sum(selected) != 1) {
-      warning(
-        "Can't find section '", titles[[i]], "' in ?",
-        sources[[i]], call. = FALSE
+      cli::cli_warn(
+        c(
+          "@inheritSection failed in topic {.str {topic$get_name()}}.",
+          x = "Can't find section {.str {titles[[i]]}} in topic {sources[[i]]}."
+        ),
+        call = NULL
       )
+      return()
     }
 
     topic$add(
@@ -328,7 +335,7 @@ inherit_field <- function(topic, topics, rd_name, roxy_name = rd_name) {
 
   # Otherwise, try each try function listed in inherits
   for (inherit_from in topic$inherits_from(roxy_name)) {
-    inherit_topic <- get_rd(inherit_from, topics)
+    inherit_topic <- get_rd(inherit_from, topics, source = topic$get_name())
     if (is.null(inherit_topic)) {
       next
     }
@@ -392,33 +399,51 @@ tweak_links <- function(x, package) {
 
 # Find info in Rd or topic ------------------------------------------------
 
-get_rd <- function(name, topics) {
+get_rd <- function(name, topics, source) {
   if (has_colons(name)) {
     # External package
     parsed <- parse_expr(name)
     pkg <- as.character(parsed[[2]])
     fun <- as.character(parsed[[3]])
 
-    get_rd_from_help(pkg, fun)
+    get_rd_from_help(pkg, fun, source)
   } else {
     # Current package
     rd_name <- topics$find_filename(name)
     if (identical(rd_name, NA_character_)) {
-      warn(paste0("Can't find help topic '", name, "' in current package"))
+      cli::cli_warn(
+        c(
+          "@inherits failed in topic {.str {source}}.",
+          x = "Can't find topic {.str {name}}."
+        ),
+        call = NULL
+      )
     }
     topics$get(rd_name)
   }
 }
 
-get_rd_from_help <- function(package, alias) {
+get_rd_from_help <- function(package, alias, source) {
   if (!is_installed(package)) {
-    warn(paste0("Can't find package '", package, "'"))
+    cli::cli_warn(
+      c(
+        "@inherits failed in topic {.str {source}}.",
+        x = "Package {package} is not installed."
+      ),
+      call = NULL
+    )
     return()
   }
 
   help <- utils::help((alias), (package))
   if (length(help) == 0) {
-    warn(paste0("Can't find help topic '", alias, "' in '", package, "' package"))
+    cli::cli_warn(
+      c(
+        "@inherits failed in topic {.str {source}}.",
+        x = "Can't find topic {package}::{alias}."
+      ),
+      call = NULL
+    )
     return()
   }
 
diff --git a/tests/testthat/_snaps/rd-inherit.md b/tests/testthat/_snaps/rd-inherit.md
index 4a6035e6..125efe16 100644
--- a/tests/testthat/_snaps/rd-inherit.md
+++ b/tests/testthat/_snaps/rd-inherit.md
@@ -5,6 +5,16 @@
     "sha1"}
     } 
 
+# warns if can't find section
+
+    @inheritSection failed in topic "b".
+    x Can't find section "A" in topic a.
+
+# warned if no params need documentation
+
+    @inheritParams failed in topic "x".
+    x No parameters remain to be inherited from with @inheritParams.
+
 # can inherit all from single function
 
     [1] "test-rd-inherit-dots.txt"
@@ -25,3 +35,30 @@
       }}
     } 
 
+# useful warnings if can't find topics
+
+    Code
+      get_rd("base2::attach", source = "source")
+    Condition
+      Warning:
+      @inherits failed in topic "source".
+      x Package base2 is not installed.
+    Output
+      NULL
+    Code
+      get_rd("base::function_not_found", source = "source")
+    Condition
+      Warning:
+      @inherits failed in topic "source".
+      x Can't find topic base::function_not_found.
+    Output
+      NULL
+    Code
+      get_rd("function", RoxyTopics$new(), source = "source")
+    Condition
+      Warning:
+      @inherits failed in topic "source".
+      x Can't find topic "function".
+    Output
+      NULL
+
diff --git a/tests/testthat/test-rd-inherit.R b/tests/testthat/test-rd-inherit.R
index cc1179e9..3281dfb3 100644
--- a/tests/testthat/test-rd-inherit.R
+++ b/tests/testthat/test-rd-inherit.R
@@ -268,6 +268,20 @@ test_that("can inherit single section", {
   expect_equal(section$content, "1")
 })
 
+
+test_that("warns if can't find section", {
+  code <- "
+    #' a
+    a <- function(x) {}
+
+    #' b
+    #'
+    #' @inheritSection a A
+    b <- function(y) {}
+  "
+  expect_snapshot_warning(roc_proc_text(rd_roclet(), code))
+})
+
 # Inherit parameters ------------------------------------------------------
 
 test_that("multiple @inheritParam tags gathers all params", {
@@ -395,7 +409,7 @@ test_that("warned if no params need documentation", {
     #' @inheritParams foo
     x <- function(x, y) {}
   "
-  expect_warning(roc_proc_text(rd_roclet(), code), "no parameters to inherit")
+  expect_snapshot_warning(roc_proc_text(rd_roclet(), code))
 })
 
 test_that("argument order, also for incomplete documentation", {
@@ -631,9 +645,11 @@ test_that("can inherit all from single function", {
 # get_rd() -----------------------------------------------------------------
 
 test_that("useful warnings if can't find topics", {
-  expect_warning(get_rd("base2::attach"), "Can't find package")
-  expect_warning(get_rd("base::function_not_found"), "Can't find help topic")
-  expect_warning(get_rd("function", RoxyTopics$new()), "Can't find help topic")
+  expect_snapshot({
+    get_rd("base2::attach", source = "source")
+    get_rd("base::function_not_found", source = "source")
+    get_rd("function", RoxyTopics$new(), source = "source")
+  })
 })
 
 test_that("can find section in existing docs", {

From 1f9105ee19b5dd437cd2b5ddae06081c28d805fc Mon Sep 17 00:00:00 2001
From: Hadley Wickham <h.wickham@gmail.com>
Date: Thu, 31 Mar 2022 08:43:27 -0500
Subject: [PATCH 4/5] Needs cli

---
 DESCRIPTION | 1 +
 1 file changed, 1 insertion(+)

diff --git a/DESCRIPTION b/DESCRIPTION
index 9d5d4719..cb471eaa 100644
--- a/DESCRIPTION
+++ b/DESCRIPTION
@@ -21,6 +21,7 @@ Depends:
     R (>= 3.3)
 Imports: 
     brew,
+    cli,
     commonmark,
     desc (>= 1.2.0),
     digest,

From 649bb922344505c0cb1bbd05ac4a0b46f464a3f4 Mon Sep 17 00:00:00 2001
From: Hadley Wickham <h.wickham@gmail.com>
Date: Thu, 31 Mar 2022 08:44:25 -0500
Subject: [PATCH 5/5] Tweak warning

---
 R/rd-inherit.R                      | 2 +-
 tests/testthat/_snaps/rd-inherit.md | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/R/rd-inherit.R b/R/rd-inherit.R
index a1f26c4b..7c00be48 100644
--- a/R/rd-inherit.R
+++ b/R/rd-inherit.R
@@ -136,7 +136,7 @@ inherit_params <- function(topic, topics) {
     cli::cli_warn(
       c(
         "@inheritParams failed in topic {.str {topic$get_name()}}.",
-        x = "No parameters remain to be inherited from with @inheritParams."
+        x = "All parameteres are already documented; none remain to be inherited."
       ),
       call = NULL
     )
diff --git a/tests/testthat/_snaps/rd-inherit.md b/tests/testthat/_snaps/rd-inherit.md
index 125efe16..22dd3548 100644
--- a/tests/testthat/_snaps/rd-inherit.md
+++ b/tests/testthat/_snaps/rd-inherit.md
@@ -13,7 +13,7 @@
 # warned if no params need documentation
 
     @inheritParams failed in topic "x".
-    x No parameters remain to be inherited from with @inheritParams.
+    x All parameteres are already documented; none remain to be inherited.
 
 # can inherit all from single function