Skip to content

Enhancement: no-misused-promises should not flag functions whose contents are wrapped in try/catch #9319

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
4 tasks done
benmccann opened this issue Jun 10, 2024 · 2 comments
Labels
enhancement: plugin rule option New rule option for an existing eslint-plugin rule locked due to age Please open a new issue if you'd like to say more. See https://typescript-eslint.io/contributing. package: eslint-plugin Issues related to @typescript-eslint/eslint-plugin wontfix This will not be worked on

Comments

@benmccann
Copy link
Contributor

Before You File a Proposal Please Confirm You Have Done The Following...

My proposal is suitable for this project

  • I believe my proposal would be useful to the broader TypeScript community (meaning it is not a niche proposal).

Link to the rule's documentation

https://typescript-eslint.io/rules/no-misused-promises/

Description

The no-misused-promises documentation does not explain the motivation for the rule, so I'm left to infer that it is because promise rejections are not handled properly. There are a couple references to no-floating-promises in the docs, which support this view. However, if a function is wrapped entirely in a try/catch then we know it can never error and so any triggering of this rule would be a false positive

Fail

/** @type {NodeJS.Timeout} */
let timeout;

/** @type {() => Promise<boolean>} */
async function check() {
	clearTimeout(timeout);

	// this line would fail because check is async and setTimeout expects a synchronous function
	if (interval) timeout = setTimeout(check, interval);

	const res = await fetch(`${assets}/${__SVELTEKIT_APP_VERSION_FILE__}`, {
		headers: {
			pragma: 'no-cache',
			'cache-control': 'no-cache'
		}
	});

	if (!res.ok) {
		return false;
	}

	const data = await res.json();
	const updated = data.version !== version;

	if (updated) {
		set(true);
		clearTimeout(timeout);
	}

	return updated;
}

// this line would fail because check is async and setTimeout expects a synchronous function
if (interval) timeout = setTimeout(check, interval);

Pass

/** @type {NodeJS.Timeout} */
let timeout;

/** @type {() => Promise<boolean>} */
async function check() {
	try {
		clearTimeout(timeout);

		// this can't fail since the contents of check is wrapped in a try/catch
		if (interval) timeout = setTimeout(check, interval);

		const res = await fetch(`${assets}/${__SVELTEKIT_APP_VERSION_FILE__}`, {
			headers: {
				pragma: 'no-cache',
				'cache-control': 'no-cache'
			}
		});

		if (!res.ok) {
			return false;
		}

		const data = await res.json();
		const updated = data.version !== version;

		if (updated) {
			set(true);
			clearTimeout(timeout);
		}

		return updated;
	} catch {
		return false;
	}
}

// this can't fail since the contents of check is wrapped in a try/catch
if (interval) timeout = setTimeout(check, interval);

Additional Info

No response

@benmccann benmccann added enhancement: plugin rule option New rule option for an existing eslint-plugin rule package: eslint-plugin Issues related to @typescript-eslint/eslint-plugin triage Waiting for team members to take a look labels Jun 10, 2024
@bradzacher
Copy link
Member

bradzacher commented Jun 11, 2024

no-misused-promises a sister rule to no-floating-promises, yeah.

  • NFP checks "expression statement" locations
  • NMP checks "expression" locations

NMP does a few extra checks for obviously wrong code:

const promise1 = Promise.resolve(true);
if (promise2) { // obviously completely incorrect
}

const promise = Promise.resolve({a: ''});
const spread = {...promise}; // obviously completely incorrect

But I assume you're specifically talking about the cases covered by the checksVoidReturn option.
Those checks are essentially enforcing the same thing as NFP, yes - preventing you from passing promises to locations where they are not awaited to ensure that your async work is correctly handled.

Taking one of the examples from the docs:

[1, 2, 3].forEach(async value => {
  await fetch(`/${value}`);
});

This is likely incorrect because the work is un-awaitable and the failures are un-catchable.

In your specific case of setTimeout - sure it's technically completely safe to pass your promise like that. But consider the general purpose case.

For example imagine this:

function loadingWithTimeout(cb: () => void, timeout: number) {
  setTimeout(() => {
    setUiToShowLoading(true);
    cb();
    setUiToShowLoading(false);
  }, timeout);
}

loadingWithTimeout(async () => {
  try {
    await someAsyncOperation();
  } catch {}
}, 100);

By passing an async function where one is not expected you've broken your code - your async operation completed well after the utility expected it to and now your UI has "torn" and in a potentially "undefined" / broken state.

It's not just about error handling - but error handling is a big concern, for sure.

@bradzacher
Copy link
Member

Essentially what you're asking for here is the ability to ignore "infallible promises". However TS (or rather JS as a whole) has no concept of an infallible promise.

So in order for us to encode this we would either need to invent a pattern that can be used to declare it, or we would have to implement analysis that can prove it.
The former is, in a way, a superset of the latter as you wouldn't want to declare a promise as infallible if it isn't actually infallible for obvious reasons.

The issue is that solving the later in the general case involves solving the halting problem - which is an unsolved problem. Put another way - we would need to invent a general-purpose algorithm which can analyse a function body to determine if it's possible for the code to throw an exception.

A similar thing has been asked for for NFP - and we've given the same answer as I'll give now -- we are not going to build such a system. Even solving for a small subset is complicated and difficult given you have to analyse code across function and (usually) module boundaries. It's quite an expensive task to do this.

#8404 is our answer to this - it will allow you to declare a set of type names that should never be reported by NFP. In that way you can do something like this

interface InfalliblePromise<T> extends Promise<T> {}
function infallible(): InfalliblePromise<T> { ... }

// not reported by NFP
infallible();

If there's desire we could also add this option to NMP. But you'd be the first request!

@bradzacher bradzacher closed this as not planned Won't fix, can't repro, duplicate, stale Jun 11, 2024
@bradzacher bradzacher added wontfix This will not be worked on and removed triage Waiting for team members to take a look labels Jun 11, 2024
@github-actions github-actions bot added the locked due to age Please open a new issue if you'd like to say more. See https://typescript-eslint.io/contributing. label Jun 19, 2024
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jun 19, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement: plugin rule option New rule option for an existing eslint-plugin rule locked due to age Please open a new issue if you'd like to say more. See https://typescript-eslint.io/contributing. package: eslint-plugin Issues related to @typescript-eslint/eslint-plugin wontfix This will not be worked on
Projects
None yet
Development

No branches or pull requests

2 participants