Fixing code quality issues with static analysis

[de-sh]

Hi, I was going through the code and decided to run it through DeepSource, and found some interesting results. You might want to have a look for yourself as it detected 292 performance issues, 641 bug risks and 116 potential security vulnerabilities. Consider taking a look at the report generated by DeepSource.

I was considering opening a PR to solve these issues, shall I?

[wyli]

is it possible to check every pull request and report automatically?
if you want to resolve those please make separate PRs for different types of issues (so that they are easy for review)

[tvercaut]

In previous projects I worked on, we had an assert_or_raise function that would assert in debug mode but raise in production mode. The rationale was to avoid dramatic exits of the software or undefined/untested behavior (as asserts are typically simply disabled by -O or explicitly setting __debug__ to false in python or #define NDEBUG in C++) in case the asserted condition was not met during real uses. A more graceful error message could be shown to the user in many cases.

[srijan-deepsource]

but I guess we can set some config to filter out the tests directory

@ericspod That is right. DeepSource silences many issues that don't make sense in test files. We just need a glob to recognize the files.

Also, about assert statements: the idea behind the issue is the same that @tvercaut mentioned.

[rijobro]

Hi @tvercaut, great idea, but I guess we wouldn't want to change all assert to assert_or_raise. In previous code I worked on (C++), we had an assert when accessing array data to check that each of the indices were in bounds. This was considerably slower, and was only really necessary when debugging.

[tvercaut]

@rijobro Indeed, in time critical code, we also were not converting asserts into exceptions but just skipped these when in production mode.

[ericspod]

@srijan-deepsource OK makes sense, that looks like an easy change. My feeling with asserts though is whenever you would want an exception raise in production you would want that in testing as well. Assert is for those cases where you would want a check in test but not in production because your code is considered correct and the check isn't needed. This is a minor point but in general the feedback from the tools looks very helpful.