feat: Implement SourceCode#applyInlineConfig()

[nzakas]

Prerequisites checklist

• I have read the contributing guidelines.

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

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

What changes did you make? (Give an overview)

The original intent of this commit was to implement SourceCode#getInlineConfig() as described in the Language Plugins RFC. However, once I started to implement this feature I realized that the design in the RFC wouldn't work and I had to make some changes. The specific problem is that I was thinking we could extract the inline config from the source code and then the core could use that to lint the source code. The thing I didn't realize is that a lot of the inline config actually changes the state of SourceCode by adding or modifying globals, which the core won't know about in a language-neutral future.

So, these changes amount to:

1. Added SoureCode#getInlineConfigComments() method - this just retrieves all the config comments so when inline config is disabled and warnings are enabled, the core can warn about all the config comments. This is only called when both inline config comments are disabled and warnings for inline config comments are enabled.
2. Added SourceCode#applyLanguageOptions() method - ESLint core calls this to pass in the calculated language options. This allows SourceCode to update itself as needed.
3. Added SourceCode#applyInlineConfig() method - ESLint core calls this so SourceCode will further update its state based on the inline config comments. In this case, it involves tracking down the global and exported definitions.
4. Added SourceCode#finalize() method - ESLint core calls this so SourceCode knows the core will not be passing in any further information before linting begins.
5. Because those last three methods change the state of SourceCode, I'm not allowing them to be called within rules. Instead of passing SourceCode directly, the core now creates a proxy that disallows calling any of those methods. The proxy is passed through to rules rather than SourceCode itself.

Refs #16999

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

Does this approach make sense?

Any concerns about the proxy approach?

Given that freezing SourceCode was intended to protect against modification in a rule, does using a proxy mean we don't need to freeze SourceCode anymore?

[eslint-github-bot]

Hi @nzakas!, thanks for the Pull Request

The first commit message 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"?
• There should be a space following the initial tag and colon, for example 'feat: Message'.
• The first letter of the tag should be in lowercase

To Fix: You can fix this problem by running git commit --amend, editing your commit message, and then running git push -f to update this pull request.

Read more about contributing to ESLint here

[netlify]

✅ Deploy Preview for docs-eslint canceled.

Name	Link
🔨 Latest commit	5a8363f
🔍 Latest deploy log	https://app.netlify.com/sites/docs-eslint/deploys/650db20743c54d0008e8db81

[eslint-github-bot]

Hi @nzakas!, thanks for the Pull Request

The first commit message 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"?
• There should be a space following the initial tag and colon, for example 'feat: Message'.
• The first letter of the tag should be in lowercase

To Fix: You can fix this problem by running git commit --amend, editing your commit message, and then running git push -f to update this pull request.

Read more about contributing to ESLint here

[eslint-github-bot]

Hi @nzakas!, 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"?
• There should be a space following the initial tag and colon, for example 'feat: Message'.
• The first letter of the tag should be in lowercase

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

[mdjermanovic]

Overall, I don't understand the intended usage of the new SourceCode methods in regard to eslint and eslint-disable-* comments.

getInlineConfigComments returns eslint and eslint-disable-* comments as well.

applyLanguageOptions processes eslint comments.

However, _verifyWithFlatConfigArrayAndWithoutProcessors still uses linter's own function getDirectiveComments for extracting eslint and eslint-disable-* comments and processing eslint comments. Shouldn't it switch completely to the new SourceCode methods?

[nzakas]

Overall, I don't understand the intended usage of the new SourceCode methods in regard to eslint and eslint-disable-* comments.

getInlineConfigComments returns eslint and eslint-disable-* comments as well.

Yes. To restate from my original comment: retrieves all the config comments so when inline config is disabled and warnings are enabled, the core can warn about all the config comments. This is only called when both inline config comments are disabled and warnings for inline config comments are enabled. So this method is retrieving the comments but isn't trying to evaluate them.

applyLanguageOptions processes eslint comments.

No. applyLanguageOptions applies data passed into the method. It does not process eslint comments.

applyInlineConfig processes eslint comments only and not disable directives. It's only adding globals, exports, and extracting rule configs so the core can apply those. Processing disable directives needs to be done in the core (to mark areas where rules should not be applied), and I'm saving that for a different step.

However, _verifyWithFlatConfigArrayAndWithoutProcessors still uses linter's own function getDirectiveComments for extracting eslint and eslint-disable-* comments and processing eslint comments. Shouldn't it switch completely to the new SourceCode methods?

Good point. getDirectiveComments can use getInlineConfigComments now. I can also split this into two functions, one for the eslintrc code path and one for the flat config code path to avoid duplicate functionality. I can update that. To the larger point, processing of disable directives need to be handled separately. As we discussed on the RFC, I'm not sure if the current way we structure the info about disable directives makes sense for all languages, so I need to look at that separately next.

[fasttime]

Just a note that I am still able to call disallowed SourceCode methods inside a rule like this:

sourceCode.__proto__.applyInlineConfig.call(sourceCode);

Or using another non-proxy instance:

const { SourceCode } = require("eslint");
const dummySourceCode = new SourceCode(sourceCode.text, sourceCode.ast);

dummySourceCode.applyInlineConfig.call(sourceCode);

[nzakas]

@fasttime of course, we can't completely block people from calling those methods if they really want to. We're just trying to let them know that if they do, we can't guarantee what will happen.

[nzakas]

I've been digging into fixing the bugs that @mdjermanovic pointed out, and I've discovered more complexity.

For inline config, we are doing one-off rule validation just on that config in eslintrc mode; in flat config mode, we actually don't have a way to do one-off rule validation, so I need to do some thinking around what that might look like.

[nzakas]

Notes on additional changes:

1. SourceCode#getInlineConfigComments() is now called SourceCode#getInlineConfigNodes() to make it more generic. Even though we are using comments right now, I'm not sure it's idiomatic in other languages where people might use decorators/attributes for such purposes. By specifying this as "nodes" it allows us to deal with any type of nodes in the future including comments.
2. SourceCode#applyInlineConfig() now returns an array called configs where each item is an object with a config property containing an actual config and node property that is a reference to the node (comment) that defines that config. The node property is used to output the location of the comment when there is a problem with the config.
   Note: I use config with just a rules property because I'm unsure if in the future we'd like to allow configuring more than just rules inline. This allows us the option of adding more properties to the config in the future. The other option is to replace config with rules directly and then add on additional properties to each object in configs if we need to add more information...I didn't like that approach because it will make a config-like object that isn't really a config, but definitely open to feedback.

[nzakas]

Also note: Node.js 19 appears to change the format of the JSON parse error message that we were relying on in a test, so I had to update that to use a regex so it will pass in all Node.js versions.

[github-actions]

Hi everyone, it looks like we lost track of this pull request. Please review and see what the next steps are. This pull request will auto-close in 7 days without an update.

[mdjermanovic]

Any concerns about the proxy approach?

It seems to significantly impact performance.

time npx eslint . without proxy:

real    0m41.127s
user    0m0.015s
sys     0m0.151s

time npx eslint . with proxy:

real    0m51.178s
user    0m0.015s
sys     0m0.151s

npm run test:performance without proxy:

Loading:
  Load performance Run #1:  230.6313ms
  Load performance Run #2:  280.6211ms
  Load performance Run #3:  299.4569ms
  Load performance Run #4:  216.506ms
  Load performance Run #5:  217.934ms

  Load Performance median:  230.6313ms


Single File:
  CPU Speed is 2695 with multiplier 13000000
  Performance Run #1:  4483.5138ms
  Performance Run #2:  4381.0608ms
  Performance Run #3:  4368.7255000000005ms
  Performance Run #4:  4359.9843ms
  Performance Run #5:  4327.2511ms

  Performance budget ok:  4368.7255000000005ms (limit: 4823.747680890538ms)


Multi Files (450 files):
  CPU Speed is 2695 with multiplier 39000000
  Performance Run #1:  12333.5897ms
  Performance Run #2:  12235.3641ms
  Performance Run #3:  12212.1394ms
  Performance Run #4:  12220.1246ms
  Performance Run #5:  12229.7475ms

  Performance budget ok:  12229.7475ms (limit: 14471.243042671615ms)

npm run test:performance with proxy:

Loading:
  Load performance Run #1:  233.8499ms
  Load performance Run #2:  294.5263ms
  Load performance Run #3:  314.7176ms
  Load performance Run #4:  271.8502ms
  Load performance Run #5:  228.772ms

  Load Performance median:  271.8502ms


Single File:
  CPU Speed is 2695 with multiplier 13000000
  Performance Run #1:  4663.8604ms
  Performance Run #2:  4561.8331ms
  Performance Run #3:  4591.1485ms
  Performance Run #4:  4620.0033ms
  Performance Run #5:  4560.5226999999995ms

  Performance budget ok:  4591.1485ms (limit: 4823.747680890538ms)


Multi Files (450 files):
  CPU Speed is 2695 with multiplier 39000000
  Performance Run #1:  13518.9048ms
  Performance Run #2:  13341.3334ms
  Performance Run #3:  13395.097ms
  Performance Run #4:  13337.3276ms
  Performance Run #5:  13336.2579ms

  Performance budget ok:  13341.3334ms (limit: 14471.243042671615ms)

Perhaps we could instead prevent calling the new methods by throwing an error from the method if it's called after the source code object is finalized?

[nzakas]

Perhaps we could instead prevent calling the new methods by throwing an error from the method if it's called after the source code object is finalized?

I can try switching to Object.create() and nulling those methods out to see if that improves the situation. We can't implement this inside of SourceCode because then every language plugin would have to duplicate that functionality.

[nzakas]

Okay, Object.create() approach is going to confuse this inside of SourceCode, so that's not going to work.

The reason I chose a proxy is because it's the only way I could think of to ensure that context.sourceCode will look the same even with having restrictions. If we create a wrapper object around SourceCode, that will exhibit a different prototype that could potentially break existing rules.

As I mentioned, we can't really implement this blocking inside of SourceCode because every language plugin would have to duplicate that for safety, which is a poor developer experience.

We could just pass the SourceCode object in without any protection, but the potential to call state-changing methods could mean hard-to-track-down bugs.

So I guess the decision here is if the safety of the proxy is worth the performance hit or not?

[mdjermanovic]

So I guess the decision here is if the safety of the proxy is worth the performance hit or not?

Since the performance hit seems large (on my computer, with proxies it takes ~25% more time to lint the eslint codebase; on CI, Verify Files > Lint Files step takes 1m 17s on this branch, while it's only 41s on the main branch, although this may not be just because of the proxy) I think the safety of the proxy is not worth the performance hit.

[nzakas]

Okay, I can remove the proxy. I'm wondering if maybe we should at least change those methods to use symbols instead of names? Maybe that will be a hint to rule developers that they shouldn't be calling these methods directly?

I suppose I can also check inside of SourceCode if it has been finalized, just a shame that every other language will have to do the same.

Do you have any other ideas?

[mdjermanovic]

Okay, I can remove the proxy. I'm wondering if maybe we should at least change those methods to use symbols instead of names? Maybe that will be a hint to rule developers that they shouldn't be calling these methods directly?

Symbols sound good to me. I think that would be a good hint to rule developers that these methods should not be used.

[nzakas]

Another option (which could be used with or without symbols) is to flag these methods in RuleTester. What do you think about that?

[mdjermanovic]

Another option (which could be used with or without symbols) is to flag these methods in RuleTester. What do you think about that?

Sounds good to me.

[nzakas]

I'm going to try to leave the methods as strings and see how far I can get with RuleTester. I realize this might be a bit tricky because, unlike other methods we check in RuleTester, these ones will actually be called by Linter first. If I can't get that to work then I'll go with symbols.

[nzakas]

I was able to get this working in RuleTester, so I left the methods as strings (easier to read overall). Let me know what you think.

[nzakas]

Updated and added missing tests to verify the changes.

[nzakas]

Looks like Node.js 19 and 20 have changed their JSON parsing error messages, so need to update tests accordingly.

[mdjermanovic]

Do we need the ok property here? Linter doesn't use it. Since this method is part of the required interface for custom SourceCode implementations, it might be confusing for them when and how to set this property. I think it would be better to remove this property.

[nzakas]

Good point. I think we should keep ok, as I believe we need a way to say "stop". For this, we should probably just always return true, but I could envision other languages wanting a different behavior.

[nzakas]

Upon further review, it looks like ok was intended just to determine if there were any problems to display warnings about, so I agree we don't actually need it.

[mdjermanovic]

This comment doesn't seem to apply here?

[nzakas]

Good catch!

[mdjermanovic]

Moving this to finalize() will now unfreeze these objects in the eslintrc mode, since we're not calling finalize() in the eslintrc mode.

Maybe we could keep this freezing here, as it doesn't seem we're actually modifying these objects later, neither in eslintrc nor in flat config mode. We only modify them in the new source code tests by adding scopeManager to the instance, but those tests might be more representative anyway if we pass scopeManager to the constructor like Linter does.

[nzakas]

I think ultimately we want freezing to happen in finalize(), but I agree that while we have eslintrc in the mix we should keep things consistent.

[mdjermanovic]

LGTM, thanks!