`expect_s4_class()` may fail where `expect_true(is(...))` succeeds

[etiennebacher]

expect_s4_class_linter() recommends using expect_s4_class() instead of expect_true(is(...)) but sometimes expect_s4_class() fails.

lintr::lint("expect_true(is(mtcars, 'data.frame'))\n", lintr::expect_s4_class_linter())
#> <text>:1:1: warning: [expect_s4_class_linter] expect_s4_class(x, k) is better than expect_true(is(x, k)). Note also expect_s3_class() available for testing S3 objects.
#> expect_true(is(mtcars, 'data.frame'))
#> ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

testthat::expect_true(is(mtcars, 'data.frame'))
testthat::expect_s4_class(mtcars, 'data.frame')
#> Error: Expected `mtcars` to be an S4 object.
#> Actual OO type: S3.

The second part of the message suggests also trying expect_s3_class() but the main recommendation is expect_s4_class(). Maybe the message should be tweaked or "data.frame" (and possibly other classes) should be skipped?

---

The example used above isn't so rare (although I don't really know the number of matches required for a pattern to be "not rare"): https://github.com/search?q=lang%3AR+%2Fexpect_true%5C%28is%5C%28%5Ba-zA-Z0-9%5D%2B%2C+%22data%5C.frame%22%5C%29%5C%29%2F&type=code

[MichaelChirico]

Hm, I think I'm happy with the current state of the lint message.

• is() is meant for S4 objects -- the original test is incorrect (IMO)
• We do mention that expect_s3_class() might be needed, so either:
  • You see this right away and make the correct change
  • You naively switch to expect_s4_class(), get the test failure, and then make the correct change when the {testthat} failure helpfully points out the S3 issue

If we restrict your search to CRAN we only get 17 hits, so I don't think that's too common: https://github.com/search?q=lang%3AR+%2Fexpect_true%5C%28is%5C%28%5Ba-zA-Z0-9%5D%2B%2C+%22data%5C.frame%22%5C%29%5C%29%2F+org%3Acran&type=code

Even a much more flexible search possibly allowing more whitespace/complicated expressions/newlines only gets 30 hits:

https://github.com/search?q=lang%3AR+%2Fexpect_true%5C%28.*is%5C%28.*%2C+%22data%5C.frame%22%5C%29.*%5C%29%2F+org%3Acran&type=code

If you wanted to do a more thorough search, here are all the usages of is() on CRAN:

https://github.com/search?q=org%3Acran+path%3Atestthat+%2F%5B%5E%5Cw.%5Dis%5C%28%5B%5E%29%5D*%2C%2F&type=code

We could make a table of the RHS of is() calls and see what's the most common S3 target, and maybe redirect for a few.

[etiennebacher]

If we restrict your search to CRAN we only get 17 hits, so I don't think that's too common:

Right, I always forgot to add this restriction. Indeed that looks quite rare in that case.

I hit this issue when I tried to implement this rule in Jarl (https://github.com/etiennebacher/jarl) because I also wanted to have an automatic fix, but maybe I won't have one because of this issue. I just thought about reporting this in case it is of interest for you.

Feel free to close this issue if you prefer, and thanks for the explanations.

[MichaelChirico]

I agree it's a case where some judgment is needed -- an LLM can likely make the right call. Is it possible for the automated fixes to have a hook for a targeted prompt+response to the user's preferred model backend?

[etiennebacher]

Is it possible for the automated fixes to have a hook for a targeted prompt+response to the user's preferred model backend?

I'm not sure I understand the question but I don't want my linter to call LLMs. It should be a deterministic static code analysis. I might just implement the rule and its automated fix and mark it as "unsafe fix".

[MichaelChirico]

I don't want my linter to call LLMs

That settles it then :)

For completeness, I have a prompt-tuned version of this interaction in mind: