Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix function in call is S3 matching #125

Merged
merged 4 commits into from
Sep 26, 2017
Merged

Fix function in call is S3 matching #125

merged 4 commits into from
Sep 26, 2017

Conversation

mschubert
Copy link
Collaborator

If b is a do.call(function(...)), the is_S3_user_generic will check a function in the next recursion.
Because this can not be indexed by b[[1]], we get the following error:

Error in b[[1]] : object of type 'closure' is not subsettable

The PR additionally checks if b is a function, and gets its body if it is.

Alternatively, if b is a call and not S3, do we need to check its contents for S3?

If `b` is a `do.call(function(...))`, the `is_S3_user_generic` will check a `function` in the next recursion.
Because this can not be indexed by `b[[1]]`, we get the following error:

> Error in b[[1]] : object of type 'closure' is not subsettable

The PR additionally checks if `b` is a function, and gets its body if it is.

Alternatively, if `b` is a `call` and *not* S3, do we *need* to check its contents for S3?
@klmr
Copy link
Owner

klmr commented Sep 6, 2017

Could you provide an MWE that illustrates the failure? … ideally as a unit test that fails without the PR and greenlights afterwards, but this isn’t necessary.

At any rate, the body call in https://github.com/klmr/modules/blob/9581658ad69622d4091dc2dcd01356398c82f9c2/R/S3.r#L57 can probably be omitted now, right?

@mschubert
Copy link
Collaborator Author

I was afraid you'd say that 😄

I tried. And after an hour I gave up. It's got something to do with a function in an environment attached to a closure, but I couldn't easily reproduce it. However, that is the reason why currently import('ebits/stats') fails (which is a mess by the way, I should rewrite that 😇).

And yes, the outer body call could be omitted then.

@klmr
Copy link
Owner

klmr commented Sep 7, 2017

Argh, that’s unfortunate. I’ll take a look as soon as I get a minute.

@klmr klmr added the ⚠️ bug label Sep 9, 2017
@klmr
Copy link
Owner

klmr commented Sep 26, 2017

Alright, here’s an MWE:

# x.r
f = function () NULL
body(f) = substitute(do.call(g, list()), list(g = function () 24))

I don’t think there’s a way of achieving the same without manipulating the function’s body at runtime (but I’d love to be proven wrong). Now on to create a test case.

@klmr klmr merged commit 1411886 into develop Sep 26, 2017
@klmr klmr deleted the mschubert-patch-1 branch September 26, 2017 15:50
klmr added a commit that referenced this pull request Oct 24, 2017
Hotfix #125 (1411886) into master:

Fix function in call is S3 matching
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants