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

Allow an AST to be passed in to ESLint.verify [$10 awarded] #1025

Closed
mrennie opened this Issue Jun 27, 2014 · 19 comments

Comments

Projects
None yet
7 participants
@mrennie

mrennie commented Jun 27, 2014

In Orion we use ESLint as our default linter and one of the problems that we have for migrating to new versions is that we have to update the verify code to allow us to pass in our own pre-computed AST rather than have ESLint parse the source again for us.

The reason for this is that we have a customized 'Orion version' of Esprima with a high degree of error tolerance.

Basically we make the following updates to eslint.js:

api.verify = function(textOrAST, config, filename, saveState) {

var ast = (textOrAST && typeof textOrAST === "object") ? textOrAST : ast = parse(textOrAST);

I can create a pull request for this if there is any interest in it.

The $10 bounty on this issue has been claimed at Bountysource.

@jrajav

This comment has been minimized.

Contributor

jrajav commented Jun 27, 2014

Duplicate of #948, #1013 maybe somewhat related too. Main conclusion from #948 is that it's not feasible to just pass in AST since many rules rely on the tokens or the source as well, and esprima needs to have generated the AST with certain options. However a caching or a pre-parse solution provided by eslint could be an option.

@nzakas nzakas added wontfix and removed wontfix labels Jun 27, 2014

@nzakas

This comment has been minimized.

Member

nzakas commented Jun 27, 2014

@mrennie as @jrajav pointed out, ESLint can't work reliably with just any AST. I know Orion is several versions behind, which might be why you haven't seen any significant issues with this approach.

Given our needs, we can't just allow any AST to be passed in because it just won't work for many rules, and will either throw errors or give incorrect results.

I'd like to make it easier to use for Orion, it's just that this proposal won't work.

@mrennie

This comment has been minimized.

mrennie commented Jul 2, 2014

I know Orion is several versions behind, which might be why you haven't seen any significant issues with this approach.

We are only a few versions back due to our IP process - the update to 0.6.2 has been tested and is only waiting for the beginning of 7.0 development. That said, the other reason we see no issues is that Orion does not use the stock set of rules from ESLint (again an IP process).

Given our needs, we can't just allow any AST to be passed in because it just won't work for many rules, and will either throw errors or give incorrect results.

You could simply make the API contract explicit in the JSDoc - 'The AST must be a well-formed Mozilla spec'd syntax tree with the following attributes...'

@nzakas

This comment has been minimized.

Member

nzakas commented Jul 2, 2014

As I mentioned, this isn't just about the AST. The rules also need access to the tokens and source text. Without those three, there's no guarantee anything is going to work correctly. Allowing people to just pass in an AST would be a broken contract.

Would you be able to pass in all three pieces of data?

@mrennie

This comment has been minimized.

mrennie commented Jul 2, 2014

Would you be able to pass in all three pieces of data?

Yes, our AST is always generated with tokens, and we also have the handle to the editor source being linted.

@nzakas

This comment has been minimized.

Member

nzakas commented Jul 2, 2014

Okay, then what I'd propose is that we make a custom type that can be alternately passed in as the first argument to verify. Something like:

function SourceCode(ast, tokens, text) {
    this.ast = ast;
    this.tokens = tokens;
    this.text = text;
}

SourceCode.prototype.validate = function() {
     // some validation logic to make sure we have all the data we need
};

Then pass in an instance of SourceCode instead of text as the first argument to verify().

(Don't take this as a literal description of what this should ultimately be - this is just off the top of my head.)

I think that would solve your case and address my concerns. Thoughts?

@mrennie

This comment has been minimized.

mrennie commented Jul 2, 2014

Sounds good to me.

@kevinbarabash

This comment has been minimized.

kevinbarabash commented Jan 15, 2015

👍

@tqc

This comment has been minimized.

tqc commented May 26, 2015

I'm looking at implementing at least part of this (see #2610). My main concern is the implementation of SourceCode.validate.

The simple implementation is just

return this.tokens != null && this.text != null;

That would catch stupid mistakes, but will let more complex problems through.

The complete, provably correct implementation I strongly suspect reduces to the halting problem.

What level of validation is needed to consider a supplied AST valid?

@nzakas

This comment has been minimized.

Member

nzakas commented May 28, 2015

(Don't take this as a literal description of what this should ultimately be - this is just off the top of my head.)

We just need all three pieces of data. I don't think validate() is necessary.

@tqc

This comment has been minimized.

tqc commented May 28, 2015

Ok, that simplifies things. Would you be ok with adding a property to the result returned from esprima? That way we don't need a new object, just a requirement that ast.text is set by the time it gets to verify, same as it is with tokens.

@nzakas

This comment has been minimized.

Member

nzakas commented May 28, 2015

No, I want to stick with esprima format as it is.

tqc added a commit to tqc/eslint that referenced this issue May 29, 2015

@tqc

This comment has been minimized.

tqc commented May 29, 2015

I have put together an initial implementation. Note that it isn't tested and I have deliberately left some errors in the interest of a clear diff, but it shows the general approach.

I have added a new api.verifyPreparedAST to provide a separate path for externally prepared ASTs that may need more validation and to keep the original api.verify simple, but it could be done with the object/string parameter switch if you don't like adding new public methods.

Look good to you?

@nzakas

This comment has been minimized.

Member

nzakas commented May 29, 2015

Can you send a pull request?

Fwiw, I don't want a separate method.

@IanVS

This comment has been minimized.

Member

IanVS commented Jul 14, 2015

I've posted a $10 bounty on Bountysource for this issue. Implementing this may simplify automatic configuration creation.

@ilyavolodin ilyavolodin changed the title from Allow an AST to be passed in to ESLint.verify to Allow an AST to be passed in to ESLint.verify ($10) Jul 14, 2015

@ilyavolodin ilyavolodin added the bounty label Jul 14, 2015

@ilyavolodin ilyavolodin changed the title from Allow an AST to be passed in to ESLint.verify ($10) to Allow an AST to be passed in to ESLint.verify [$10] Jul 14, 2015

@IanVS

This comment has been minimized.

Member

IanVS commented Jul 23, 2015

@tqc Do you still have interest in submitting a pull request for this issue?

@tqc

This comment has been minimized.

tqc commented Jul 24, 2015

@IanVS Sorry, got most of the way there then suddenly got very busy with other things. Pull request coming up.

@IanVS

This comment has been minimized.

Member

IanVS commented Jul 30, 2015

@nzakas Since #3151 went a direction you weren't happy with and was closed, can you give a little more guidance on what you'd like to see in this change? Your comment in #1025 (comment) was a little unsure sounding. Is that definitely the direction to go here?

@nzakas

This comment has been minimized.

Member

nzakas commented Jul 30, 2015

Yes

@nzakas nzakas closed this in fc3bc8a Aug 24, 2015

nzakas added a commit that referenced this issue Aug 24, 2015

Merge pull request #3448 from eslint/issue1025
Update: Allow pre-parsed code (fixes #1025, fixes #948)

@nzakas nzakas changed the title from Allow an AST to be passed in to ESLint.verify [$10] to Allow an AST to be passed in to ESLint.verify [$10 awarded] Sep 17, 2015

@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.