S7 method documentation links

[mduncans]

Hello, I'm really happy to see the new S7 support!

I have a package using S7 classes where I am trying to document some methods separately from the generic. I followed the new rd-S7 vignette and added this to my generic's roxygen comments:

#' Methods are available for the following classes:
#'
#' `r doclisting::methods_list("add_spec_columns")`

Running devtools::document() generated the following Rd content:

Methods are available for the following classes:

\itemize{
  \item \code{hyperion.tables::SummarySpec}
  \item \code{hyperion.tables::TableSpec}
}

so there was no link to the method's specific documentation. This also generated some warnings:

Warning messages:
1: In path.expand(path) : restarting interrupted promise evaluation

With Claude's help, I was able to trace this to pkgload:::dev_topic_parse. The doclisting::methods_list() call generates a method alias of the form "add_spec_columns,hyperion.tables::TableSpec-method", which gets parsed as:

> pkgload:::dev_topic_parse("add_spec_columns,hyperion.tables::TableSpec-method")
$topic
[1] "TableSpec-method"

$pkg_names
[1] "add_spec_columns,hyperion.tables"

It looks like dev_topic_parse is splitting on :: and treating the first piece as a package name. For the S7 method aliases in my case, the class name itself is fully qualified (pkg::Class), so :: appears embedded inside the topic. The resulting pkg_names doesn't correspond to a real package, which I think is what cascades into the warnings I was seeing, and ultimately to doclisting not finding a help topic.

Locally I patched dev_topic_parse to only split on :: when the prefix is
actually a loaded dev package:

dev_topic_parse <- function(topic, dev_packages = NULL) {
  stopifnot(is_string(topic))

  pkgs <- dev_packages %||% dev_packages()

  pieces <- strsplit(topic, ":::?")[[1]]
  # Only treat as `pkg::topic` when the prefix is a loaded dev package.
  # Otherwise `::` is part of the topic itself (e.g., S7/S4 method aliases
  # like "generic,pkg::Class-method" where `pkg::Class` is the class name).
  if (length(pieces) > 1 && pieces[1] %in% pkgs) {
    pkgs <- pieces[1]
    topic <- pieces[2]
  }

  list(
    topic = topic,
    pkg_names = pkgs
  )
}

With that change the generated Rd correctly contains links:

Methods are available for the following classes:

\itemize{
  \item \code{\link[=add_spec_columns-hyperion.tables-SummarySpec-method]{hyperion.tables::SummarySpec}}
  \item \code{\link[=add_spec_columns-hyperion.tables-TableSpec-method]{hyperion.tables::TableSpec}}
}

Let me know if you'd like a reprex or to move this issue to a different package.

[hadley]

Is calling doclisting::methods_list("add_spec_columns") at the console sufficient to generate the error? I'm having a bit of trouble understanding where dev_topic_parse() comes in as neither roxygen2 nor doclisting uses it? Does it work on if the package is installed (i.e. all though R's regular help not devtools?)

The problem is that I might not have fully thought through the alias names since :: is a special symbol.

[hadley]

I should also say thanks for trying out a full end to end application, I really appreciate it! I'm mostly on vacation now, so any extra detail you can supply will be super helpful for when I get back to work.

[mduncans]

Thanks Hadley, enjoy the vacation!

For an installed package:

> library(hyperion.tables)
> doclisting::methods_list("add_spec_columns")
[1] "\\itemize{\n  \\item \\code{\\link[=add_spec_columns-hyperion.tables-SummarySpec-method]{hyperion.tables::SummarySpec}}\n  \\item \\code{\\link[=add_spec_columns-hyperion.tables-TableSpec-method]{hyperion.tables::TableSpec}}\n}"

No warnings and clearly a link is there. However, once devtools or roxygen::roxygenize() is called and loads the package I start getting the warnings. All of which are the same.

> devtools::document()
ℹ Updating hyperion.tables documentation
ℹ Loading hyperion.tables
There were 50 or more warnings (use warnings() to see the first 50)
> doclisting::methods_list("add_spec_columns")
[1] "\\itemize{\n  \\item \\code{hyperion.tables::SummarySpec}\n  \\item \\code{hyperion.tables::TableSpec}\n}"
Warning messages:
1: In path.expand(path) : restarting interrupted promise evaluation
2: In path.expand(path) : restarting interrupted promise evaluation
3: In path.expand(path) : restarting interrupted promise evaluation
4: In path.expand(path) : restarting interrupted promise evaluation

If I catch this warning to get a stack trace I see a call to pkgload::dev_topic_find ( 17. below) which immediately calls dev_topic_parse(topic, dev_packages)

> withCallingHandlers(
+     doclisting::methods_list("add_spec_columns"),
+     warning = function(w) {
+         cat("=== WARNING ===\n")
+         print(w)
+         
+         cat("\n=== STACK TRACE ===\n")
+         print(rlang::trace_back(bottom = sys.frame(-1)))
+         
+         browser()
+     }
+ )
=== WARNING ===
<simpleWarning in path.expand(path): restarting interrupted promise evaluation>

=== STACK TRACE ===
     ▆
  1. ├─base::withCallingHandlers(...)
  2. ├─doclisting::methods_list("add_spec_columns")
  3. │ └─doclisting:::methods_prep(x, package)
  4. │   └─doclisting:::methods_find(x)
  5. │     └─doclisting:::methods_find_s7(match.fun(x), x)
  6. │       └─doclisting:::help_topic(method, package)
  7. │         └─doclisting:::help_path(x, package)
  8. │           └─base::mapply(locate_help_doc, x, package, SIMPLIFY = FALSE)
  9. │             └─doclisting (local) `<fn>`(dots[[1L]][[1L]], dots[[2L]][[1L]])
 10. │               └─doclisting (local) help(x, (package))
 11. │                 ├─base::tryCatch(expr = shim_help(x, (package)), error = function(e) character())
 12. │                 │ └─base (local) tryCatchList(expr, classes, parentenv, handlers)
 13. │                 │   └─base (local) tryCatchOne(expr, names, parentenv, handlers[[1L]])
 14. │                 │     └─base (local) doTryCatch(return(expr), name, parentenv, handler)
 15. │                 └─pkgload:::shim_help(x, (package))
 16. │                   └─pkgload::dev_help(topic_str, package_str)
 17. │                     └─pkgload::dev_topic_find(topic, dev_packages)
 18. │                       └─pkgload:::dev_topic_path(topic$topic, path = ns_path(pkg_name))
 19. │                         └─pkgload::pkg_path(path)
 20. │                           └─base::tryCatch(...)
 21. │                             └─base (local) tryCatchList(expr, classes, parentenv, handlers)
 22. │                               └─base (local) tryCatchOne(expr, names, parentenv, handlers[[1L]])
 23. │                                 └─value[[3L]](cond)
 24. │                                   ├─cli::cli_abort(msg, class = "pkgload_no_desc")
 25. │                                   │ └─cli:::vcapply(message, format_inline, .envir = .envir)
 26. │                                   │   └─base::vapply(X, FUN, FUN.VALUE = character(1), ..., USE.NAMES = USE.NAMES)
 27. │                                   │     └─cli (local) FUN(X[[i]], ...)
 28. │                                   │       ├─cli::cli_fmt(...)
 29. │                                   │       │ └─cli:::cli__rec(expr)
 30. │                                   │       └─cli (local) fun(..., .envir = .envir)
 31. │                                   │         ├─cli:::cli__message(...)
 32. │                                   │         │ └─"id" %in% names(args)
 33. │                                   │         └─cli:::glue_cmd(..., .envir = .envir, .call = sys.call(), .trim = FALSE)
 34. │                                   │           └─cli:::glue(...)
 35. │                                   └─base::normalizePath(path)
 36. │                                     └─base::path.expand(path)
 37. ├─cli (local) `<fn>`(".path {normalizePath(path)}")
 38. │ ├─.transformer(expr, .envir) %||% character()
 39. │ └─cli (local) .transformer(expr, .envir)
 40. │   └─cli:::glue(...)
 41. ├─cli (local) `<fn>`("normalizePath(path)")
 42. │ ├─.transformer(expr, .envir) %||% character()
 43. │ └─cli (local) .transformer(expr, .envir)
 44. │   ├─eval(expr, envir = envir) %??% ...
 45. │   │ └─chain_error(expr, err, srcref = utils::getSrcref(sys.call()))
 46. │   │   └─base::withCallingHandlers(...)
 47. │   └─base::eval(expr, envir = envir)
 48. │     └─base::eval(expr, envir = envir)
 49. └─base::.signalSimpleWarning(...)
 50.   └─base::withRestarts(...)
 51.     └─base (local) withOneRestart(expr, restarts[[1L]])
 52.       └─base (local) doWithOneRestart(return(expr), restart)

Happy to provide any more details that you'd like!

[hadley]

Aaaah ok, that makes sense. Looks like a bug in the doclisting package, so I've moved it there.

Is there any way you can share the source code for the package?

[mduncans]

Yup package is here: https://github.com/A2-ai/hyperion.tables/tree/roxygen-update and here is a link to the doclisting call in roxygen comments: https://github.com/A2-ai/hyperion.tables/blob/8f65c8469b306a9266b3ba0517123e3f336bc69b/R/spec-setters.R#L18

[hadley]

Yeah, looks like the fix needs to go in pkgload. That will require a little coordination but is in flight now. Thanks again for figuring this out!