Tslint: "Result value must be used" rule

[s-panferov]

Original issue and discussion https://github.com/Microsoft/TypeScript/issues/8240

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.

@mhegazy in the original discussion proposed to use ambient decorators. But this requires to perform not only syntactical analysis, but global type analysis too (using typescript checker API). I'm interested is this in scope of tslint? Or we need to create another linter tool which will work with types to resolve this issue?

[jkillian]

See also https://github.com/palantir/tslint/issues/1135, which is a similar suggestion for core JS functions

[s-panferov]

@JKillian yes, this looks similar. I want to know what tslint maintainers think about types-based checks in linter. I'm asking because I'm really interested to make this issue resolved and I can create a PR if this is in scope of tslint.

I'm an author of docscript and awesome-typescript-loader projects, so I know TS internals quite well to implement this.

[myitcv]

My two cents: this doesn't belong in core tslint because it's specific to the Immutable JS library (which we also use incidentally). It is probably best housed in the Immutable JS repo (or a sibling repo)

In any case, to implement this properly you're going to find yourself blocked on #680 - you will need full type information, which you don't have in tslint right now

[ScottSWu]

I'm looking to implement this lint check for core JS functions on a blacklisting basis. The idea is that a function would fail the check if:

1. The function's return value is non-void
2. The function is blacklisted
3. The return value is not used

(1) is now available through type checking.

(2) is up for discussion. An easy solution that is compatible with rule options and does not require extra code is to have an array of blacklisted function names. For example: [["Array", "concat"], ["string", "trim"]], where the first element could be the type of the caller or symbol name or other. Rule options could be appended to a list of built-in functions.

Some other possibilities are a generic type or a JSDoc annotation, which would be more robust but require changing code and .d.ts files. Ambient decorators would probably be the best option, but it doesn't look like they are in TypeScript yet.

(3) can be determined by walking along parent nodes until a certain node type is reached. Otherwise if an ExpressionStatement or SourceFile is reached first, then the return value is unused.

Thoughts?

[alexeagle]

For context, this is how we did it on my Java static checker project:
http://errorprone.info/bugpattern/CheckReturnValue

API maintainers mark the functions whose return values must not be ignored. As Scott says, we don't have https://github.com/Microsoft/TypeScript/issues/2900 yet, and we'd also need to change TS syntax to allow Decorators on functions. So I think this is out.

We should explore a generic wrapper type, eg.

type MustUse<T> = T;
function trim(s: string): MustUse<string> {
  return s.trim();
}
let p = trim(" hello ");

http://www.typescriptlang.org/play/#src=type%20MustUse%3CT%3E%20%3D%20T%3B%0D%0Afunction%20trim(s%3A%20string)%3A%20MustUse%3Cstring%3E%20%7B%0D%0A%20%20return%20s.trim()%3B%0D%0A%7D%0D%0A%0D%0Alet%20p%20%3D%20trim(%22%20hello%20%22)%3B

We could force users to import {MustUse} from 'some-package', or allow any symbol named MustUse like in that example (or whatever name we agree on).

JSDoc is a crappy last choice, IMHO.

Separately, we need to maintain our own list of APIs from the core/standard libraries whose return value must not be ignored. In Java we assumed that we'll never get a change upstream into those libraries to mark them. We might have better luck with TypeScript, but it would still take a long time to get agreement about how we mark these, discussion of how the type system should represent them instead ( cc @rkirov ) and then actually add them to lib/lib.es6.d.ts and so on.

[alexeagle]

Scott has a WIP for this, see https://github.com/ScottSWu/tslint/commit/21bed19ff5492fed07ba46ff95d298fb4cf59c7f

@JKillian any thoughts about the design questions?

[jkillian]

Hmm, I'm curious as to why you find JSDoc a bad choice?

/** @MustUse */
function trim(s: string): {
  return s.trim();
}

// disallowed:
trim(" string ");

// allowed?
trim(" string ") || someFunc();

// allowed
const newStr = trim(" string ");

If we required a MustUse<T> type, would users define it themselves or import it from some library? Would the type be too confusing to interact with if it were part of a public API?

[alexeagle]

JSDoc has a few drawbacks:

• most editors don't know comments contain special stuff. No spellcheck, no type-checking. Eg. accidentally put @Mustuse instead of @MustUse
• by putting it in a type position, we can use TS typechecker to determine if a method returns a MustUse. In JSDoc, we have to go find the declaration of the method, and look through the JSDoc for the annotation, which could appear anywhere in the comment? I realize we already have this situation for @internal (and Angular's @Annotation, see https://github.com/angular/tsickle/blob/2c0645347dd0f243c925c4a38818525252636b98/src/decorator-annotator.ts#L49 ) so it's not without precedent
• In the type position, it's better documented. Actually I expected in the playground link above, that completion on trim would show in the tooltip that it's a MustUse, which would be nice to know while coding, but it only shows string :(
• Those of us with a lot of experience in Google's Closure Compiler get itchy when we see type information appear in comments.

Like you say, the drawback of a real type is that the user must either declare that awkward type line, or import a library to get it.

Having laid out the reasoning, I'd be fine to go either way.

[jkillian]

Given what you said, I'm still leaning towards JSDoc. It feels less invasive to me than adding a MustUse type - an actual type like that feels much more like a language-level feature to me and not something for us to implement as part of general-purpose TSLint. Plus, the language services not showing aliases is a big drawback, we lost a lot of the benefit of a type.

That being said, I'd guess that you have more experience and perspective here, if you still think the type is a better answer, I can probably be convinced

most editors don't know comments contain special stuff. No spellcheck, no type-checking. Eg. accidentally put

I think we could accept any capitalization of @mustuse

[alexeagle]

Oh, one other thing, we'll have to lookup (and probably memoize) the declaration of every function or method called in the program to see if the JSDoc contains the special bit. We'll have to see how it affects performance.

[sompylasar]

Why isn't that a core rule for all functions? (we have tslint ignore for corner cases)

Example: https://gist.github.com/arackaf/7826ff661db492c7b5d3ef95dd2afef4

A requirement to add a "must use" declaration (in any shape: change return type or add a comment) requires changing code you don't own hence cannot or should not change: typings for standard objects (Array.concat), async function return types (must be Promise), typings for libraries (immutable.js), etc.

[alexeagle]

We can totally change the typings for standard objects and libraries. We already do this as TypeScript adds features to allow libraries to be typed more precisely.

[sompylasar]

@alexeagle The fact we can, in general, doesn't mean we should for this usecase, on a per-app basis (that's how tslint is configured). Overriding the standard library or any other library type definitions is a workaround to update an app while the upstream libraries are lagging behind on the type definitions.

Unless MustUse is proposed to be a TypeScript compiler feature (which AFAIK is not the case, we're talking tslint now).

[JoshuaKGoldberg]

💀 _It's time!_ 💀

TSLint is being deprecated and no longer accepting pull requests for major new changes or features. See #4534. 😱

If you'd like to see this change implemented, you have two choices:

• Recommended: Check if this is available in ESLint + typescript-eslint ✅
• _Not Recommended: Fork TSLint locally_ 🤷‍♂️

👋 It was a pleasure open sourcing with you!

_If you believe this message was posted here in error, please comment so we can re-open the issue!_

[JoshuaKGoldberg]

🤖 Beep boop! 👉 TSLint is deprecated 👈 _(#4534)_ and you should switch to typescript-eslint! 🤖

🔒 This issue is being locked to prevent further unnecessary discussions. Thank you! 👋