Usage of minimatch's matchBase prevents overrides from targeting project root files.

[rwjblue]

Tell us about your environment

• ESLint Version: 4.13.1
• Node Version: v8.6.0
• npm Version: 5.5.1

What parser (default, Babel-ESLint, etc.) are you using?
Default.

Please show your full configuration:

Configuration

module.exports = {
  root: true,
  rules: {
    'no-console': 'error'
  },
  overrides: [
    {
      files: ['index.js'],
      rules: {
        'no-console': 'off'
      },
    }
  ]
};

What did you do? Please include the actual source code causing the issue, as well as the command that you used to run ESLint.

Files present:

// src/index.js
console.log('no-console is disabled due to overrides improperly matching nested file');

// index.js
console.log('no-console is properly disabled');

Command Ran:

eslint index.js src/index.js

I've created a repo to make testing this easier:

git clone git@github.com:rwjblue/__match-base-issue.git
cd __match-base-issue
yarn
yarn test

What did you expect to happen?

Expected output:

/path/to/src/index.js
  1:1  error  Unexpected console statement  no-console

✖ 1 problem (1 error, 0 warnings)

What actually happened? Please include the actual, raw output from ESLint.

No errors were reported.

---

Detailed Explanation

The new overrides feature added in ESLint 4.1.0 (#8081) is awesome (massively better than "sprinkling" config around in a repo or in specific files via comments). Unfortunately, there is a hole in the overrides support due to a small implementation decision.

When the overrides are processed to determine if each individual override matches or does not match a given file, the matchBase option is passed in to minimatch and the includes/excludes are processed.

The code that does this can be found here, but is also included below for explanation purposes:

function pathMatchesGlobs(filePath, patterns, excludedPatterns) {
  const patternList = [].concat(patterns);
  const excludedPatternList = [].concat(excludedPatterns || []);

  patternList.concat(excludedPatternList).forEach(pattern => {
    if (path.isAbsolute(pattern) || pattern.includes("..")) {
      throw new Error(`Invalid override pattern (expected relative path not containing '..'): ${pattern}`);
    }
  });

  const opts = { matchBase: true };

  return patternList.some(pattern => minimatch(filePath, pattern, opts)) &&
    !excludedPatternList.some(excludedPattern => minimatch(filePath, excludedPattern, opts));
}

The usage of matchBase: true to minimatch means that patterns without a / in them will be matched against the filePaths base name (i.e. path.basename(filePath)). This allows matching "all JS files" as simple as *.js instead of using **/*.js. Unfortunately, this means that it is now
impossible to match files in the project root without also matching nested files with the same base name.

[j-f1]

Does using /index.js fix the issue?

[rwjblue]

Using an absolute path (or .. in the path) results in an error thrown (from the code in the link above):

Invalid override pattern (expected relative path not containing '..'): ${pattern}

[rwjblue]

I've updated the description with a better summary of the problem, along with a reproduction repo that makes it easier to test and step through the issue.

[platinumazure]

I'm not sure why we're using matchBase. I think it might be so we match .gitignore semantics as much as possible... at least that would make sense for how we handle .eslintignore. I'm not sure we need to match .gitignore semantics for overrides include/excludes though, I would want those to be more explicit as a user...

That said, I haven't delved into the code at all so there might be (i.e., probably is) something I'm missing.

[rwjblue]

I'm taking a stab at fixing using the following semantics:

• If the pattern to be matched starts with ./: remove ./ and do not use matchBase
• All other patterns use existing semantics of matchBase

This is a non-breaking change (I confirmed that ./anything.js cannot match anything today), but adds the ability to match root project files.

@platinumazure - To do this, I am adding unit tests for ConfigOps.pathMatchesGlobs (makes it much easier to confirm expected behavior with a given set of filePath, pattern, and excludes combinations). Would you prefer that I PR tests for the existing semantics separately from adding support for ./? That way it might make it clearer exactly what is changing when I actually make the changes to lib/config/config-ops.js...

[platinumazure]

If you think the current test coverage is insufficient, I think we would welcome a separate PR to shore up coverage. As you can see, this area of the code is pretty touchy. I'm sorry to ask, but would you mind separating those out? Thanks so much, we really appreciate you helping us look into this.

…

On Dec 20, 2017 8:50 AM, "Robert Jackson" ***@***.***> wrote: I'm taking a stab at fixing using the following semantics: - If the pattern to be matched *starts* with ./: remove ./ and do not use matchBase - All other patterns use existing semantics of matchBase This is a non-breaking change (I confirmed that ./anything.js cannot match anything today), but adds the ability to match root project files. @platinumazure <https://github.com/platinumazure> - To do this, I am adding unit tests for ConfigOps.pathMatchesGlobs (makes it much easier to confirm expected behavior with a given set of filePath, pattern, and excludes combinations). Would you prefer that I PR tests for the existing semantics separately from adding support for ./? That way it might make it clearer exactly what is changing when I actually make the changes to lib/config/config-ops.js... — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#9740 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AARWeqreCnOPGbjS92Fr-pj88hvCMwsNks5tCR7RgaJpZM4RH3Gi> .

[platinumazure]

Labeling as core bug for now, but if anyone on the team thinks this should definitely be regarded as an enhancement request, feel free to relabel.

[rwjblue]

@platinumazure - Not a problem at all! I totally agree that having some additional coverage will make review of the actual fix for this issue much easier. Submitted a battery of unit tests for ConfigOps.pathMatchesGlobs in #9744.

[rwjblue]

Now that the baseline tests are landed, I have submitted #9748 with the potential fix I mentioned above in #9740 (comment). I believe that this is convenient backwards compatible change...

[eslint-deprecated]

Unfortunately, it looks like there wasn't enough interest from the team
or community to implement this change. While we wish we'd be able to
accommodate everyone's requests, we do need to prioritize. We've found
that issues failing to reach accepted status after 21 days tend to
never be accepted, and as such, we close those issues.
This doesn't mean the idea isn't interesting or useful, just that it's
not something the team can commit to.

Thanks for contributing to ESLint and we appreciate your understanding.