tidyverse / magrittr

Improve the readability of R code with the pipe
https://magrittr.tidyverse.org
Other
957 stars 157 forks source link

Pipe does not do what is supposed to do when the function is given with library name and no parens #193

Closed balwierz closed 4 years ago

balwierz commented 5 years ago

Not sure if this is fixable or even if it is an unexpected behaviour.

require(IRanges)
IRanges(start=c(1,5), end=c(6,9)) %>% IRanges::reduce
Error in .::IRanges : unused argument (reduce)

#these two however work:
IRanges(start=c(1,5), end=c(6,9)) %>% IRanges::reduce()
IRanges(start=c(1,5), end=c(6,9)) %>% (IRanges::reduce)
ColinFay commented 5 years ago

Hey,

I think this is a {maggritr} question. I don't see how it relates to {purrr} 🤔 ?

balwierz commented 5 years ago

Hi Colin,

I am a user, not a developer of purrr and don't know its internal code. I am not explicitly loading maggritr. After loading purrr my sessionInfo does not show maggritr. I assumed the pipe is (re)implemented in purrr.

purr pipe actually exists: purrr::%>% and is being called.

From my user perspective it is purrr::%>% being called. As a user I do not even need to be aware of the existence of the maggritr package. :) A package maintainer can use his expert knowledge to decide what to do next, e.g. resubmit it to the appropriate upstream project.

PS. Yes, you are right. If I correctly guess what the syntax in the beginning of utils.R in purrr/R mean, purrr::%>% is a pointer to magrittr::%>%. Shall I resubmit it there?


> purrr::`%>%`
function (lhs, rhs)
{
    parent <- parent.frame()
    env <- new.env(parent = parent)
    chain_parts <- split_chain(match.call(), env = env)
    pipes <- chain_parts[["pipes"]]
    rhss <- chain_parts[["rhss"]]
    lhs <- chain_parts[["lhs"]]
    env[["_function_list"]] <- lapply(1:length(rhss), function(i) wrap_function(rhss[[i]],
        pipes[[i]], parent))
    env[["_fseq"]] <- `class<-`(eval(quote(function(value) freduce(value,
        `_function_list`)), env, env), c("fseq", "function"))
    env[["freduce"]] <- freduce
    if (is_placeholder(lhs)) {
        env[["_fseq"]]
    }
    else {
        env[["_lhs"]] <- eval(lhs, parent, parent)
        result <- withVisible(eval(quote(`_fseq`(`_lhs`)), env,
            env))
        if (is_compound_pipe(pipes[[1L]])) {
            eval(call("<-", lhs, result[["value"]]), parent,
                parent)
        }
        else {
            if (result[["visible"]])
                result[["value"]]
            else invisible(result[["value"]])
        }
    }
}
<bytecode: 0x55b5e5f68d98>
<environment: 0x55b5e5f5ab68>

> IRanges(start=c(1,5), end=c(6,9)) %>% IRanges::reduce
Error in .::IRanges : unused argument (reduce)
> traceback()
8: function_list[[k]](value)
7: withVisible(function_list[[k]](value))
6: freduce(value, `_function_list`)
5: `_fseq`(`_lhs`)
4: eval(quote(`_fseq`(`_lhs`)), env, env)
3: eval(quote(`_fseq`(`_lhs`)), env, env)
2: withVisible(eval(quote(`_fseq`(`_lhs`)), env, env))
1: IRanges(start = c(1, 5), end = c(6, 9)) %>% IRanges::reduce

> sessionInfo()
R version 3.5.3 (2019-03-11)
Platform: x86_64-pc-linux-gnu (64-bit)
Running under: Arch Linux

Matrix products: default
BLAS: /usr/lib/libopenblasp-r0.3.5.so
LAPACK: /usr/lib/liblapack.so.3.8.0

locale:
 [1] LC_CTYPE=en_DK.UTF-8       LC_NUMERIC=C
 [3] LC_TIME=en_DK.UTF-8        LC_COLLATE=C
 [5] LC_MONETARY=en_DK.UTF-8    LC_MESSAGES=en_DK.UTF-8
 [7] LC_PAPER=en_DK.UTF-8       LC_NAME=C
 [9] LC_ADDRESS=C               LC_TELEPHONE=C
[11] LC_MEASUREMENT=en_DK.UTF-8 LC_IDENTIFICATION=C

attached base packages:
[1] stats4    parallel  stats     graphics  grDevices utils     datasets
[8] methods   base

other attached packages:
[1] IRanges_2.16.0      S4Vectors_0.20.1    BiocGenerics_0.28.0
[4] purrr_0.3.0

loaded via a namespace (and not attached):
[1] compiler_3.5.3 magrittr_1.5   rlang_0.3.1
ColinFay commented 5 years ago

Hello,

Yes, {purrr} re-exports the pipe from {magrittr} and doesn't do anything more than "copying" it, so as far as I know the issue belongs other there :)

Best, Colin

batpigandme commented 5 years ago

After loading purrr my sessionInfo does not show maggritr. I assumed the pipe is (re)implemented in purrr.

Hi @balwierz, FWIW, this is why it's so helpful to have a full reprex (self-contained reproducible example) in the original bug report (check out the reprex package here), so we can see libraries you've attached etc.

library(IRanges)
#> Loading required package: BiocGenerics
#> Loading required package: parallel
#> 
#> Attaching package: 'BiocGenerics'
#> The following objects are masked from 'package:parallel':
#> 
#>     clusterApply, clusterApplyLB, clusterCall, clusterEvalQ,
#>     clusterExport, clusterMap, parApply, parCapply, parLapply,
#>     parLapplyLB, parRapply, parSapply, parSapplyLB
#> The following objects are masked from 'package:stats':
#> 
#>     IQR, mad, sd, var, xtabs
#> The following objects are masked from 'package:base':
#> 
#>     anyDuplicated, append, as.data.frame, basename, cbind,
#>     colMeans, colnames, colSums, dirname, do.call, duplicated,
#>     eval, evalq, Filter, Find, get, grep, grepl, intersect,
#>     is.unsorted, lapply, lengths, Map, mapply, match, mget, order,
#>     paste, pmax, pmax.int, pmin, pmin.int, Position, rank, rbind,
#>     Reduce, rowMeans, rownames, rowSums, sapply, setdiff, sort,
#>     table, tapply, union, unique, unsplit, which, which.max,
#>     which.min
#> Loading required package: S4Vectors
#> Loading required package: stats4
#> 
#> Attaching package: 'S4Vectors'
#> The following object is masked from 'package:base':
#> 
#>     expand.grid
library(purrr)
#> 
#> Attaching package: 'purrr'
#> The following object is masked from 'package:IRanges':
#> 
#>     reduce

IRanges(start=c(1,5), end=c(6,9)) %>% IRanges::reduce
#> Error in .::IRanges: unused argument (reduce)

IRanges(start=c(1,5), end=c(6,9)) %>% IRanges::reduce()
#> IRanges object with 1 range and 0 metadata columns:
#>           start       end     width
#>       <integer> <integer> <integer>
#>   [1]         1         9         9
IRanges(start=c(1,5), end=c(6,9)) %>% (IRanges::reduce)
#> IRanges object with 1 range and 0 metadata columns:
#>           start       end     width
#>       <integer> <integer> <integer>
#>   [1]         1         9         9

Created on 2019-03-18 by the reprex package (v0.2.1)

@ColinFay is right that purrr re-exports the magrittr pipe (no need to worry, it's easy enough for us to move an issue).

Since you're not using purrr::reduce() (you explicitly use IRanges::reduce()), we can also look and see if there's anything actually purrr-y about the error. For example, since dplyr also reexports the magrittr pipe, we can see what happens with that, or with magrittr directly:

with dplyr and magrittr
``` r library(IRanges) #> Loading required package: BiocGenerics #> Loading required package: parallel #> #> Attaching package: 'BiocGenerics' #> The following objects are masked from 'package:parallel': #> #> clusterApply, clusterApplyLB, clusterCall, clusterEvalQ, #> clusterExport, clusterMap, parApply, parCapply, parLapply, #> parLapplyLB, parRapply, parSapply, parSapplyLB #> The following objects are masked from 'package:stats': #> #> IQR, mad, sd, var, xtabs #> The following objects are masked from 'package:base': #> #> anyDuplicated, append, as.data.frame, basename, cbind, #> colMeans, colnames, colSums, dirname, do.call, duplicated, #> eval, evalq, Filter, Find, get, grep, grepl, intersect, #> is.unsorted, lapply, lengths, Map, mapply, match, mget, order, #> paste, pmax, pmax.int, pmin, pmin.int, Position, rank, rbind, #> Reduce, rowMeans, rownames, rowSums, sapply, setdiff, sort, #> table, tapply, union, unique, unsplit, which, which.max, #> which.min #> Loading required package: S4Vectors #> Loading required package: stats4 #> #> Attaching package: 'S4Vectors' #> The following object is masked from 'package:base': #> #> expand.grid library(dplyr) #> #> Attaching package: 'dplyr' #> The following objects are masked from 'package:IRanges': #> #> collapse, desc, intersect, setdiff, slice, union #> The following objects are masked from 'package:S4Vectors': #> #> first, intersect, rename, setdiff, setequal, union #> The following objects are masked from 'package:BiocGenerics': #> #> combine, intersect, setdiff, union #> The following objects are masked from 'package:stats': #> #> filter, lag #> The following objects are masked from 'package:base': #> #> intersect, setdiff, setequal, union IRanges(start=c(1,5), end=c(6,9)) %>% IRanges::reduce #> Error in .::IRanges: unused argument (reduce) IRanges(start=c(1,5), end=c(6,9)) %>% IRanges::reduce() #> IRanges object with 1 range and 0 metadata columns: #> start end width #> #> [1] 1 9 9 IRanges(start=c(1,5), end=c(6,9)) %>% (IRanges::reduce) #> IRanges object with 1 range and 0 metadata columns: #> start end width #> #> [1] 1 9 9 ``` Created on 2019-03-18 by the [reprex package](https://reprex.tidyverse.org) (v0.2.1)
with magrittr
``` r library(IRanges) #> Loading required package: BiocGenerics #> Loading required package: parallel #> #> Attaching package: 'BiocGenerics' #> The following objects are masked from 'package:parallel': #> #> clusterApply, clusterApplyLB, clusterCall, clusterEvalQ, #> clusterExport, clusterMap, parApply, parCapply, parLapply, #> parLapplyLB, parRapply, parSapply, parSapplyLB #> The following objects are masked from 'package:stats': #> #> IQR, mad, sd, var, xtabs #> The following objects are masked from 'package:base': #> #> anyDuplicated, append, as.data.frame, basename, cbind, #> colMeans, colnames, colSums, dirname, do.call, duplicated, #> eval, evalq, Filter, Find, get, grep, grepl, intersect, #> is.unsorted, lapply, lengths, Map, mapply, match, mget, order, #> paste, pmax, pmax.int, pmin, pmin.int, Position, rank, rbind, #> Reduce, rowMeans, rownames, rowSums, sapply, setdiff, sort, #> table, tapply, union, unique, unsplit, which, which.max, #> which.min #> Loading required package: S4Vectors #> Loading required package: stats4 #> #> Attaching package: 'S4Vectors' #> The following object is masked from 'package:base': #> #> expand.grid library(magrittr) IRanges(start=c(1,5), end=c(6,9)) %>% IRanges::reduce #> Error in .::IRanges: unused argument (reduce) IRanges(start=c(1,5), end=c(6,9)) %>% IRanges::reduce() #> IRanges object with 1 range and 0 metadata columns: #> start end width #> #> [1] 1 9 9 IRanges(start=c(1,5), end=c(6,9)) %>% (IRanges::reduce) #> IRanges object with 1 range and 0 metadata columns: #> start end width #> #> [1] 1 9 9 ``` Created on 2019-03-18 by the [reprex package](https://reprex.tidyverse.org) (v0.2.1)

It's possible the syntax isn't "legal" for some reason — but, we could certainly document that better, if it's the case. But, the issue title is a bit misleading, as it does work with prefix notation, just not without parentheses.

balwierz commented 5 years ago

@batpigandme : Thanks

An idea if this is not fixable due to the syntax: When you pipe an object to an unnamed function it throws an error and the user immediately knows what is going on. Maybe the same can be done when a pipe is to a library name?

> 7 %>% function(x) {5 + x}
Error: Anonymous functions myst be parenthesized

// Yes it says "myst" not "must" :-)

klmr commented 5 years ago

The issue is that x %>% pkg::fun is equivalent to x %>% `::`(pkg, fun). That is, the right-hand side is just a normal function call, and magrittr handles this by injecting the left-hand side as the first argument:

`::`(x, pkg, fun)

… which fails because `::` is as binary operator. The way magrittr handles this is “by design”. The same issue exists for example with x %>% env$fun. A straightforward workaround is to simply always use parentheses after function calls after the right-hand side in a pipeline. That way you’re using consistent syntax and there’s never any ambiguity (including in other cases, such as function factories).

balwierz commented 5 years ago

That was my guess what happened.