Stricter validation of return value from a rule's create() function

[bmish]

Current behavior

ESLint currently allows objects, arrays, functions, and literal values to be returned by a rule's create() function. I suspect it's unintentional that we allow anything but objects right now.

Rule visitor functions should be defined in a returned object. For someone wanting to bail out of a rule early, the current convention is to return an empty object:

module.exports = {
    meta: {
        // ...
    },

    create(context) {
        if (foo) {
            // bail out since rule doesn't apply
            return {};
        }
        return {
            BlockStatement(node) {},
        };
    }
};

Proposed behavior

Require the return value from a rule's create() function to be one of the following:

Return Value	Description	Current Behavior	New Behavior
{ /* visitors */ }	standard object of visitor functions	Rule executes	No change
{}	empty object used for bailing out early	Rule no-ops	No change
null / undefined		Exception (see below)	I think it would actually be useful to allow these as a means to bail out early as they could be considered a more elegant way to signal no-op behavior vs. returning an empty object (note: this is an optional part of this proposal but could be omitted in favor of only allowing empty objects for this purpose)
All other data types (arrays / functions / literal values, etc)		Rule no-ops	Exception (see below)

Here's the exception referenced in the table:

Error: The create() function for rule my-rule did not return an object.
Occurred while linting file.js

Links

Originally posted by @bmish in #16075 (comment)

See more context in #16075 where I added some messaging around this when the wrong return type is provided.

[nzakas]

Can you explain what currently happens for various types of values and then explain what you would like to happen instead?

When you say “not allowed”, does that mean an error is thrown? Something else?

[bmish]

@nzakas good point, I updated the description to include a more detailed table of the return types.

[nzakas]

Thanks that’s very helpful.

Some thoughts:

• For null/undefined, I feel like the current behavior is correct. While yes, someone may want to exit early, I tend to think of that as an edge case. I don’t think it’s a regular occurrence for someone to create a rule that doesn’t return listeners. I think the more common case is that this was an error, so throwing a better error seems like the right choice.
• For other data types, I can see an argument for throwing an error for primitive values. For other object types like arrays and functions I’m not entirely sure. In principle it seems like throwing an error makes sense since there’s no reason to do that when you can just return an object, but I’m not sure if there’s some use case we aren’t thinking of.

So overall, I think I’m all cases we want to err on the side of helping the developer identify unintentional errors with clear messages.

[bmish]

@nzakas okay, given your preference, I'm fine with keeping the current behavior of throwing an exception for null/undefined. The benefit of this choice is that we maintain only a single way return {}; of bailing out early. I have updated the proposal to reflect this.

Regarding other data types, I'm still unclear why/how arrays or functions could be an acceptable return type here, given that we use it as an object. If someone is somehow getting by using an array/function with additional properties on it, this seems like a good chance for us to catch that so they can fix it up into a standard object.

[nzakas]

If arrays or functions have own properties defining visitors, they will still work today. Why someone would want to do that is beyond me, but it’s not really an error condition. These are still just objects so I’m not sure if there’s a good reason to make them an exception case, but we can see what others think.

[mdjermanovic]

My thoughts:

• undefined - this could indicate a bug, in particular forgetting to return visitors in some code paths or at all. I think the current behavior is more useful than accepting undefined and silently doing nothing.
• null - this is less likely to be a bug, so it might be an alternative way to say that there are no visitors, but I'd still keep this simple and allow only a single way return {}.
• Primitive value - this certainly looks like a bug, I'm 👍 to throw an error.
• Array/Function - this looks a lot more like a bug than an intention. I would support throwing an error, but this might need an RFC to get attention.

[bmish]

@mdjermanovic I agree with your thoughts (throw on everything except an object).

However, I am still a bit perplexed by the concern around disallowing arrays/functions. In general, I believe:

1. We should be stricter in validating data types across our APIs, especially to help catch usage mistakes, and that we can add validation in major releases (as we have done recently with RuleTester and other APIs).
2. Increasing strictness can include disallowing broader types (e.g. arrays/functions) that were previously tolerated even though the original intention was to only accept a more specific type (e.g. object).
3. Increasing strictness to match the documentation generally shouldn't require an RFC.

In the ESLint Working with Rules documentation, we specify that the create function "returns an object", which to me positions this as an example where we could safely increase strictness. If needed, we could also add a warning about this before implementing the breaking change.

Note: if there's still disagreement, I could try to put together an RFC at some point, but that seems like a lot of overhead for what could be just one of many similar type-strictness improvements we could make.

[nzakas]

In general, breaking changes require an RFC to get as much feedback as possible.

Strictness is useful for finding conditions that will not work. My concern is that arrays and functions are just specific types of objects (fulfilling what is specified in the documentation) and will also currently work without error. I’m not sure that there’s a benefit to rule developers to disallow something that currently works even if it is unusual.

It’s possible you are right, and this really is an error condition, but then why are we cutting off at testing for arrays and functions? Why not maps and sets? Why not proxies? Requiring the return value constructor to be Object is another possibility, but that still seems a bit overboard to me.

It also seems like maybe this type of check is more appropriately done as a build time check via TypeScript than a runtime check?

@btmills still looking for feedback from you here.

[bmish]

...why are we cutting off at testing for arrays and functions? Why not maps and sets?

This is a good point. I think the reasoning comes from how one pattern commonly found on the internet to "check if a variable is an object" goes like this:

if (
    typeof yourVariable === 'object' &&
    !Array.isArray(yourVariable) &&
    yourVariable !== null
) {
    executeSomeCode();
}

As you point out, the array part of this could be considered arbitrary/incomplete given that it's just one of many possible data structures.

Given this, would you support enforcing a simple typeof yourVariable === 'object' check?

Regarding TypeScript, I'd love to take advantage of it, but how would this come into play given that ESLint isn't written in TypeScript?

[github-actions]

Oops! It looks like we lost track of this issue. What do we want to do here? This issue will auto-close in 7 days without an update.