conflicted doesn't like inops

[moodymudskipper]

@KKPMW FYI : r-lib/conflicted#48

[karoliskoncevicius]

Yes, it's related to <<- overloading.

I tried loading the version with that function removed and had no issues.

One possibility is to simply remove the support for standard binary operators. The replacement for all of them can be achieved with %in% versions:

x == 2 <- NA
x %in{}% 2 <- NA

x < 2 <- NA
x %in[)% c(-Inf, 2) <- NA

etc

But also maybe it's something that conflicted will change. Let's wait and see?

[moodymudskipper]

They're some of my favourite things about the package. Basically I use %in~%, %[in~% and out counterparts a bunch, then the replacement versions of comparison ops, I'd much rather keep them, or in the worst case just withdraw <<- even if it'd be awkward.

But conflcted alone is not a good enough reason, there would need to be a more fundamental implication.

[moodymudskipper]

I'm pretty sure it's a conflicted problem and our package is fine. conflicted checks if conflicted functions are "superset" of base functions by checking their arguments, and it chokes on base::`<<-` because it's a primitive (has no formals).

library(conflicted) triggers conflicted:::.onAttach(), which calls conflicted:::conflicts_register(), and down the line, rlang::fn_fmls() is called, and it chokes on primitives.

[karoliskoncevicius]

Yup, it gives the same error for my "annmatrix" package, which overwrites @.

[karoliskoncevicius]

Regarding keeping it in / removing it: I am just worried that a lot of potential users will bail after seeing that it over-writes <<-. Not sure how you would react, but if I loaded a relatively little known package and it overwrote, say, <-, I would not trust it.

Other than that - I have nothing against it!

[moodymudskipper]

Maybe we should add a message with .onAttach to explain that the original behavior of <<- is maintained and that the fact we override it doesn't impact the performance of any packaged function, but in as few words as possible or it would have the opposite effect (more red ink = more scary).
Maybe we can override the current message ? It wouldn't be safe from R to allow it, but R is R :), and we could do it responsibly.

[moodymudskipper]

tidyverse actually does it so it's possible, but I think it's because it doesn't have conflicts itself but attaches other packages. I think a trick would be to define the function in .onLoad(), and move it to as.environment("package:inops") through on.Attach, along with displaying an accurate concise and not scary message.

Could still be documented as is, just replacing the definition by NULL and adding an explicit roxygen name.

Ironically this way it shouldn't be accessible by getNamespaceExports() which conflicted uses, so, two birds with one stone ?

[bastistician]

I am just worried that a lot of potential users will bail after seeing that it over-writes <<-. Not sure how you would react, but if I loaded a relatively little known package and it overwrote, say, <-, I would not trust it.

I'm one of those potential users. 😉
Just wanted to stress that the information that <<- no longer simply means base::"<<-" after attaching inops is essential and no steps should be taken to suppress this information.

I think CRAN repository policy is also relevant in this context:

A package must not tamper with the code already loaded into R: any attempt to change code in the standard and recommended packages which ship with R is prohibited.

I know, base::"<<-" is not changed by your package, but overloading this basic assignment operator of the R language is delicate.

[moodymudskipper]

Hi Sebastian, thanks for sharing your concern.

I can assure you that we will never try to dissimulate the information that <<- is masked by our package, the discussion is about making the warning easier to understand, for the benefit of all, and we're worried about this message leading to misunderstandings rather than legitimate worries.

The CRAN text that you quote refers to changes that would impact all base functions or packages wrapping the functionality, these are to be avoided at all costs. Our overloading of this operator won't affect how <<- works in any namespaced function.

It should also not change the way it works at all, because its definition is only altered to deal with the case where 3 arguments are provided, to be able to do x < y <- z, the original binary use is unaffected, it should only suffer a very small performance cost when defined in the session.

Here is the code in case you haven't seen it :

inops::`<<-`
function (x, y, value) 
{
    if (missing(value)) {
        eval.parent(substitute(.Primitive("<<-")(x, y)))
    }
    else {
        replace(x, x < y, value)
    }
}

All that being said I realize this is surprising, and still reflecting on what's the best way to deal with this, but I fine the operator useful and would like to keep it.

Maybe this should be its own issue, that we'd pin, as other users might come looking for it.

[moodymudskipper]

@KKPMW I added a pinned issue, if we need to discuss related technical issues let's do them here our in a separate issue and leave the pinned issue to address user comments if you don't mind.

[karoliskoncevicius]

@moodymudskipper good idea with the pinned issue. We might also add it in the notes section of the readme.

I also agree with @bastistician that we should not hide or downplay the fact that the package overloads <<-. But not sure what is the best way to go about it. It's just an issue of unfortunate naming convention R uses. If assignment functions had a different special naming convention this issue would not arise.

[moodymudskipper]

The following would be feasible in theory, but cause other issues, so I think we just have to live with the message :

---

I -

• define <<- in package but don't export
• in .onAttach() :
  • display clearer error message
  • unlock as.environment("package:inops")
  • place <<- there
  • lock back environment

In practice to unlock the environment it seems the only ways to do this are by using C/C++ with either package inline or Rcpp, and adding a dependency just to hack a message is overkill of course.

---

II -

• define <<- in package but don't export
• in .onAttach() :
  • display clearer error message
  • attach(list(inops_aux = `<<-`))
• in .onDetach() :
  • detach("inops_aux")

This would work smoothly I think, and other packages attach things to environments, like devtools, imports.

Note that or conflicted for that matter avoids the warning messages by overriding library() and require() in a new environment, and doesn't warn anyone about it. devtools does it also at least with ?, and no one seems to complain.

I don't think our issue deserves making the search path weirder, but it's interesting that Hadley and CRAN think it's ok to override without message as long as it's not in "package:mypkg" and that general behavior is kept (I had problems with the ? from devtools with my pakages doubt and selfbm though so I think a message is justified).

---

III -

• define <<- in package but don't export
• in .onAttach() :
  • assign <<- in the global environment

Mentioned for completeness, but very bad practice, probably forbidden by CRAN anyway

[bastistician]

Thanks for the clarifications. I agree that inops should not hack around the default message about masking <<-. Users of inops will deliberately attach the package and will know that it defines additional operators. They may also have seen the note about <<- in your README. So if users eventually find the message annoying, they will simply use

library("inops", warn.conflicts = FALSE)

in their scripts.

[moodymudskipper]

I just noticed today that tidyverse packages don't warn anymore that %>% is masked.
It's perfectly acceptable, and is welcome in my opinion as %>% was just reexported and not overridden with a new definition as we do here, still this is interesting, I haven't looked yet into how they do it.

library(magrittr)
library(purrr)
#> 
#> Attaching package: 'purrr'
#> The following object is masked from 'package:magrittr':
#> 
#>     set_names

^{Created on 2019-12-09 by the reprex package (v0.3.0)}