fix: Fix embedding of rule option schema array so that $ref's work

[matwilko]

Prerequisites checklist

• I have read the contributing guidelines.

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

[X] Documentation update
[X] Bug fix (template)
[ ] New rule (template)
[ ] Changes an existing rule (template)
[ ] Add autofix to a rule
[ ] Add a CLI option
[ ] Add something to the core
[ ] Other, please explain:

What changes did you make? (Give an overview)

As documented at Options Schemas, ESLint will break $ref's in schemas when providing an array of schemas ($ref will work fine in explicit schemas).

These $ref's break because ESLint currently embeds these schemas directly into a new schema, and therefore the path that a $ref might have previously been referencing is no longer valid. It is technically possible to construct a working $ref by having it account for the new structure, but this relies on implementation details and shouldn't be supported.

JSON Schemas do however support bundling multiple schemas together into a single document, allowing them to separately process $ref's locally. This does require each bundled schema to have a unique $id, and any $ref's below will be resolved relative to that schema. If the $id is chosen carefully, the schemas can also reference each other via { $ref: '/n/some/json/path' }, so common definitions can be shared between them.

There's a few details I'm omitting here (but are in the code and the docs in the PR), but here's a before and after transform to show the change of layout:

const origSchema = [
    { enum: ['a', 'b', 'c'] },
    {
        type: 'object',
        properties: {
            someProperty: { $ref: '#/definitions/stringOrNumber' },
            someOtherProperty: { $ref: '#/definitions/stringOrNumber' }
        }
        definitions: {
            stringOrNumber: { anyOf: [ { type: 'number' }, { type: 'string' } ] }
        }
    }
];

const origTransformed = {
    type: 'array',
    items: [
        { enum: ['a', 'b', 'c'] },
        {
            type: 'object',
            properties: {
                someProperty: { $ref: '#/definitions/stringOrNumber' }, // This $ref is broken now!
                someOtherProperty: { $ref: '#/definitions/stringOrNumber' } // This $ref is broken now!
            }
            definitions: {
                stringOrNumber: { anyOf: [ { type: 'number' }, { type: 'string' } ] }
            }
        }
    ],
    minItems: 0,
    maxItems: 2
};

const newTransformed = {
    type: 'array',
    items: [ { $ref: '#/definitions/0', $ref: '#/definitions/1' } ],
    minItems: 0,
    maxItems: 2,
    definitions: {
        0: {
            $id: 'https://123456.eslint.org/0',
            enum: ['a', 'b', 'c']
        },
        1: {
            $id: 'https://123456.eslint.org/1',
            type: 'object',
            properties: {
                someProperty: { $ref: '#/definitions/stringOrNumber' }, // This $ref is relative to the original schema and still works!
                someOtherProperty: { $ref: '#/definitions/stringOrNumber' } // This $ref is relative to the original schema and still works!
            }
            definitions: {
                stringOrNumber: { anyOf: [ { type: 'number' }, { type: 'string' } ] }
            }
        }
    ]
};

For simple schemas without any $ref's involved, there's no performance penalty, as ajv will inline those schemas when compiling the validation function.

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

Since it's technically possible to make $ref's work currently (by relying on implementation details), does this count as non-backwards-compatible, as it can in theory break existing rule schemas? Relying on undocumented behaviour seems like the kind of thing it's ok to break to me, but I'm not sure what ESLint's appetite for that is. I'd anticipate that it wouldn't be a big problem since the docs pretty clearly warn off using $ref for array schemas entirely.

I've only added tests for the new $ref support in getRuleOptionsSchema, but #17198 includes a more comprehensive set of tests to ensure this change doesn't break existing functionality if that is merged before this. I'll rebase and properly merge the tests in if/when that PR is merged, in the meantime, feedback on this change by itself would be welcome :)

[linux-foundation-easycla]

• ❌ - login: @matwilko / name: Matt Wilkinson . The commit (9efdcf4, 8f99df7, 3e037b7, c4d5185) is not authorized under a signed CLA. Please click here to be authorized. For further assistance with EasyCLA, please submit a support request ticket.

[eslint-github-bot]

Hi @matwilko!, thanks for the Pull Request

The pull request title isn't properly formatted. We ask that you update the message to match this format, as we use it to generate changelogs and automate releases.

• The commit message tag wasn't recognized. Did you mean "docs", "fix", or "feat"?

To Fix: You can fix this problem by clicking 'Edit' next to the pull request title at the top of this page.

Read more about contributing to ESLint here

[netlify]

✅ Deploy Preview for docs-eslint ready!

Name	Link
🔨 Latest commit	c4d5185
🔍 Latest deploy log	https://app.netlify.com/sites/docs-eslint/deploys/646bda81c401a50008f9879b
😎 Deploy Preview	https://deploy-preview-17208--docs-eslint.netlify.app/extend/custom-rules
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

---

To edit notification comments on pull requests, go to your Netlify site settings.

[mdjermanovic]

Hi @matwilko!

Thanks for the PR, but I'm not in favor of this change. In my opinion, supporting $ref's in the array format doesn't justify the added complexity in the code and in the docs. The array format is an eslint-specific feature provided for simplicity. If one needs to use advanced features such as $ref, they can simply switch the schema to the full JSON Schema format:

const schema = {
    definitions: {
        stringOrNumber: { anyOf: [ { type: 'number' }, { type: 'string' } ] }
    },
    type: 'array',
    items: [
        { enum: ['a', 'b', 'c'] },
        {
            type: 'object',
            properties: {
                someProperty: { $ref: '#/definitions/stringOrNumber' },
                someOtherProperty: { $ref: '#/definitions/stringOrNumber' }
            }            
        }
    ],
    minItems: 0,
    maxItems: 2
};

[matwilko]

The array format is an eslint-specific feature

Agreed that it's eslint specific, but it also breaks the implied promise of "provide some valid JSON Schemas and they'll work". I think it's valuable to be able to be able to drop in potentially already working schemas and have things "just work", otherwise this corner case seems like a pit of failure.

I can definitely see how the ability to reference between the various array elements is probably a bit much - that kind of just fell out of the way I structured the definitions and would be easy enough to disable (so only internal $ref's would be allowed).

To my mind, the corner case should be eliminated, either by supporting it (and the schema changes necessary to do so aren't that complex), or there needs to be some logic that will reject arrays-of-schemas that have any $ref's in them (I'm not a fan of merely documenting cases that won't work and hoping developers read it when you can prevent them entirely!)

For the latter, it would be possible to construct a reduced meta-schema for the array case with the "advanced" features removed - perhaps just reducing to the basic types with none of the combining/referencing elements (oneOf, anyOf, $ref, etc.) and strictly checking properties, so you can only construct number/string/object/array checks. Would that be a reasonable direction if you want the array case to only cover "simple" cases?

[mdjermanovic]

I think that anyOf and oneOf work well inside the array form. We have some core rules with such schemas:

eslint/lib/rules/no-restricted-exports.js

Lines 29 to 90 in 5b68d51

	schema: [{
	anyOf: [
	{
	type: "object",
	properties: {
	restrictedNamedExports: {
	type: "array",
	items: {
	type: "string"
	},
	uniqueItems: true
	}
	},
	additionalProperties: false
	},
	{
	type: "object",
	properties: {
	restrictedNamedExports: {
	type: "array",
	items: {
	type: "string",
	pattern: "^(?!default$)"
	},
	uniqueItems: true
	},
	restrictDefaultExports: {
	type: "object",
	properties: {
	// Allow/Disallow `export default foo; export default 42; export default function foo() {}` format
	direct: {
	type: "boolean"
	},
	// Allow/Disallow `export { foo as default };` declarations
	named: {
	type: "boolean"
	},
	// Allow/Disallow `export { default } from "mod"; export { default as default } from "mod";` declarations
	defaultFrom: {
	type: "boolean"
	},
	// Allow/Disallow `export { foo as default } from "mod";` declarations
	namedFrom: {
	type: "boolean"
	},
	// Allow/Disallow `export * as default from "mod"`; declarations
	namespaceFrom: {
	type: "boolean"
	}
	},
	additionalProperties: false
	}
	},
	additionalProperties: false
	}
	]
	}],

[matwilko]

I think that anyOf and oneOf work well inside the array form. We have some core rules with such schemas:

See, now that I'm coming round to "array of schemas should be simple", I think that rule schema is kind of hard to read, at least in the sense that it's easy to miss the containing array and the implicit behaviour that entails - and I'd probably now suggest that that should graduate to a full object schema to explicitly spell out the behaviour 😄

Totally understand that it's not necessarily going to be possible to restrict the behaviour of arrays of schemas given the existing ecosystem, though I'd be interested to do a survey of existing plugins to see if the "advanced" behaviour is in use on a wider basis. I assume that kind of change would require an RFC though, if you thought it might be worth pursuing?

[mdjermanovic]

See, now that I'm coming round to "array of schemas should be simple", I think that rule schema is kind of hard to read, at least in the sense that it's easy to miss the containing array and the implicit behaviour that entails - and I'd probably now suggest that that should graduate to a full object schema to explicitly spell out the behaviour 😄

Totally understand that it's not necessarily going to be possible to restrict the behaviour of arrays of schemas given the existing ecosystem, though I'd be interested to do a survey of existing plugins to see if the "advanced" behaviour is in use on a wider basis. I assume that kind of change would require an RFC though, if you thought it might be worth pursuing?

I'm not sure if you're suggesting dropping the array form or limiting it to simple features only. Either way, while I agree that the array form might be more confusing (as it itself is not valid JSON Schema) than useful, I don't think we can make those changes at this point.

[matwilko]

Sure, I was just riffing on your "array-of-schemas should be simple" thought and seeing where it lead in terms of simplifying - let's drop that train of thought so we don't muddle the issue :)

As a more concrete change that would address the original case of $ref not working (without taking a dependency on implementation details), we could go the opposite direction to this PR and make the use of $ref in array-of-schemas illegal?

I know ESLint doesn't do any validation of the schema at runtime beyond the implied check that ajv does for it being a structurally valid JSON Schema, and so plugins can manage to specify schemas that will either silently become weaker ($ref's not working) or will just always cause an error when the config is loaded (e.g. schema: { type: 'object', properties: { ... } }).

As a starting point, I can go suggest some new rules over in eslint-plugin-eslint-plugin to lint out $ref's in array-of-schemas, and some other cases in the object-schema case that would always be invalid. That should at least deal with the problem at authoring time, and I could then update the ESLint docs to be a bit more forceful? (e.g. "you must not use $ref's in array-of-schemas)

Given that there seems to be movement towards tightening up schemas for rules in v9, would there be any appetite for going one step further and adding some enforcement at runtime of "valid schemas"?

A lot of the kinds of invalid schema possible today will cause an error at runtime anyway (e.g. the { type: 'object' } case) because there's no possible valid options, but the error will make the user think they've done something wrong in their config file, when the problem is really a rule authoring issue, and there's nothing the user can do.

A more informative error message pointing out that "Rule A from Plugin B is malformed, please go bother the plugin author" would probably be better than some cryptic message about the user's config that they can't fix.

Moving this enforcement to runtime also means invalid schemas will be picked up by RuleTester, adding another authoring-time check.

I realise this would be a breaking change to core, so would need an RFC, just floating the idea - I know that it might just be a "we already considered this in the past and it's a hard no" case.

[mdjermanovic]

We prefer not to validate schemas at runtime as there's not much the end user can do about it aside from turning the rule off, while the rule might actually be working perfectly well for them with their configuration for that rule.

On the other hand, we can consider adding more validations directly in RuleTester so that the problems can be detected at the authoring time. That would also be a breaking change, but we can certainly discuss it. If this sounds good to you, feel free to open an issue with a proposal.

[nzakas]

Just catching up on this, and I think all of the relevant points have already been made.

Overall I agree with @mdjermanovic that this change isn't worth the additional complexity. ESLint is ten years old, and making changes to how we do schemas now, while making things technically more correct, has the potential to cause problems for existing plugins. We already had to abort upgrading Ajv because it was more strictly following JSON Schema which led to more errors in existing rules.

I also agree with @mdjermanovic that we can always add more validation into RuleTester to catch potential schema issues. Please feel free to open an issue to discuss such changes if you'd like.

Thanks!