feat: Use safe accessor to vector elements

[Antonov548]

For #1631 .

[aviator-app]

Current Aviator status

Aviator will automatically update this comment as the status of the PR changes.
Comment /aviator refresh to force Aviator to re-examine your PR (or learn about other /aviator commands).

This PR was merged manually (without Aviator). Merging manually can negatively impact the performance of the queue. Consider using Aviator next time.

---

See the real-time status of this PR on the Aviator webapp.

Use the Aviator Chrome Extension to see the status of your PR within GitHub.

[szhorvat]

There are two things that bother me in the implementation:

• We should avoid using int for indexing. The R API has a different type for this (I forget the name) that resolves to either a 32-bit or a 64-bit integer depending on configuration.
• Naked calls to igraph_errorf() should be avoided. The behaviour depends on context (see how IGRAPH_R_CHECK sets global variables to control this). Normally, error checks are done through return codes using either IGRAPH_CHECK (usual) or IGRAPH_R_CHECK (top level). If we allow such naked calls, then we must keep in mind in what context functions like R_get_int_scalar are or aren't allowed to be called, which is a bit too much mental burden. It is always possible to have a function that takes a pointer to the variable it is going to set, and returns an error code.

I just wanted to note these potential issues. Proceed as you see fit.

[krlmlr]

Perhaps:

Suggested change

	igraph_errorf("Wrong index. Attempt to get element with index %" PRIuPTR " from vector of length %" PRIuPTR ".",
	__FILE__, __LINE__, IGRAPH_EINVAL, (uintptr_t) index, (uintptr_t) Rf_xlength(sexp));
	Rf_error("Wrong index. Attempt to get element with index %" PRIuPTR " from vector of length %" PRIuPTR ".",
	file, line, IGRAPH_EINVAL, (uintptr_t) index, (uintptr_t) Rf_xlength(sexp));

with file and line passed from the invocation point (e.g., https://github.com/igraph/rigraph/pull/1633/files#diff-44d6b09dbc4b7f024f1b2df8dcc51a6e3d489408a6f82def8dd49ffb1bcd5dc4R4610) .

[szhorvat]

This won't properly unwind the "finally" stack.

[krlmlr]

This is still better than undefined behavior.

I don't have a good answer here. Let's draft and iterate.

[Antonov548]

This won't properly unwind the "finally" stack.

Is it not only the way to properly unwind the finally stack is to call igraph_error() directly?

[szhorvat]

In the C core we use this approach:

igraph_error_t R_get_int_scalar(SEXP sexp, R_xlen_t index, igraph_integer_t *res) {
    if (Rf_xlength(sexp) <= index) {
        IGRAPH_ERRORF("Wrong index. Attempt to get element with index %" PRIuPTR " from vector of length %" PRIuPTR ".", IGRAPH_EINVAL, (uintptr_t) index, (uintptr_t) Rf_xlength(sexp));
    }
    *res = (igraph_integer_t)REAL(sexp)[index];
    return IGRAPH_SUCCESS;
}

Then R_get_int_scalar returns an error code as usual, and can be wrapped in IGRAPH_CHECK or IGRAPH_R_CHECK.

Would this work for you?

[Antonov548]

In the C core we use this approach:

igraph_error_t R_get_int_scalar(SEXP sexp, R_xlen_t index, igraph_integer_t *res) {
    if (Rf_xlength(sexp) <= index) {
        IGRAPH_ERRORF("Wrong index. Attempt to get element with index %" PRIuPTR " from vector of length %" PRIuPTR ".", IGRAPH_EINVAL, (uintptr_t) index, (uintptr_t) Rf_xlength(sexp));
    }
    *res = (igraph_integer_t)REAL(sexp)[index];
    return IGRAPH_SUCCESS;
}

Then R_get_int_scalar returns an error code as usual, and can be wrapped in IGRAPH_CHECK or IGRAPH_R_CHECK.

Would this work for you?

Yes, this is what I was thinking about. Thanks.
I'll try this way.

[szhorvat]

Is it not only the way to properly unwind the finally stack is to call igraph_error() directly?

Yes in the sense that igraph_error() should be used, but normally it is called through IGRAPH_ERROR, which ensures returning from the function with an error code, which can then be processed by IGRAPH_CHECK.

In igraph 0.10, the unwinding process can potentially have multiple stages, each of which need to be triggered appropriately. This means that igraph_error() should not blindly longjmp. That might leave some stages untriggered.

This is why in the R interface we have IGRAPH_R_CHECK which must only be used at the top level. Only at this level is it safe to longjmp from the error handler, i.e. to call Rf_error(). IGRAPH_R_CHECK controls whether Rf_error() is called by igraph_error() by setting a global flag.

Now one might argue that R_get_int_scalar() is (or should be) only called at the top level anyway, so we can forget all about this. (Note that I am not actually sure whether this is true, there might be some non-top-level functions where things like REAL(sexp)[0] are still used!) If you decide to not worry about this, there's a risk that someone will use R_get_int_scalar() in an inappropriate context in the future, which causes a bug that's only triggered in an error condition, and will be very painful to track down. Thus if you go this route, I recommend clearly documenting what check function is allowed to be called in what context, and perhaps even naming these functions in such a way that any misuse is immediately apparent.

[Antonov548]

Thus if you go this route, I recommend clearly documenting what check function is allowed to be called in what context, and perhaps even naming these functions in such a way that any misuse is immediately apparent.

Not sure right now how to manage this, but I would think that functions which are have SEXP type as argument should be used only in top level functions. Which is also the case for R_get_int_scalar(). Maybe it make sense.

[krlmlr]

I agree that R_get_int_scalar() and friends should only be called at the top level and before any finalizers are set up. Also agree to document the purpose and the constraints.

[krlmlr]

Thanks!