fix: validate files and ignores elements

[fasttime]

This PR ensures that files and ignores properties of config objects are fully validated before they are accessed in the getConfig method.

This should fix issue eslint/eslint#17434, except that we may want to add tests to the eslint repo as well.

The list of changes in the current draft implementation is as follows:

• Extracted a new schema files-and-ignores from the base schema to validate only files and ignores.
• Added a new method assertValidFilesAndIgnores that uses the new schema.
• Validating files and ignores of each config (not just of the merged configs) in the normalize and normalizeSync methods.
• Schema validation now ensures that the files array is not empty.
• Removed other assertions that are no longer necessary.
• Fixed an error message reported when ignores has invalid elements.
• In the base schema, files and ignores validation is now a noop.

[nzakas]

Thanks for digging into this. I think this is generally the right direction. My only concern is that now we will be validating files and ignores twice: once initially before doing any matching and then once when we merge configs together, and that could add up in large config arrays.

What I think might make sense is actually to use a separate schema for the initial validation than the second. So in the initial validation inside of getConfig(), we would use the current validation scheme of checking array items but the schema that's used for merging could instead just noop the validate method for files and ignores. We know that all files and ignores are valid at that point, so there really isn't any reason to re-validate.

What do you think?

[fasttime]

Thanks @nzakas! You are correct, there is no reason to re-validate files and ignores. I added commit c61908e where I'm extracting a new schema to validate just files and ignores and keeping a noop for these properties in the base schema. I'm just not sure about the name of the new schema, so I chose files-and-ignores. We can change that if there's a better name.

[fasttime]

I've added another commit to make sure that validation fails when files or ignores is explicitly set to undefined. This is consistent with how other options are being validated.

[nzakas]

It looks like this is the only place that's validating files/ignores? I understand from the way the code is currently written that this will effectively be called before getConfig(), but that's relying on an implementation detail that could change in the future, nevermind being a bit confusing for people trying to learn (or re-learn) the codebase.

What do you think about changing assertValidFilesAndIgnores to cache the validations and then also call it elsewhere, maybe in getConfig()?

[fasttime]

Updated in 5108e26, thanks! Definitely a cleaner approach to call the validation function in getConfig() beforehand. I've added a flag to the dataCache data so we can keep track of config arrays that have already passed the files-and-ignores validation, and not validate them again.

[nzakas]

Thanks, this looks good to me. Would like @mdjermanovic to take a look before merging.

[mdjermanovic]

This works well when getConfig is called.

However, there are other public methods that might be called first. For example, I copied the build from this branch into node_modules of my local eslint clone, then added { ignores: [1] } to eslint.config.js and ran npx eslint lib:

Oops! Something went wrong! :(

ESLint: 8.47.0

TypeError: matcher.startsWith is not a function
    at C:\milos\eslint\node_modules\@humanwhocodes\config-array\api.js:336:17
    at Array.reduce (<anonymous>)
    at shouldIgnorePath (C:\milos\eslint\node_modules\@humanwhocodes\config-array\api.js:327:17)
    at FlatConfigArray.isDirectoryIgnored (C:\milos\eslint\node_modules\@humanwhocodes\config-array\api.js:1044:13)
    at deepFilter (C:\milos\eslint\lib\eslint\eslint-helpers.js:279:47)
    at Object.isAppliedFilter (C:\milos\eslint\node_modules\@nodelib\fs.walk\out\readers\common.js:12:31)
    at AsyncReader._handleEntry (C:\milos\eslint\node_modules\@nodelib\fs.walk\out\readers\async.js:89:50)
    at C:\milos\eslint\node_modules\@nodelib\fs.walk\out\readers\async.js:65:22
    at callSuccessCallback (C:\milos\eslint\node_modules\@nodelib\fs.scandir\out\providers\async.js:103:5)
    at C:\milos\eslint\node_modules\@nodelib\fs.scandir\out\providers\async.js:29:13

In this case, user still gets the same uninformative error as described in the original issue eslint/eslint#17434.

Perhaps we could do this validation in function normalize and function normalizeSync?

[nzakas]

Perhaps we could do this validation in function normalize and function normalizeSync?

We can, but that would shift where the validation errors are thrown from. Not a huge deal but something to be aware of.

[fasttime]

Thanks for the review @mdjermanovic. I overlooked that isDirectoryIgnored gets called also outside of getConfig in the case you describe.

Perhaps we could do this validation in function normalize and function normalizeSync?

Alternatively, we could validate files and ignores in other public members beside getConfig where the config is expected to be already normalized, i.e. get files, get ignores, isDirectoryIgnored and isExplicitMatch. The overhead should be negligible since the validation would run only once after checking that the flag is not set.

Existing unit tests would not need to be changed (they would if validation is shifted into normalize and normalizeSync).

Thoughts? @nzakas

[mdjermanovic]

Normalization already throws errors in some cases - when it finds an array/function item but arrays/functions are not enabled. I personally think it's fine to do some basic validation when normalizing the config array (after preprocessing configs). The basic validation would be that the items are objects and that files and ignores are valid, if present.

[mdjermanovic]

Also, if the config array contains a primitive value, for example:

// eslint.config.js
module.exports = ["eslint:reccommended"]; // typo

after this change, the error becomes:

Oops! Something went wrong! :(

ESLint: 8.47.0

TypeError: Cannot use 'in' operator to search for 'files' in eslint:reccommended
    at assertValidFilesAndIgnores (C:\milos\eslint\node_modules\@humanwhocodes\config-array\api.js:175:14)
    at FlatConfigArray.forEach (<anonymous>)
    at FlatConfigArray.getConfig (C:\milos\eslint\node_modules\@humanwhocodes\config-array\api.js:832:9)
    at FlatConfigArray.isFileIgnored (C:\milos\eslint\node_modules\@humanwhocodes\config-array\api.js:995:15)
    at C:\milos\eslint\lib\eslint\eslint-helpers.js:312:49
    at Array.reduce (<anonymous>)
    at entryFilter (C:\milos\eslint\lib\eslint\eslint-helpers.js:299:28)
    at Object.isAppliedFilter (C:\milos\eslint\node_modules\@nodelib\fs.walk\out\readers\common.js:12:31)
    at AsyncReader._handleEntry (C:\milos\eslint\node_modules\@nodelib\fs.walk\out\readers\async.js:86:20)
    at C:\milos\eslint\node_modules\@nodelib\fs.walk\out\readers\async.js:65:22

Before the change, the error was more informative: Error: All arguments must be objects.

[fasttime]

Before the change, the error was more informative: Error: All arguments must be objects.

Thanks! This should be fixed now in 21fb3c4.

We still have to decide where we want to validate files and ignores.

[nzakas]

Normalization already throws errors in some cases - when it finds an array/function item but arrays/functions are not enabled.

My point wasn't that the method didn't already throw errors in some cases, but rather that the type of error it throws is different than the types thrown by getConfig(). All of the config-level validation (via ObjectSchema) was being done in getConfig() whereas normalize() was handling more programmatic error checking. Maybe that's okay because files and ignores are always validated by ConfigArray and can't be overridden by a subclass -- I just wanted to call this out.

Alternatively, we could validate files and ignores in other public members beside getConfig where the config is expected to be already normalized, i.e. get files, get ignores, isDirectoryIgnored and isExplicitMatch. The overhead should be negligible since the validation would run only once after checking that the flag is not set.

I'm not a fan of littering validation throughout the class methods. I'd rather keep it centralized, so if that can't be in getConfig() then normalize/normalizeSync is my second choice.

[fasttime]

I'd rather keep it centralized, so if that can't be in getConfig() then normalize/normalizeSync is my second choice.

Fine! I will move validation of files and ignores into normalize and normalizeSync and update the unit tests accordingly.

[fasttime]

Update as per discussion. Please, have a look: c421b26.

[nzakas]

Can we rename it_ to something else? Not a fan of the dangling underscore. Maybe localIt?

[fasttime]

Done in 34f47d5.

[nzakas]

LGTM. Would like @mdjermanovic to re-verify.

[mdjermanovic]

LGTM, thanks!