Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Provide access to parser from plugin. #3670

Closed
benmosher opened this Issue Sep 5, 2015 · 16 comments

Comments

Projects
None yet
5 participants
@benmosher
Copy link
Contributor

benmosher commented Sep 5, 2015

Hello,

For my ES6 import-validating plugin, I parse dependencies and ensure import statement are valid.

Currently, if a user is using a custom parser (generally babel-eslint) they must configure it twice in the .eslintrcfile, once at the top level (parser) and again in settings (import/parser).

I'm not currently aware of any benefit to this being independent, so optimally I'd like to

  • have access to the parser setting in the plugin, so I can require the same one (good)
  • have access to the parser itself and use it directly (better)

I like the latter because currently every file is parsed twice, once for its own rules, and once again assuming some file imports it. Having access to the parser means it could theoretically memoize the AST based on the path and avoid the additional parse. IM(anecdotal)E babel-eslint is substantially slower than espree, so I might be able to cut the full-project lint time in half.

Either would be an improvement over the redundant setting that is necessary today. I just answered an issue this morning where it was not obvious that the parser needed to be declared twice.

Thanks for your consideration. Given acceptance of one of these solutions (or something else, I'm not picky), I'm happy to do the work and submit a PR. 😄

@eslintbot

This comment has been minimized.

Copy link

eslintbot commented Sep 5, 2015

Thanks for the issue! We get a lot of issues, so this message is automatically posted to each one to help you check that you've included all of the information we need to help you.

Reporting a bug? Please be sure to include:

  1. The version of ESLint you are using (run eslint -v)
  2. The source code that caused the problem
  3. The configuration you're using (for the rule or your entire config file)
  4. The actual ESLint output complete with line numbers

Requesting a new rule? Please be sure to include:

  1. The use case for the rule - what is it trying to prevent or flag?
  2. Whether the rule is trying to prevent an error or is purely stylistic
  3. Why you believe this rule is generic enough to be included

Requesting a feature? Please be sure to include:

  1. The problem you want to solve (don't mention the solution)
  2. Your take on the correct solution to problem

Including this information in your issue helps us to triage it and get you a response as quickly as possible.

Thanks!

@eslintbot eslintbot added the triage label Sep 5, 2015

@nzakas

This comment has been minimized.

Copy link
Member

nzakas commented Sep 5, 2015

@benmosher I'm sorry, I'm having a hard time understanding what you're doing. Plugins and rules are designed so that shouldn't need to know what parser is being used, as they should not be parser-dependent. Can you explain why you need to include the parser?

@benmosher

This comment has been minimized.

Copy link
Contributor Author

benmosher commented Sep 5, 2015

As one example, given

// mod.js
import { x, y } from `./foo`

in the file being linted by ESLint proper, my plugin will

  • resolve the path to ./foo
  • parse it (this is where I'd like to share the setting / parser object)
  • find its ES6 exports:
// foo.js
export const x = "such x"
export const notY = "this is not the y you're looking for"
  • validate that x, y are each exported by ./foo.

Given foo.js above, the import of y would be reported as an error. (import/named)

There are a number of other similar rules that do the same for default exports, namespaces, etc.

Internally, the export maps are cached so a given imported module will be parsed for exports only once, even if it is imported more than once across the set of files being linted, so the parsing is not crazy expensive.

So yes, it's a little weird to be parsing inside a rule, but so far it is necessary to provide the static analysis of ES6 imports/exports that the syntax is designed to allow.

@nzakas

This comment has been minimized.

Copy link
Member

nzakas commented Sep 5, 2015

Ah, interesting. So really what we are talking about here is giving a rule access to which parser is being used. We can just stick that on context so all rules can see it.

@ilyavolodin

This comment has been minimized.

Copy link
Member

ilyavolodin commented Sep 6, 2015

Honestly, it feels to me that what you are trying to achieve goes beyond the scope of what plugins are supposed to be able to do. Plugins were designed to be simple collections of custom rules.
I proposed something like this a long time ago, but as part of the core of ESLint. I wanted to be able to import global variables based on comments (as in if file "a" says that it depends on file "b" - parse "b" first, retrieve all global variables and put them into some hashmap that will be available to rules linting file "a").
If we were to implement something like that, and make it pluggable enough so you could have substitute comments for "import" or "require" and globals for "export", "define" or "module.exports" that would be pretty powerful, and would solve the issue you are trying to hack around:-) But I do understand that it's extremely hard to implement in a way that isn't going to affect performance much.

@ilyavolodin ilyavolodin added core feature and removed triage labels Sep 6, 2015

@nzakas

This comment has been minimized.

Copy link
Member

nzakas commented Sep 6, 2015

@ilyavolodin you're going quite a bit off the topic here. :) What we are talking about is just exposing info from .eslintrc to rules. We already expose ecmaFeatures and settings, so I don't see this as much different than that.

@nzakas nzakas added the accepted label Sep 6, 2015

@benmosher

This comment has been minimized.

Copy link
Contributor Author

benmosher commented Sep 6, 2015

One thought: a benefit to providing the actual parser module (instead of just the string from the parser key in config) is that the module would need only to be reachable (require-able) from ESLint, and not also the plugin.

That said, I suspect there aren't many cases where it would matter as one or both are likely installed globally, and just the string value would be an improvement over the double-config I have now. I can figure some kind of warning out if the configured parser can't be require-d by the plugin.

@BYK

This comment has been minimized.

Copy link
Member

BYK commented Sep 7, 2015

I think exposing the actual parser interface is a better option. 👍 to @benmosher

@nzakas

This comment has been minimized.

Copy link
Member

nzakas commented Sep 7, 2015

It's only better if want to use the same parser as ESLint, not if you want to check for a particular parser (if (context.parser !== "espree")). We need to consider all use cases here.

@BYK

This comment has been minimized.

Copy link
Member

BYK commented Sep 10, 2015

Why can't we have parser.name or something on the exposed parser object? I still feel like having access to the actual parser object would be more useful than rules checking for the parser name.

@benmosher

This comment has been minimized.

Copy link
Contributor Author

benmosher commented Sep 11, 2015

@{owners}: do you have an implementation in mind for this, or would it be helpful for me to put together a PR or two with some implementation options?

@nzakas

This comment has been minimized.

Copy link
Member

nzakas commented Sep 14, 2015

@BYK then you'd need every parser to implement parser.name or else we would need to add it.

@nzakas

This comment has been minimized.

Copy link
Member

nzakas commented Sep 14, 2015

@benmosher we don't have agreement on the implementation, so it's not yet time to build something. If you have implementation suggestions, please describe here.

@benmosher

This comment has been minimized.

Copy link
Contributor Author

benmosher commented Sep 15, 2015

I can imagine adding one or both of the following to RuleContext:

  • parser: for consistency with the other options, this would be the string module name, as configured (or DEFAULT_PARSER if unconfigured)
  • parse: a light function wrapper around the chosen parser that takes only the contents of a file, and--using the same parser options as ESLint would use--returns the parsed AST. This would prevent duplication of transcription from ESLint settings to parse option config (i.e. turning ecmaFeatures into parse options) and also ensures custom rules have access to the same parse behavior as ESLint proper.
@BYK

This comment has been minimized.

Copy link
Member

BYK commented Sep 15, 2015

@BYK then you'd need every parser to implement parser.name or else we would need to add it.

Us attaching parser.name doesn't sound too terrible. Just worried about cluttering the object. @benmosher's latest suggestion also sounds good (may be better than exposing the raw parser module).

@nzakas

This comment has been minimized.

Copy link
Member

nzakas commented Sep 15, 2015

Interesting idea. Normally I'm not a fan of providing things where there's just one character difference, as the potential for typos is very high.

However, there are other things provided on the parser that people might need as well, such as the tokenize() method, so I'm not sure just providing a parse() method is enough.

benmosher added a commit to benmosher/eslint that referenced this issue Dec 15, 2015

benmosher added a commit to benmosher/eslint that referenced this issue Dec 16, 2015

@nzakas nzakas closed this in bbf7f27 Jan 13, 2016

nzakas added a commit that referenced this issue Jan 13, 2016

Merge pull request #4649 from benmosher/parser
New: provide parsing faculties via RuleContext (fixes #3670)

@eslint eslint bot locked and limited conversation to collaborators Feb 7, 2018

@eslint eslint bot added the archived due to age label Feb 7, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.