Add protection against integer overflows in sugar functions

[Enchufa2]

Closes #1454

Work in progress.

Checklist

• Code compiles correctly
• R CMD check still passes all tests
• Preferably, new tests were added which fail without the change
• Document the changes by file in ChangeLog

[kevinushey]

Do these handle NAs? Or would these only ever be invoked for the "not NA" cases?

[Enchufa2]

In principle, sugar functions already handle NAs, so these will be invoked for non-NA cases. But I haven't triaged all the cases yet, so we'll see. If this implementation looks good so far, I'll continue with that.

[Enchufa2]

BTW @eddelbuettel I don't see the sugar directory listed in Codecov. Isn't the covr workflow running all the tests?

[eddelbuettel]

I don't think we exclude Sugar from tests or coverage. There is a specific entry point test_sugar.R with matching cpp file.

[Enchufa2]

Yes, and it requires RunAllRcppTests set to yes, so I was wondering if the covr task is not setting it?

[eddelbuettel]

BTW I was 'this month old' when I heard about the boo-boo with min here. We need to be careful.

> Rcpp::evalCpp("std::numeric_limits<int>::min()") # normal
[1] NA
> Rcpp::evalCpp("std::numeric_limits<int>::max()") # normal
[1] 2147483647
> Rcpp::evalCpp("std::numeric_limits<int>::min() * 1.0") # also normal
[1] -2147483648
> Rcpp::evalCpp("std::numeric_limits<int>::min()") # R special
[1] NA
> 
> 
> Rcpp::evalCpp("std::numeric_limits<double>::min()") # WTF: positive number
[1] 2.22507e-308
> Rcpp::evalCpp("std::numeric_limits<double>::lowest()") # normal
[1] -1.79769e+308
> 
> Rcpp::evalCpp("std::numeric_limits<int>::lowest()") 
[1] NA
> Rcpp::evalCpp("std::numeric_limits<int>::lowest() * 1.0") 
[1] -2147483648
> 

Mind you we are not running overflow for double, are we? If it is only for int aka int32_t it may be less complex than I feared.

[eddelbuettel]

covr runs R CMD check which goes via tests/tinytest.R as usual so it should. I recently added another clause there because we had to break mold and upload 'four part' fix up versions like 1.1.0.8 to CRAN. Maybe I borked the logic / started listening to the wrong environment variable. I will debug in a side thread.

[Enchufa2]

Mind you we are not running overflow for double, are we?

No, we are not. The overflow checks are enabled only for integral types. Because doubles do not overflow, they become Inf.

[eddelbuettel]

only for integral types

So int as we don't really do addition on character or bool. And given that Sugar is on SEXP we do not worry about, say, uint8_t do we?

[eddelbuettel]

I validated the assumptions in tests/tinytest.R in another debug repo I use for CI, and found no issue. So if coverage really does not run for Sugar then I do not know why. There is one exclusion line in codecov.yml but I do not think it should bite. Hm.

[Enchufa2]

So int as we don't really do addition on character or bool. And given that Sugar is on SEXP we do not worry about, say, uint8_t do we?

Correct.

[Enchufa2]

I validated the assumptions in tests/tinytest.R in another debug repo I use for CI, and found no issue. So if coverage really does not run for Sugar then I do not know why. There is one exclusion line in codecov.yml but I do not think it should bite. Hm.

I think that the coverage we see is just what calls into Rcpp.so, and nothing else. Because all tests compile cpp code via sourceCpp into a temporary directory, and this doesn't even get the required -O0 --coverage flags. So the current percentage is very misleading, and fixing this is far from trivial: we would need to compile all cpp code in tests in the package tree with the coverage flags, and then instruct covr to include additional paths to look for gcov files (because I think it only looks into the src folder by default).

But anyway, this is another matter. We can discuss this further in another issue.

[eddelbuettel]

Good thinking. Sounds like a testing / coverage package having all the tests in src/ may help. If we care.

I never know what to of coverage. It's a metric, and as Goodhart told us decades ago, of course it is being gamed.