[kevinushey]

Fixes #1529.

class_<T>::complete() (behind tab completion on module objects) sizes its output as the number of method names minus specials, but AddMethod() incremented specials once per overload of a [-prefixed method. With [[ overloaded, the last ordinary method went missing from the completions. If the class had no ordinary methods, the subtraction underflowed and the loop ran past the end of the method map. See #1529 for the reprex.

Change (inst/include/Rcpp/module/class.h, class_::AddMethod): increment specials only when a new method name is inserted, so it counts names, which is what complete() assumes. This also keeps the count right for methods copied in by derives(), which go through AddMethod() as well.

Tests (inst/tinytest/test_module.R, classes added to cpp/Module.cpp):

• ModuleBrackets overloads [[ and has an ordinary method and a field: completion gives c("total( ", "value"), and [[ still dispatches.
• ModuleBracketsOnly overloads [[ and has only a field: completion gives "value".

Verification:

• The reprex from tab completion on module objects drops methods or reads past the method map when [[ is overloaded #1529 prints "scale" and attempt to set index 0/0 in SET_STRING_ELT against current master, and "total( " "scale" and "scale" against this branch.
• Against master the new tests fail: test_module.R stops with the same SET_STRING_ELT error. On this branch test_module.R (48/48), test_modref.R, test_expose_class.R and test_module_client_package.R pass with RunAllRcppTests=yes RunVerboseRcppTests=yes (R 4.6.1, macOS).

Checklist

• Code compiles correctly
• R CMD check still passes all tests (module test files run locally as above)
• Preferably, new tests were added which fail without the change
• Document the changes by file in ChangeLog

[eddelbuettel]

@kevinushey Can you look into rebasing this following the earlier merges?

[eddelbuettel]

Did another re-alignment with the main branch.

[eddelbuettel]

LGTM and the beam-1.0 LLM (see details).

Details

Plan: Integrate Rcpp PR 1530 (count special module methods once per name)

Evaluation

PR 1530 fixes issue #1529. class_<T>::complete() sizes its output as
vec_methods.size() - specials, where vec_methods has one entry per method
name, but AddMethod() incremented specials once per overload for any
method whose name starts with [. With [[ overloaded, specials was too
large: ordinary methods were dropped from completions, and when there were no
ordinary methods size_t underflowed and complete() ran past end().

The PR's one-line change (increment specials only when a new method name is
inserted) is correct and minimal. It also keeps the count right for methods
imported via derives(), which go through AddMethod().

The PR is based on a master snapshot before #1528/#1532, so its test hunk
needs rebasing onto the current tree. Verified locally on a patched copy:
test_module.R goes from 78 to 81 tests with 0 failures; test_modref.R,
test_expose_class.R, and test_module_client_package.R also pass.

Changes

1. Core fix

File: inst/include/Rcpp/module/class.h, in class_::AddMethod.

Move the specials increment inside the new-name branch:

self& AddMethod(const char* name_, method_class* m, ValidMethod valid = &yes, const char* docstring = 0){
    RCPP_DEBUG_MODULE_1( "AddMethod( %s, method_class* m, ValidMethod valid = &yes, const char* docstring = 0", name_ )
    self* ptr = get_instance() ;
    typename map_vec_signed_method::iterator it = ptr->vec_methods.find( name_ ) ;
    if( it == ptr->vec_methods.end() ){
        it = ptr->vec_methods.insert( vec_signed_method_pair( name_, new vec_signed_method() ) ).first ;
        // complete() relies on this counting names, not overloads
        if( *name_ == '[' ) ptr->specials++ ;
    }
    (it->second)->push_back( new signed_method_class(m, valid, docstring ) ) ;
    return *this ;
}

2. Test fixtures

File: inst/tinytest/cpp/Module.cpp.

Add, before class ModuleGadget:

class ModuleBrackets {
public:
    ModuleBrackets() : value(1.0) {}

    double at(int i) { return value * i; }
    double at2(int i, int j) { return value * i * j; }
    double total() { return value; }

    double value;
};

class ModuleBracketsOnly {
public:
    ModuleBracketsOnly() : value(1.0) {}

    double at(int i) { return value * i; }
    double at2(int i, int j) { return value * i * j; }

    double value;
};

Register, in RCPP_MODULE(demoModule) before class_<ModuleGadget>:

    // overloaded [[, with and without an ordinary method
    class_<ModuleBrackets>("ModuleBrackets")
        .constructor()
        .method("[[", &ModuleBrackets::at)
        .method("[[", &ModuleBrackets::at2)
        .method("total", &ModuleBrackets::total)
        .field("value", &ModuleBrackets::value)
        ;

    class_<ModuleBracketsOnly>("ModuleBracketsOnly")
        .constructor()
        .method("[[", &ModuleBracketsOnly::at)
        .method("[[", &ModuleBracketsOnly::at2)
        .field("value", &ModuleBracketsOnly::value)
        ;

3. Tests

File: inst/tinytest/test_module.R, inserted after
expect_true( Module_scope_is_cleared() ) and before the retry tests:

## completion lists the ordinary methods and the fields; an overloaded [[
## used to be counted once per overload, which dropped the last ordinary
## method, or ran past the end of the method map when there was none (#1529)
brackets <- new( ModuleBrackets )
expect_equal( Rcpp:::complete( brackets ), c("total( ", "value") )
expect_equal( brackets[[2L]], 2.0 )
expect_equal( Rcpp:::complete( new( ModuleBracketsOnly ) ), "value" )

4. ChangeLog

Add to the current top dated block:

* inst/include/Rcpp/module/class.h (class_::AddMethod): Count special
'[' methods once per name rather than once per overload, as complete()
expects; overloading '[[' dropped methods from completion or ran past
the end of the method map (#1529)
* inst/tinytest/cpp/Module.cpp: Add classes overloading '[['
* inst/tinytest/test_module.R: Test their completions

No DESCRIPTION version bump is needed.

Verification

R CMD INSTALL --library=/home/edd/lab/lib .
cd inst/tinytest
R_LIBS=/home/edd/lab/lib Rscript -e 'Sys.setenv(RunAllRcppTests="yes",RunVerboseRcppTests="yes"); tinytest::run_test_file("test_module.R")'

Expected: 81 tests, 0 failures (baseline 78). Also run
test_modref.R, test_expose_class.R, and test_module_client_package.R.

[eddelbuettel]

Thanks for the merge