Warn on usage of the result symbol (feature req)

[arnetheduck]

The usage of the result symbol has resulted in numerous security issues on our codebase that trivially would have been made visible by more explicit forms of initialization, and so we've deprecated its use.

It would be useful for us to have a warning that triggers when it's used, so as to make usage of it explicit and detectable at compile time for the whole codebase (vs grep which is much harder) with the option to disable for certain modules (legacy, 3rd party etc).

See also https://status-im.github.io/nim-style-guide/03_language.html#result-return - it's understood that some people prefer to use result - this warning is not for them (it can be disabled by default).

[Araq]

I strongly prefer to enforce a Java-like "definite assignment" ( https://docs.oracle.com/javase/specs/jls/se6/html/defAssign.html ) analysis. We already have this in the compiler, but it's only enabled for .requiresInit typed variables. I would make this the default. Would that be equally acceptable or do you insist on "result is bad"?

[arnetheduck]

For our codebase, it's gone so far / happened so often that "result is bad" at this stage: there are very few tangible benefits but significant risks.

requiresInit solves a slightly different problem - it works per type meaning you must have access to the source type to change it and typically requires that the type is designed for it with appropriate helpers - it's a great step forwards, but we've also had practical difficulties applying it beyond toy examples - things like serialization, case objects, certain assignment patters become difficult to express, so this requires more work before it will be widely applicable. It also doesn't cover ref which instead could use not nil analysis in addition to init.

Avoiding result on the other hand happens at the call site, which is where we want it to happen because we can improve the codebase step by step where it matters the most. Also, requiresInit doesn't make visible enough several other patterns that are prevalent in code that uses result - overwriting values in if jungles, templates etc. It does help the compiler to be more strict but it doesn't help a human reviewer understand the security implications of a refactoring with the same clarity that more explicit options do.

It is also confusing and problematic with shadowing, templates, closures and other parts of the language and the issues often appear when refactoring code as unintended side effects - at this point, we prefer an explicit marker that a variable is being initialized and later returned, either using the return keyword or implicit expressions, depending on the complexity of the code.

Finally, every feature has a mental cost to parse and understand it - with 3 ways to return things in Nim, cutting one out simplifies understanding code when collaborating and reviewing and makes the code less exposed to compiler bugs because we now have more uniform usage that stresses certain code paths more which helps discover bugs in those paths more easily. The other two ways of returning things have tangible benefits in the form of explicitness and strictness without making further changes to the language.

We have similar policies in place to prefer let over var etc in order to increase visibility of risky practices - mutability should as much as possible be visible at the call / usage site and avoiding result this is just one expression of that more general policy.

[Araq]

Can't say I agree -- a warning that is opt-in tends to cause much more problems. You quickly need push warning[ResultUsed]:off within a template/macro so that you can call into the stdlib which does use result. Definite assignment analysis which is not opt-in (I once patched the stdlib and Nim compiler to allow for bootstrapping with this feature turned on) but prevents many of your problems with result is the better solution. Yes, it doesn't solve all your gripes with result, but then it also applies equally well to every other local variable. That includes cases where the programmer used an easy workaround like var res: T; res = f(res); return res in order to fullfill your new "no result variable" policy.

[arnetheduck]

not opt-in

This is coming at it from from a more pragmatic angle - an all out assignment analysis that does what it's supposed to and is turned on by default doesn't sound like something that Nim easily can introduce without breaking lots of code (sometimes legitimately exposing latent bugs, sometimes not)?

warning that is opt-in

this is mostly an acknowledgement that some people really like result - we have the policy based on our experience that it frequently causes issues in critical code and introducing it simplifies life in large codebase like ours.

an easy workaround

sure - I mean it's complementary to other policies and there's always a way out - in the style guide we strive to establish safe defaults that promote being able to understand the code in review and refactor it later, regardless if you're the original author or not - for most constructs in Nim, there exists a legitimate use case and we leave the door open for people to step through when they're aware of what waits on the other side.

That said, there are of course people that will take the guide literally and write poor code just to rebel against the rule - that's fine as well - it's plainly visible and explicit at that point. I also generally agree that the rule is a bit blunt - sweeping rules like this tend to be blunt and catch some legitimate cases as well - result in particular however far beyond the tipping point because its usage has repeatedly caused significant damage that would have been plainly visible with other constructs, and has had significant bugs in both compiler, libraries like asyncdispatch and our code - from our point of view there are already 2 other more explicit and easier to understand constructs with fewer problems.

All that said, I would also like to have good DAA - it's great for many things, specially when async comes into play, and would love for it to make it into the compiler and language - I see us using it as a complement to the result warning, since I think we both agree they solve slightly different problems (with some overlap).

[arnetheduck]

You quickly need

this reminds me that it would be nice to be able to disable warnings per package :) nim std lib is after all just one more package - a large one, granted, but can be treated mostly the same.

[Araq]

Already warnings are only emitted for the "main" Nimble package that you compile. I don't know if the feature is worth its costs, the error reporting logic inside the compiler isn't something to be proud of. ;-)

[arnetheduck]

well, I see this issue as a litmus test and precursor for more linting - we have a whole list of candidates that are allowed by the language but terrible ideas in actual code, and would eventually build on this support. A warning might be too strong, perhaps a style hint is more appropriate but it's really the same mechanism (along with observable side effects etc) that would need an explicit error-and-warning-collection mechanism - I imagine it's also part of the same feature set that enables error checkers to highlight the whole file instead of stopping at first error.

result is just a particularly blatant example where the glass has seen an overflow many times too many now.

[dom96]

FWIW I think the definite assignment feature would be brilliant to have, I've used a couple languages now that implement it and it works quite well.

[Varriount]

@arnetheduck For what it's worth, I do like "result"... However I think I'd be equally happy if it was just a stylistic convention too.

In non-Nim code I write, I often prefer using a "result" variable, just because it makes the data flow a tad more explicit.