Result value must be used check

[s-panferov]

Hello.

Quite often while working with ImmutableJS data structures and other immutable libraries people forget that values are immutable:

They can accidentally write:

obj.set('name', 'newName');

Instead of:

let newObj = obj.set('name', 'newName');

These errors are hard to discover and they are very annoying. I think that this problem can't be solved by tslint, so I propose to add some sort of "you must use the result" check into the compiler. But right now I have no idea how I want it to be expressed in the language, so I create this issue mainly to start a discussion.

Rust lang, for example, solves a similar problem with #[must_use] annotation. This is not exact what we want, but it's a good example and shows that the problem is quite common.

[SPY]

+1

[kitsonk]

I think that this problem can't be solved by tslint

Why? Not using a return value is exactly what a linter is best at doing. Identifying non-syntatical errors that may lead to logic errors in the code.

[SPY]

Why? Not using a return value is exactly what a linter is best at doing. Identifying non-syntatical errors that may lead to logic errors in the code.

but linter does not have type information I suppose. so it will be very noisy about legal cases

[s-panferov]

Yes, we must rely on type information here, but tslint doesn't have an access to it.

[kitsonk]

but linter does not have type information I suppose. so it will be very noisy about legal cases

tslint has access to all the information that the TypeScript compiler has when compiling code, because it integrates to the language services of TypeScript. For example there are several custom rules that teams have Microsoft have made available for tslint and at least a couple of rules (e.g. no-backbone-get-set-outside-model, jquery-deferred-must-complete) that do specific code analysis on how specific libraries are used.

[mhegazy]

I agree with @kitsonk on this issue. it is perfectly valid to ignore the result of a call. so unless we introduce new syntax that marks these functions, this looks like a style restriction for specific teams and does not apply for the general population.

[s-panferov]

so unless we introduce new syntax that marks these functions

This is exactly what I'm asking for.

I really don't believe that it's a style restriction, because it breaks logic, not appearance. And this error is quite common and the harm is obvious. Sometimes a lot of time must be spent to find out why object field doesn't contain a value which I've set to it. And this applies at least to all people who uses immutable data structures (not only ImmutableJS) in some way. There are other potential use-cases like Result<Ok,Err> type, which must be always used to handle potential error cases.

[s-panferov]

@mhegazy, @kitsonk

The example you show doesn't use type information, it uses only syntax analysis:
https://github.com/Microsoft/tslint-microsoft-contrib/blob/master/src/jqueryDeferredMustCompleteRule.ts#L19-L30

In my example there is no any syntactical markers which can be used to find invalid usage, so we need information of type level.

I think it will try to write a formal proposal for this to explain what I'm taking about and why this is useful.

[mhegazy]

I still believe this is a style guideline more than a compiler error. i would assume some users want to opt out of the checking, or disable the error for some cases. this puts more in the vein of linting. i would assume something like #2900 to define the metadata on the function, and a linter to impose the rule.

[s-panferov]

@mhegazy this can really work with ambient decorators if tslint:

1. Can understand that in expression instanceOfObj.set('smth', 1) set is a function set of class Obj.
2. Can find it's declaration somewhere in another file where class Obj is declared.

Is this possible with tslint?

[mhegazy]

The compiler API exposes a checker object that can answer these questions, so i do not see why not.

[s-panferov]

@mhegazy but doesn't it require a second compilation pass for tslint?

[mhegazy]

it requires global analysis, yes.

[mhegazy]

Reopening for consideration, since we have got multiple requests for this.

[RyanCavanaugh]

Tracking at #8584

[simonbuchan]

This certainly got bounced around! I'm quite happy for this to be "just" a decorator and lint, but the --lib typings should be able to benefit from it too. Would declaration merging support adding design time decorators?

[abdulkareemnalband]

I think it should be implemented similar to c++ nodiscard, that both

1. A function return type is marked as must use
2. A type (class or interface or enum) can marked as must be used, so any function returning them by default is nodiscard

[TimWolla]

After having been burnt by a library updating a method to be const/pure in a new major version, whereas it modified state previously: +1

Calling pure methods without using the return value almost certainly is a bug. So if the library author was able to indicate this in some standardized way (e.g. the nodiscard as suggested above), TypeScript might've been able to prevent this issue from happening to me.

[nh2]

Here is another example of a refactoring error that this type of check would prevent:

Consider having an onclick handler:

const onMovementClick = (direction: "forward" | "backward") => () => { ... }

used like:

<div onClick={onMovementClick("forward") }>Forward</div>
<div onClick={onMovementClick("backward")}>Backward</div>

Now you refactor, adding some logging:

<div onClick={() => { log("forward");  onMovementClick("forward");  } }>Forward</div>
<div onClick={() => { log("backward"); onMovementClick("backward"); } }>Backward</div>

This refactor is wrong; your buttons now no longer have any effect and your web app is broken due to what's logically a type error that TypeScript did not help prevent. The correct code has some added () function calls:

<div onClick={() => { log("forward");  onMovementClick("forward")();  } }>Forward</div>
<div onClick={() => { log("backward"); onMovementClick("backward")(); } }>Backward</div>

Here, one needed to call f(someArg)() instead of f(someArg); the latter returns a function that is then unused.

If TypeScript had the ability to warn on unused return values, like Haskell and Rust can do with -Wall, this mistake would be impossible to make.

[LordOfTheDings]

What's the current status on this?

[gunnarmein-ts]

I agree that a "nodiscard" decorator would be very useful. As someone said above, these bugs a hard to find once someone put them in.

[ns-vpanfilov]

such decorator would also be useful if we want to return errors (similar to result in rust) instead of throwing errors. There was a similar request before, but it was closed since it is, supposedly, a duplicate of other issues: #38303

[emilioplatzer]

If the implementation of Result value must be used is like C++ [[nodiscard]] (that is a function can be marked as nodiscard and also a type can be marked as nodiscard then all functions returning that are nodiscard) then this can be used for detecting floating promises. Promise need to have nodiscard by default.

Also ther is need to be an explicit way to discard values. May be:

function discardReturnedValue<T>(valueToDiscard:T):void{}

then

async closeFile(f:FileHandler):Promise<void>{...}

discardReturnedValue(closeFile(f))

see #13376

[nh2]

A [[nodiscard]] as in C++ (callee-side) is not as useful as e.g. Haskell's -Wall compiler flag (caller-side-determined check):

It moves control to the writer of the function, instead of giving control over wardnings to the downstream project that uses them.

In C++, this makes [[nodiscard]] almost useless, as almost library code uses it. That introduces lots of bugs (e.g. ignored bool success return values). In contrast, those types of bugs do not exist in Haskell, where most users/projects choose to use -Wall to enable stricter warnings. It is also possible to turn these warnings into errors using -Werror, which can be useful in CI.

Further, a callee-side annotation would not fix key issues like my example (#8240 (comment)), as there the thing whose return value is ignored is itself a function call, and it's tough to find a place where you could put the annotation when partially applied functions are involved.

So I think a caller-side check that the user can enable as a warning is much better.

Even if a [[nodiscard]] callee-side feature was considered valuable, that should exist independent of caller-side warnings.

[xuhdev]

@nh2 I'm a bit confused. Isn't [[nodiscard]] in C++ a callee-side annotation? Just trying to understand your argument on the proposed TS feature.

[gunnarmein-ts]

@xuhdev I would say he means that any callee side annotation - and he points out that feature is, in C++ - would be useless unless it gets retrofitted onto every library function that needs it, and chances of that are zero. That's why it is more practical to have a caller-side mechanism so that those of us who care about it can use it when calling all kinds of libraries and be safe.

[laverdet]

I think it would be adopted. Developers of good software look forward to features like this which make their programs more resilient. It's like saying readonly is useless because no one uses it.

TypeScript also gives you escape hatches:

• Define your own .d.ts with types
• type MakeNodiscard<Fn extends (...args: unknown[]) => unknown> = Fn extends (...args: infer Args) => infer Result ? (...args: Args) => Nodiscard<Result> : never
• Send a PR to @types

[nh2]

I would say he means that any callee side annotation - and he points out that feature is, in C++ - would be useless unless it gets retrofitted onto every library function that needs it, and chances of that are zero. That's why it is more practical to have a caller-side mechanism so that those of us who care about it can use it when calling all kinds of libraries and be safe.

This is exactly what I meant.

[Tyler-Murphy]

This eslint rule might be useful: https://github.com/SonarSource/eslint-plugin-sonarjs/blob/master/docs/rules/no-ignored-return.md

And here's the source code if anyone wants to try to modify the rule so that it can be used for your own functions. I think eslint is flexible enough to read the jsdoc comments on a function and look for whatever tag you want to add, e.g. nodiscard.