Conflict with `base::ifelse` causing `{quarto}` to throw a fatal error when plotting from a non-interactive session

[mattsams89]

(This is a snippet. I've been at work for 14 hours and am tired and angry, so I'll clean this up and add more detail and hopefully a reprex tomorrow.)

I came across a hell of a doozy when rendering some reports tonight. One work laptop was still running 0.8, so I updated it to the latest dev version (for reference, the other laptop is still on 2b0efe9605091ff4619653ba4a71960e2fde86f4, which fixed #565). All my reports caught fire when I attempted to re-render them from said updated laptop. Specifically, I received the following:

Error in fifelse(condition, args$true, args$false, args$missing) :
  'yes' is of type NULL but 'no' is of type character. Please make sure that both arguments have the same type.
Calls: .main ... run_hook_plot -> hook -> ifelse -> if_else -> fifelse

I did some digging based on the error, and it looks like the {knitr} plotting hook {quarto} uses has an ifelse...somewhere I haven't found yet...that's erroring out after being masked by the new dotless ifelse. I assume it's failing because of a conflict that gets ignored by the {base} version of ifelse, but my pretty halfhearted search of the {knitr} repo didn't produce any obvious hits.

At any rate, I tried to diagnose what commit started producing the error. The last commit that runs successfully is 5267e8a0f54cd2949348f29fb12ce3250c18f013, so the first dotless commit (83b826060a126c1974d18c4646b6739fdbfdbedc) is where the error first appears.

I am able to circumvent the error if I call

conflicted::conflict_prefer("ifelse", "base")
conflicted::conflict_prefer("%in%", "tidytable")

but that isn't a great long-term solution.

I know this may be more of a yihui and {knitr} issue than a {tidytable} issue (and who knows, maybe they've addressed a similar problem already and I'm just too tired to read through their issue history), but figured I'd put this into the ether regardless. If you think it's more of an issue on their end, let me know and I'll open an issue there.

[markfairbanks]

Hmm this seems like it’s worth opening in the knitr repo. A function I overwrite shouldn’t affect their code.

I could just remove tidytable::ifelse() if it causes you issues, but it would also make it harder for them to reproduce.

[markfairbanks]

I could also probably fix this by accounting for NULLs. But the issue itself still seems worth opening in knitr.

[mattsams89]

I could also probably fix this by accounting for NULLs. But the issue itself still seems worth opening in knitr.

Yeah, agreed. They should be using a check with a little more type safety there, I would wager, so I'll open an issue! In the meantime, I'll probably just stick to conflict_prefer since I could see NULL-specific checks introducing their own set of unintended consequences.

[markfairbanks]

To be honest I'm not sure what the best approach is here from my side. knitr shouldn't be affected by tidytable overwriting ifelse(). Even if someone overwrites mutate.() it would still work correctly inside tidytable functions.

But ignoring that part of it - the next question is: should I build something in for NULLs? I was leaning "no" since it seems like poor behavior. But as far as I can tell this doesn't even work for base R anyway.

x <- "a"

base::ifelse(x == "a", NULL, "b")
#> Warning in rep(yes, length.out = len): 'x' is NULL so the result will be NULL
#> Error in ans[ypos] <- rep(yes, length.out = len)[ypos]: replacement has length zero

[markfairbanks]

In the short term I think I might just delete tidytable::ifelse(). Users will still have tidytable::if_else() and tidytable::ifelse.() available. I can also auto-translate ifelse() to a faster version in the background if they use it inside mutate(). That way users still get the performance benefits and I don't break quarto lol.

[mattsams89]

In case you're interested in following along.

[mattsams89]

Funny enough, apparently some other packages don't handle NAs too safely in their internal ifelse either. E.g., I have some custom color palette functions in flextable that are struggling if I don't provide explicit NA conditions. At least it's forcing me to always acknowledge and handle my NA conditions. 😂

[markfairbanks]

I deleted tidytable::ifelse() from the dev version of tidytable. However I still believe this is an issue that knitr should look at, as it's a function I would like to export in the future.

I created a branch to install the version that exports tidytable::ifelse() to help with reproducibility:

remotes::install_github("markfairbanks/tidytable@knitr-issue")

[cscheid]

Hi - quarto dev here.

However I still believe this is an issue that knitr should look at, as it's a function I would like to export in the future.

It seems that the base version of ifelse supports arguments of different types. Regardless of how good of a decision that is (I happen to agree with your stance on type strictness!), I believe that if you expose a type-strict version of ifelse under that name, that'll be a bug on tidytable. This is especially the case since up to "nullable values", the types we're using are the same, and NULLs are used pretty widely in R:

Error in fifelse(condition, args$true, args$false, args$missing): 'yes' is of type NULL but 'no' is of type character.

tl;dr: I think this is not a quarto/knitr bug, unfortunately.

[markfairbanks]

@cscheid - I more or less agree with you, and I'm fine not exporting tidytable::ifelse(). I've already removed it from the dev version of tidytable.

What's odd is this is causing an issue in quarto execution, which doesn't rely on tidytable::ifelse(). The code mentioned in that issue doesn't use tidytable::ifelse() either. It's simply printing a plot. This seems like a failure point in quarto, as that means any functions that are overwritten by external packages could cause breakages in quarto.

Edit: (I might be wrong btw - I'm not exactly sure how quarto works. This is just my impression at the moment.)

[cscheid]

yes, it's a quirk for sure, ultimately from how we're calling knitr (I believe this would happen with RMarkdown as well, though I'm not sure). We import knitr and call it on the R process that does extra work, instead of wrapping it in a subprocess call. I think that's more-or-less defensible, but I agree it's not without its risks.