pkgdown icon indicating copy to clipboard operation
pkgdown copied to clipboard

Incorrect rendering of comparison generics `<`, `>`, etc.

Open bnprks opened this issue 2 years ago • 4 comments

The usage section of the html docs for S4 method implementations of comparison generics incorrectly renders < and > with &lt; and &gt;

Minimal reproducible package example here: https://github.com/bnprks/pkgdownbug (incorrectly rendered reference here)

An R file with text:

#' Comparison methods
#'
#' Generic methods and built-in functions for MyType objects
#'
#' @name ComparisonMethods
#' @rdname ComparisonMethods
NULL

setClass("MyType")

#' @describeIn ComparisonMethods Less than comparison
setMethod("<", signature(e1= "MyType", e2= "numeric"), function(e1, e2) {return(TRUE)})
#' @describeIn ComparisonMethods Less than comparison
setMethod("<=", signature(e1= "MyType", e2= "numeric"), function(e1, e2) {return(TRUE)})
#' @describeIn ComparisonMethods Less than comparison
setMethod(">", signature(e1= "MyType", e2= "numeric"), function(e1, e2) {return(TRUE)})
#' @describeIn ComparisonMethods Less than comparison
setMethod(">=", signature(e1= "MyType", e2= "numeric"), function(e1, e2) {return(TRUE)})

gets a usage section rendered to html as follows:

# S4 method for MyType,numeric
&lt;(e1, e2)

# S4 method for MyType,numeric
&lt;=(e1, e2)

# S4 method for MyType,numeric
&gt;(e1, e2)

# S4 method for MyType,numeric
&gt;=(e1, e2)

bnprks avatar May 03 '23 02:05 bnprks

Thanks for reporting! I don't have time to investigate right now but it looks related to #2286

maelle avatar May 04 '23 11:05 maelle

It's already using {{{ }}} in the template, so it must be getting escaped elsewhere 🤔

hadley avatar Apr 18 '24 22:04 hadley

Ok, at least two problems here:

  • The escape_html() in the fallback for highlight_text() is (I think) generally unnecessary because flatten_text() calls as_html() which already escapes text (will need to double check this)
  • We're only hitting that fallback because we're generating invalid R code because we're not handling methods for infix functions correctly.

And there's yet another bug in the way we're detecting whether or not a function is infix.

hadley avatar Apr 18 '24 22:04 hadley

Ok, the root problem is that the Rd looks like this:

\S4method{<}{MyType,numeric}(e1, e2)

and we want to generate a usage like this:

# S4 method for MyType,numeric
e1 < e2

In particular, note that (e1, e2) is not the call to \S4method{} so we can't the existing Rd tokenization to help us solve this problem.

It looks like base R effectively uses a hand-rolled tokeniser to solve this problem so we'll need to implement something similar in pkgdown.


A couple of test cases for when I take a stab at this:

test_that("infix methods parsed correctly", {
  out <- rd2html("\\S3method{<}{foo}(x, y)")
  expect_equal(out[[2]], "x &lt; y")

  out <- rd2html("\\S4method{<}{foo}(x, y)")
  expect_equal(out[[2]], "x &lt; y")
})

hadley avatar Apr 22 '24 21:04 hadley

After PR motivating example looks like this: Screenshot 2024-06-03 at 09 59 40

hadley avatar Jun 03 '24 15:06 hadley