fix: check that we receive a scalar when expecting a scalar in C code

[szhorvat]

This is experimental. It is not completely clear to me if calling the error function directly at this point is fine ... there needs to be clarity about IGRAPH_CHECK vs IGRAPH_R_CHECK and where each is allowed to be used.

This fixes a potential crash in code such as sample_pref(c(), 3). Here c() is a zero-length vector which means that the C code would do an out-of-bounds access. This may or may not crash depending on your luck.

The behaviour is now like this:

> sample_pref(c(10,20), 3)
Error in sample_pref(c(10, 20), 3) : 
  At rinterface.c:1211 : Expecting a scalar but received a vector of length 2. Invalid value
> sample_pref(c(), 3)
Error in sample_pref(c(), 3) : 
  At rinterface.c:1211 : Expecting a scalar but received a vector of length 0. Invalid value

This is likely to cause revdepcheck failures. @krlmlr It would actually be worth doing a revdepcheck with this soon, as it may reveal graph bugs as well.

EDIT: Now it also checks for non-integer values (including NaN and infinities) when an integer is expected.

> sample_pref(1.23, 3)
Error in sample_pref(1.23, 3) : 
  At rinterface.c:1366 : The value 1.23 is not representable as an integer. Invalid value
> sample_pref(NA, 3)
Error in sample_pref(NA, 3) : 
  At rinterface.c:1366 : The value nan is not representable as an integer. Invalid value

[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 using Aviator.

---

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]

This already revealed an incorrect usage in an example, which I just fixed. See e44efad

[krlmlr]

Good idea, thanks!

[krlmlr]

The coverage checks take a hit here. I wonder if we should extract this conversion into a function, I suspect we won't add invalid argument checks for each and every function.

[szhorvat]

I also wonder if this should use Rf_error() instead.

I don't think so because memory is then not freed.

Error handling in R/igraph is still a bit of a mess. We don't have clear guidelines, and I don't have a full understanding of how this work or should work. This should really be figured out before 2.0. Otherwise making changes like this one is stressful and risky.

I wrote down my current understanding in a wiki page. Can you take a look? https://github.com/igraph/rigraph/wiki/Error-handling-in-C

If what I wrote is correct then igraph functions cannot be called from a callback. If they are called anyway, things will go wrong. Is this accurate?

Let's figure out how this should work, and continue afterwards.

CC @ntamas

[krlmlr]

What memory is there to free during argument checks? Do we need to separate argument checking from conversion?

[szhorvat]

What memory is there to free during argument checks? Do we need to separate argument checking from conversion?

Some arguments may be vector-valued. These may get converted to igraph vectors, which will need to be freed if there is a failure.

[krlmlr]

Is this still a problem if we first check arguments and then convert them?

[szhorvat]

Is this still a problem if we first check arguments and then convert them?

No, that is not possible. Suppose there are two vector arguments. First construct one, then the other. If the second construction fails, the first needs to be freed. This will always be a problem in some form.

The error handling will need to be clarified either way, and it is completely doable. Trying to avoid error handling just for argument checks won't gain us anything. Am I missing something?

[krlmlr]

We already agreed to use C++ and RAII for resource handling. Things will be much better once we have that. Right now, I'm focusing on releasing an update for the R package that will allow more frequent updates of the C core.

I'd like to postpone anything that's not moving us closer to that milestone. The checks in this PR are great, but I'd rather not open another can of worms here.

[szhorvat]

OK, just let me know if you plan to merge this before 2.0. If yes, I'll address your comments.

[krlmlr]

I'll run revdepchecks and push the results here. It's certainly good to have this in the code, an error (even with a memory leak) is better than undefined behavior.

[szhorvat]

Stylistic changes committed and squashed, handing this off to you now.

[krlmlr]

Thanks. I still think length and integerness checks should live in their own functions, for more realistic coverage results. We can run revdepchecks on the current state, but now that #1055 is merged, we might as well extract the functions as part of this PR.

@Antonov548: can you please extract functions for checking length one and integerness, and use them for the generated code?

[Antonov548]

Thanks. I still think length and integerness checks should live in their own functions, for more realistic coverage results. We can run revdepchecks on the current state, but now that #1055 is merged, we might as well extract the functions as part of this PR.

@Antonov548: can you please extract functions for checking length one and integerness, and use them for the generated code?

Yes, sure. I will take a look.

[krlmlr]

Only two failures, good to merge after extracting functions for the checks.

[Antonov548]

Only two failures, good to merge after extracting functions for the checks.

Does it make sense to make it in the same pr?

[krlmlr]

I think so, yes. This PR drops coverage, I'd like to get back to baseline before merging.

[szhorvat]

I think so, yes. This PR drops coverage, I'd like to get back to baseline before merging.

It doesn't truly reduce coverage. It only reduces the imperfect number that simple tools report. There are a lot of useful refactorings that drop that number, for example removing dead code often does.

[Antonov548]

I think so, yes. This PR drops coverage, I'd like to get back to baseline before merging.

It's green now

[krlmlr]

For the future: we could do something like

Suggested change

	IGRAPH_R_CHECK_INT(%I%);
	%C% = (igraph_integer_t) REAL(%I%)[0];
	%C% = IGRAPH_R_CHECK_INT(%I%);

with a suitable implementation of IGRAPH_R_CHECK_INT() . It's good as is for now.

[krlmlr]

Same for the other check functions.

[aviator-app]

This pull request can't be queued because it's currently a draft.

[szhorvat]

@krlmlr What do you think about refactoring these functions to macros? The main advantage is that the line number in the error message would actually be useful for debugging. It would pinpoint where exactly the error came from. Doing this should not have an effect on coverage (assuming that coverage is measured in the same way as for the C core).