New: prefer-promise-reject-errors rule (fixes #7685)

[not-an-aardvark]

What is the purpose of this pull request? (put an "X" next to item)

[x] New rule

See #7685

What changes did you make? (Give an overview)

This adds the no-reject-literal rule as discussed in #7685, and enables it in the ESLint repo for dogfooding.

Some of the logic from this rule is shared with no-throw-literal, so that logic has been moved out of no-throw-literal and into ast-utils.js.

Is there anything you'd like reviewers to focus on?

I'm wondering whether we should have the rule report object expressions, e.g.

Promise.reject({ foo: 1 });

Advantages of reporting that case:

• { foo: 1 } cannot be an instance of Error.

Disadvantages of reporting that case:

• { foo: 1 } is not actually a literal, so the name of the rule might end up a bit misleading.
• Reporting that case would be inconsistent with no-throw-literal, which does not report throw {foo: 1}.

The current implementation does not report object expressions.

[mention-bot]

@not-an-aardvark, thanks for your PR! By analyzing the history of the files in this pull request, we identified @IanVS, @mysticatea and @doberkofler to be potential reviewers.

[mysticatea]

I think we can use context.getDeclaredVariables(node) instead of context.getScope().variables to simplify the finding logic (I guess WeakSets are unnecessary).
If the node is a function, context.getDeclaredVariables(node) returns an array which includes the variables of the function name and parameters.

[platinumazure]

Left a few thoughts, but this looks pretty good so far.

[platinumazure]

Minor not: Consider removing "instead" at the end of this line, which may falsely imply that these rules have no point in being enabled together.

[platinumazure]

s/not/nit, sorry.

[platinumazure]

I'm a bit concerned about this logic- undefined can be redefined, and further other variables could be declared-but-not-defined for the same issue. Maybe it would be better to use escope to try to find a definition with no initialization instead?

[mysticatea]

As far as I know, other rules also do not check shadowing of the undefined variable to cover the old well-known pattern:

;(function(undefined) {
    // do something
}())

[platinumazure]

@mysticatea I have no intention of making that pattern unusable. I just want to use escope to check that whatever variable is used is definitely undefined (in value), rather than looking for the name "undefined".

[not-an-aardvark]

I added this check because no-throw-literal also has a special case for it (see here).

Personally I think the rule is better with the check, because undefined is almost always going to be the same as not passing any argument to Promise.reject. In my opinion, this is one of those cases where if you do weird things such as creating an Error object called undefined and rejecting a Promise with it, you're on your own and the rule shouldn't become worse for the majority of undefined cases just to handle that outlier.

[platinumazure]

I don't see this as a blocker- let's not change this. I'll follow up with a new issue later.

[platinumazure]

Can all this be extracted to one or more helper functions?

[platinumazure]

Do we want to add object expressions here?

[not-an-aardvark]

I assume you're referencing the note at the top of this PR. What do you think the function should return for object expressions?

[platinumazure]

False, generally (my assumption is the Error constructor crafts the stack trace, so an object with a message or what have you will usually not suffice).

[not-an-aardvark]

I would be fine with that behavior, but I'm wondering if it would cause confusion because the rule is called no-reject-literal and object expressions are not literals.

Also, I think that would be a breaking change for no-throw-literal, (unless we explicitly made no-throw-literal more lenient by allowing ObjectExpressions).

[platinumazure]

One possible documentation issue, otherwise LGTM.

[platinumazure]

I don't see a period at the end of this line, but it could be my display. Disregard if this is inaccurate.

[platinumazure]

I don't see this as a blocker- let's not change this. I'll follow up with a new issue later.

[eslintbot]

LGTM

[platinumazure]

I can respect the possible breaking change argument. No need to change this.

…

On Dec 4, 2016 3:38 PM, "Teddy Katz" ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In tests/lib/ast-utils.js <#7689>: > + foo: true, + "new Foo": true, + "Foo()": true, + "foo`bar`": true, + "foo.bar": true, + "(foo = bar)": true, + "(foo = 1)": false, + "(1, 2, 3)": false, + "(foo, 2, 3)": false, + "(1, 2, foo)": true, + "1 && 2": false, + "1 && foo": true, + "foo && 2": true, + "foo ? 1 : 2": false, + "foo ? bar : 2": true, + "foo ? 1 : bar": true I would be fine with that behavior, but I'm wondering if it would cause confusion because the rule is called no-reject-literal and object expressions are not literals. Also, I think that would be a breaking change for no-throw-literal, (unless we explicitly made no-throw-literal more lenient by allowing ObjectExpressions). — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#7689>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AARWen-1ezY5fjFRGGs-h-PD03ms1vU3ks5rEzLTgaJpZM4LDLYz> .

[not-an-aardvark]

I'll add the ObjectExpression case to our 4.0.0 to-do list.

[not-an-aardvark]

@mysticatea I noticed you left a comment on this PR (which I think I've addressed), but you didn't leave a "Request Changes" review initially. Do you want to review this PR again, or do you think it's ready to be merged? (There's no rush, but I don't want to merge it if you still have changes to suggest.)

[ljharb]

This should be totally fine - rejecting with a literal is a problem, but rejecting with nothing (simply to be able to trigger a .catch) is totally legit imo.

At the very least, could this be an option, so that I can use this rule while allowing Promise.reject()?

[not-an-aardvark]

Could you elaborate on your use case for Promise.reject()? In a stack trace that would show up as Uncaught undefined, which isn't very useful.

[ljharb]

My use case is when I'm using promises as flow control, and I don't care about the rejection reason.

[not-an-aardvark]

But if it's in a .then call, wouldn't throw accomplish the same thing?

To me, it seems like if you're using rejected Promises for something other than error handling, you would probably want to disable this rule anyway. But maybe I'm still misunderstanding the use case.

[ljharb]

There are plenty of use cases where you want to send users down the .catch path without providing an error reason, but your codebase will still have many places where you do care about the error reason, and would want to ensure that no literals are used.

[ljharb]

There are many parallels between promises and syntactic try/catch, indeed. Promises, however, are more powerful than that, and don't have to be limited to try/catch. What's the Promise.all or Promise.race parallel in try/catch?

[not-an-aardvark]

I feel like we've gotten a bit off-topic. I'd be glad to keep discussing the different things that Promises can be used for and their synchronous parallels, but maybe we should do that e.g. in a chatroom so that we don't spam peoples' inboxes.

At the moment, I'm still leaning towards 👎 for adding an exception for undefined as Promise rejection reasons, but I'm fine with waiting to see what the rest of the team thinks about it. (Alternatively, maybe it would be better to open a separate issue after this rule lands on master, since it would be a backwards-compatible change.)

[mysticatea]

I agree with @not-an-aardvark .
I don't think that this rule should allow undefined.

I felt the smell like the following code. It would ignore true errors also.

try {
    list.forEach(x => {
        if (x == null) {
            // this is an abuse of exceptions.
            throw null
        }
        doSomething(x)
    })
} catch (_) {}

[ljharb]

@mysticatea note that I'm not saying it should allow Promise.reject(undefined) - I'm saying that that is not at all the same as Promise.reject() because the latter has zero arguments, and the former one.

More specifically, Promise.reject() has no literals of any kind in it - so why is this rule, "no reject literal", concerned with it?

[mysticatea]

As the result, error handlers will 100% receive undefined.
It's one of what this rule want to prevent.

[ljharb]

new Promise(function (resolve, reject) {
  throw 'something bad';
});

this will reject too - should it be left for no-throw-literal, or should this rule cover it also?

[not-an-aardvark]

no-throw-literal already reports this, so I think it's probably best to not report it to avoid conflicts.

[mysticatea]

LGTM, thank you!

[platinumazure]

Regarding the conversation around Promise.reject() with no args: There is no throw equivalent there, so it seems like a mistake to try to go for strict equivalency there.

To me, the rule is "When rejecting with an argument, it should be an Error so that a stack trace can be used". This is the same as no-throw-literal: "When throwing something, it should be an Error so that a stack trace can be used". But you can't throw nothing.

Even if my reasoning is not persuasive, I'd like to call out an element of ESLint philosophy, which is this: ESLint is not opinionated. To me, forbidding Promise.reject() with no args due to lack of stack trace, when that is not rejecting with a literal or a non-Error (except undefined, due to how that works) and when there is no equivalent in throw, is an arbitrary opinion, not (necessarily) a best practice, and I don't believe we should make that decision for the user. I think we absolutely should provide an option that lets a user opt into this behavior, since it could be seen as a potential best practice, but actually enforcing this in a core rule with no way to opt out strikes me as too opinionated.

[not-an-aardvark]

Regarding the conversation around Promise.reject() with no args: There is no throw equivalent there, so it seems like a mistake to try to go for strict equivalency there.

This rule and no-throw-literal only care about the value of a thrown error, not the syntax that gets used to throw the error. So throw undefined is the throw equivalent to Promise.reject().

To me, forbidding Promise.reject() with no args due to lack of stack trace, when that is not rejecting with a literal or a non-Error (except undefined, due to how that works)..., is an arbitrary opinion.

But Promise.reject() is rejecting with a non-Error; it's equivalent to Promise.reject(undefined) (aside from exceptional cases where someone uses undefined as a variable). So it's as much of an issue as any other case reported by the rule. The goal of the rule is to prevent a Promise from rejecting with anything other than an Error object. Rejecting it with undefined is clearly in violation of this goal, so I disagree that reporting Promise.reject() and Promise.reject(undefined) is an arbitrary opinion.

I think we absolutely should provide an option that lets a user opt into this behavior, since it could be seen as a potential best practice, but actually enforcing this in a core rule with no way to opt out strikes me as too opinionated.

We do allow users to opt in; they can decide whether or not to enable this rule in their config. But once a user has opted into reporting non-Error rejection reasons, having the rule report Promise.reject(undefined) but not Promise.reject() based on an arbitrary syntactic distinction seems like clearly incorrect behavior.

[not-an-aardvark]

To expand on this a bit: I acknowledge that the rule name is a bit misleading. But the documentation for no-throw-literal clearly indicates that the rule cares about the value of the error, not the syntax used to create that value: From the no-throw-literal docs:

When it was first created, it only prevented literals from being thrown (hence the name), but it has now been expanded to only allow expressions which have a possibility of being an Error object.

This is why the rule currently reports statements like this:

throw (condition ? "foo" : "bar");

Clearly, the expression (condition ? "foo" : "bar") is not a Literal node. But that's irrelevant, because it can be statically determined that the thrown value will not be an Error object, and so no-throw-literal reports it. The predicted runtime value is the important part, not the ternary expression.

Similarly, the argument (or lack thereof) in Promise.reject() is clearly not a Literal node. But that's irrelevant, because it can be statically determined that the rejection reason will not be an Error object, and so no-reject-literal reports it. Again, these rules only care about what the value will end up being at runtime; they don't care about how the exception/rejection reason is created. So having different behavior for Promise.reject() and Promise.reject(undefined) wouldn't make sense, because those are simply two different ways to create the same Promise rejection reason.

Based on the confusion in this thread, I'd like to propose two changes to this rule:

• Rename it. The other rule is clearly only called no-throw-literal for backwards compatibility, so I don't think we should cause further confusion by copying that name. I think something like promise-reject-errors would be a good name, but we can bikeshed on that.
• Disallow object and array expressions as rejection reasons, in line with @platinumazure's suggestion earlier. Again, it seems like no-throw-literal only has that for backwards compatibility (or maybe it's just a bug), but I think we may as well make the behavior more robust.

[platinumazure]

I'm still not ultimately convinced that this rule should be an exact parallel to no-throw-literal. We should not be the ones who decide best practices, but rather we should defer to the community when the practices aren't objectively wrong. Thus I still think @ljharb's points deserve further consideration and should not be rejected (heh) outright.

It seems to me that if promise rejections are not the same as errors, we should not treat them the exact same in these rules (or at least, should provide options for reasonable uses).

I'm sorry to do this, but I'm now 👎 on this proposal until we can come up with something reasonable to satisfy @ljharb's use case.

[not-an-aardvark]

Thus I still think @ljharb's points deserve further consideration and should not be rejected (heh) outright.

I think I've given them consideration, though. The point of the proposal is to ensure that promise rejections always have stacktraces, to avoid confusing errors like this. @ljharb's use case explicitly creates Promise rejections that don't have stack traces, which goes against the purpose of the rule. I don't see a need to satisfy that use case in this rule, because it gets rid of the entire benefit that would be gained by enabling the rule. Someone who wants to use Promises like that can just as easily disable this rule.

If we want to have a discussion about why using Error values as Promise rejection reasons is a good practice, then I'm certainly open to doing that. But I don't think we should make this rule worse by adding an exception for code that completely goes against what the rule is trying to enforce.

[ljharb]

I want to be able, in the same codebase without monkeying with override comments, have Promise.reject(), but statically ensure that any value that is inside those parens is an Error object. I'd even want it to be stricter and not allow an identifier unless it was statically determined to have a local-scope-created new Error (or TypeError etc).

[mysticatea]

So, is this code invalid?
I'm thinking 👎 if this is invalid.

function foo(path) {
    return new Promise((resolve, reject) => {
        fs.readFile(path, (err, content) => {
            if (err) {
                reject(err)
                return
            }
            doSomething()
        })
    })
}

function foo(name) {
    return new Promise((resolve, reject) => {
        const cp = child_process.spawn(name)
        cp.on("error", (err) => {
            doSomething()
            reject(err)
        })
    })
}

[ljharb]

Good point - rejecting with the error from a nodeback is a primary use case of new Promise, so perhaps my stricter behavior would only apply to Promise.reject.

[mysticatea]

It sounds fair enough to me.
Hmm.

[not-an-aardvark]

I'd even want it to be stricter and not allow an identifier unless it was statically determined to have a local-scope-created new Error (or TypeError etc)

That would prevent things like this:

function createError() {
  if (someCondition) {
    return new FooError(args);
  } else {
    return new BarError(args);
  }
}

function businessLogic() {
  if (condition) {
    return doSomethingAsync();
  } else {
    return Promise.reject(createError());
  }
}

But I would be fine with an option for strict reporting (where the rule reports something if it's possibly wrong, rather than certainly wrong). My main problem is allowing Promise.reject() without an argument, because I think that causes the same issues as Promise.reject(notAnError);.

[mysticatea]

My position is:

• This rule should disallow Promise.reject() without an argument.
• I strongly oppose the relaxing option of Promise.reject(). I felt the use of Promise.reject() without an argument is one of bad practice since it might ignore true errors. (New: prefer-promise-reject-errors rule (fixes #7685) #7689 (comment))
  ESLint does not recommend a bad practice.
• I'm OK with the stricter option of Promise.reject().
  Just a concern is the pattern such as var err = new Error(); err.code = "FOO"; Promise.reject(err). But we can use subclassing, so I'm not worried much.

[ljharb]

eslint has rules that allow lots of bad practices, and leaves it up to the user whether to enable or configure the rules as such.

In the pattern var err = new Error(); err.code = "FOO"; Promise.reject(err) it should be trivial for eslint to know statically that err is in fact an error subclass created in the same scope, so that should be allowed.

If I'm explicitly enabling an option that allows Promise.reject(), then surely I know, better than the rule authors, what constitutes a "bad practice" in my codebase - that's the entire point of allowing rules to be configured.

[not-an-aardvark]

TSC Summary: no-reject-literal is an accepted rule proposal that aims to prevent Promises from being rejected with non-error values. There has been considerable debate about whether the rule should have an option to allow code like Promise.reject() (i.e. calling Promise.reject without explicitly passing in an argument). On the one hand, Promise.reject() is arguably useful for control flow, so an option like this could allow a user to return a rejected Promise without triggering the rule. On the other hand, calling Promise.reject() with no arguments causes the Promise to be rejected with undefined, which is the same thing that the rule is trying to prevent.

TSC Question: Should we add an option to allow Promise.reject() without any explicit arguments?

[ilyavolodin]

Per TSC meeting: We agreed to add a new option that would be off by default and would allow Promise.reject().

[not-an-aardvark]

As @ilyavolodin mentioned, the TSC resolution was to add an option for this, so I'll work on doing that.

Does anyone have thoughts on the two bullet points at the bottom of #7689 (comment)? I think the rule might be less confusing if we change the name.

[not-an-aardvark]

Since no one objected to the comment above, I'm planning to update this PR with the following changes:

• Rename the rule to prefer-promise-reject-errors (still up for bikeshedding)
• Make the rule disallow object literals: Promise.reject({ foo: 1 });
• Add an option to allow Promise.reject() with no arguments

[eslintbot]

LGTM

[not-an-aardvark]

Ok, I made the changes I described above. This is ready to be reviewed again.

I squashed the commits to make sure the commit message with the correct name makes it into the changelog. For reviewing convenience, here is a copy of the changes since the last review.

[gyandeeps]

Nit: Just curious if this filter and above filter can be combined into single filter statement?

[not-an-aardvark]

They could be -- do you think that would be better? Personally I think the code is clearer with the filters separated, but I'd be happy to change it if you feel differently.

[platinumazure]

You can probably get the best of both worlds by extracting both of those filters into a single predicate function and then using that function name, if you're worried about readability and you want to follow gyandeeps' suggestion.