`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.