Store the S7 methods table as an attribute of the S3 methods table

[hadley]

This prevents it from being listed in functions_in_S3_table (https://github.com/wch/r-source/blob/16c4fbf48efb4d43ef5dccadc86576100edd85b4/src/library/tools/R/QC.R#L370-L383) and later generating a warning when calling formals on each element of that list.

Fixes #342

[hadley]

I'm pretty sure that this is a backward incompatible change since previously built S7 packages will have stored their registered methods in a place that we no longer look. But I think this breakage is ok because there are CRAN packages that use S7 yet, and we haven't publicly announced it.

[jonthegeek]

I'm working on a reprex to locate it exactly, but having a mix of methods-as-traditional-S3s and methods-as-S7::method()<- caused issues with this fix. I can't tell yet if that would be a problem.

(I replaced one method to see if this fixed the issue I was seeing, and then had issues with all the other methods)

[jonthegeek]

This is... odd.

See the latest version of https://github.com/jonthegeek/S7primitivereprex

Specifically, this passes: https://github.com/jonthegeek/S7primitivereprex/blob/main/tests/testthat/test-my_class.R

But it fails if I run it outside of the test context:

pkgload::load_all()
my_other_letters <- my_other_class(letters)
expect_equal(length(my_other_letters), 2)
#> Error: length(my_other_letters) (`actual`) not equal to 2 (`expected`).
#> 
#>   `actual`: 26
#> `expected`:  2

It's only for the method that's defined as a "normal" S3 method, but the fact that it's invisible to the test is confusing.

[hadley]

@DavisVaughan I'm pretty sure I tried that first but I hit namespace is locked errors.

[t-kalinowski]

@DavisVaughan The namespace is already locked when .onLoad() is called (i.e., removal or addition of new bindings is forbidden, but changing of binding values is still permitted). After .onLoad() is called, then the bindings themselves are locked.

We could document users to define a .S7_methods object, or maybe even do so automatically w/ roxygen/Description, but this solution seems clean and pragmatic to me. (🤞 no other problems emerge from attaching attributes to the .__S3MethodsTable__. env.)

[hadley]

@jonthegeek yeah, I can't immediately see why that wouldn't work, but due to the general weirdness of S3 method lookup/registration, it doesn't surprise me that it doesn't fully for you. I don't think it's related to this issue.