-
-
Notifications
You must be signed in to change notification settings - Fork 208
Change Request: Program
range cover whole program
#648
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
Comments
Duplicate of #488 |
Thanks for the feedback. As I stated in #488 (comment), Espree is the canonical ESLint parser. Any parser wanting to work with ESLint should mimic what Espree does. As such, we need to be careful when making changes. The decision of where to start the I think producing comments that fall outside of the range of For reference, here's the code that adjusts the js/packages/espree/lib/espree.js Lines 184 to 210 in 2800ec8
I think the issue is that comments are not represented as tokens, so the end location currently only considers non-comment syntax. |
Thanks very much for coming back. Sorry that I didn't see this had been raised before in #488. Thanks for confirming that the primary goal for Espree remains aligning with Esprima. In that context, of course the answer is clear. The current behavior cannot change. Only 2 questions remain: Incompatibility with EsprimaIf the program contains only comments e.g.: // foo Espree produces a range of I assume that divergence from Esprima is unintended? If so, should that be considered a bug (fixing it not a breaking change)? Or something to tackle in next major release? Comments outside of
|
I think this was unintended as I can't imagine we tested this case when we switched to Acorn. I think this is an indicator that we just need to come to a decision on one question: are comments considered part of |
In my view they are. But you've clearly stated that the goal is to align with Esprima, so I thought that answered the question - Esprima does not consider comments to be part of the |
@overlookmotel As I noted, there is an inconsistency in the implementation. Trailing comments are not treated as part of the program but leading comments are. I'm seeking to resolve that inconsistency. |
@nzakas I don't see an inconsistency between how leading comments are treated vs how trailing comments are treated. I believe Program's range currently works as follows:
Is that correct? If not, can you provide an example where there's an inconsistency between how leading and trailing comments are treated? |
Sorry, I don't think I explained the inconsistency I'm seeing very well. Let me try again. For instance, here's an example with just a comment: In this case, the If I add some code underneath it, the range changes to 178 to 182: That's the inconsistency I'm concerned with. It seems like the start of the program should be the start of the program. (Similarly, the end of the program should be the end of the program.) But we are counting leading and trailing comments differently in these situations. |
Thanks for coming back. I believe the only change required to resolve this inconsistency is as follows:
i.e. in your first example, This would match Esprima's behavior. There are other alternative solutions which could also resolve the inconsistency, but they would not align with Esprima, which I've understood from your previous comments is of paramount importance. I hope I've made sense this time! |
I still don't see an inconsistency between leading and trailing comments. If there are no tokens, the range spans from the start to the end of the source code, whether the source code content is whitespace-only or contains one or multiple comments. If there is at least one token, the range spans from the start of the first token to the end of the last token, and neither leading nor trailing comments are included. For example, in the following case the /**
* Leading comment
*/
/**
* Trailing comment
*/ While in the following case the /**
* Leading comment
*/
foo
/**
* Trailing comment
*/ |
@mdjermanovic okay, you've lost me a bit. The example you show is exactly the inconsistency I'm talking about. It doesn't make sense that comments and whitespace are considered part of the program in one case and not in the other. I don't see why having one token should change how we're calculating the start and end ranges of the program. |
I think the root of this confusion goes back to my original point that the mental model doesn't entirely hang together. Consequently, different people can come at this question from different directions and reach different answers. However, it's a stated goal of Espree to align with Esprima, so we need to find a solution which both:
The rule that Esprima follows is that So, in my view, Esprima's schema is internally consistent. Therefore the only option which satisfies both criteria is to align with Esprima's behavior and make the range for an empty program Other interpretations are also valid, but they fail on the 2nd criteria. Side note: If we were starting from ground zero and didn't have to care about breaking changes or alignment with Esprima, I'd advocate ripping it all up and making |
@overlookmotel Thanks for the summary. At this point, I'm not sure we need to align this with Esprima, that's why I've opened this up for larger discussion. Espree is used far more than Esprima, so we have more license around the edges. We can't change things like the AST format or tokens format as that would break too much compatibility. Because we are already not following Esprima for |
Ah, sorry I'd misunderstood. Given that this thread has become lengthy, let me summarize my personal view on what I think would be the ideal if we were starting from scratch. The problems:
I propose that in all cases:
This would be different from Esprima, but would align with Acorn (and also Babel). For what it's worth, TS-ESLint parser maintainers also seem to think this would be preferable. I will now shut up and let others give their views! |
Yes, that's what I think as well. |
This comment has been minimized.
This comment has been minimized.
@fisker please open a separate issue for that. |
Which packages would you like to change?
espree
eslint-scope
eslint-visitor-keys
What problem do you want to solve?
The source range for
Program
is surprising when source code starts or ends with comments.e.g.:
Espree (like Esprima) produces an AST where
range
ofProgram
node excludes whitespace and comments from start and end of file. In example above, the range ofProgram
node is[ 8, 9 ]
i.e. the range of the sole statement.AST explorer - Espree
This differs from Acorn, which produces a range of
[0, 17]
i.e. the whole of the source code.AST explorer - Acorn
What is particularly surprising is that with option
comment: true
you get comments which are outside the range of the program that contains them:As a further surprise, if the file contains only comments, then the behavior changes:
// foo
This produces a range of
[ 0, 8 ]
. This does not align with Esprima, which produces range of[ 8, 8 ]
.What do you think is the correct solution?
Espree's docs state that:
Given that Esprima has not received updates for 7 years, and is essentially defunct, I'm not sure whether the maintainers of Espree consider alignment with Esprima still to be the primary goal. I'm also not sure whether that aim, if it still holds, extends to source ranges, or just the AST shape.
What's the correct behavior?
This is arguable. But, in my opinion, it's unintuitive that comments which are part of a program exist in ranges outside of the
Program
.The example where the program consists of only comments I think is illustrative. What's the logical range for
Program
to have in this case, if comments are excluded?[0, 0]
or[8, 8]
?The fact that there isn't a clear answer I feel reveals there's a problem with the mental model.
[0, 8]
(as Espree currently does) makes most sense to me. But this is inconsistent with Esprima, and also inconsistent with Espree's behavior when there are statements in the program.Others may feel differently.
Why does this matter?
In addition to the difference from Acorn,
@typescript-eslint/parser
is also different again - for the first example, it gives a range of[8, 18]
! Though the maintainers seem to think Acorn's behavior would be preferable.AST explorer - TS-ESLint
Personally, I think it's unfortunate that the 3 most depended-on ESTree-compatible parsers Espree, Acorn, and TS-ESLint differ in this respect.
What to do?
One way or another, in my opinion Espree's current behavior is incorrect.
Changing behavior to match Acorn (which personally I'd advocate for) would be a breaking change. Though, given that the 3 most popular ESTree-compatible parsers differ from each in this regard and I'm not aware of anyone complaining about it before, probably not a big one!
Participation
Additional comments
No response
The text was updated successfully, but these errors were encountered: