Proposal: add option to include comments in token store methods

[kaicataldo]

What version are you using?
Master

The problem I'm trying to solve
On instantiation, the sourceCode object creates a token store and then creates public methods that call through to the token store methods. Since this does not include comments, we have had to create a separate store that includes comments and then create additional methods, as seen here.

Working on #7696, I ended up needing to expose another method from the tokensAndCommentsStore, which doesn't feel good, is confusing, and makes it hard to maintain.

Proposed solution
I would like to propose unifying this API by adding the ability to include comments in the results with { includeComments: true } as the last parameter for any applicable token store method.

So, instead of having

sourceCode.getTokenBefore(node, skip);
sourceCode.getTokenOrCommentBefore(node, skip);

we would have

sourceCode.getTokenBefore(node, skip);
sourceCode.getTokenBefore(node, skip, { includeComments: true });

Two possible implementations immediately jump out to me:

• Having sourceCode create two token stores as it does now and create public proxy methods that call the method on the correct store using the API I've proposed above.
• Updating createTokenStore to take in ast.comments as a second parameter, having it create two separate token lists internally, and explicitly update all its public methods with the additional { includeComments: true } parameter. This is my preference, as I think it's clearer and makes more sense conceptually for the token store to manage this internally.

This can be done in a non-breaking way, as we can make getTokenOrCommentBefore and getTokenOrCommentAfter call through to the correct methods, and in any cases where methods are invoked without the { includeComments: true } argument, nothing will change.

I'm very much willing to work on implementing this :) Would love it if some JSCS folks could chime in, as I believe this is how JSCS handled this.

[not-an-aardvark]

👍 for the idea. I'm wondering if we should change the function signature to have an object as the second parameter:

sourceCode.getTokenBefore(foo, { skip: 3, includeComments: true });

For backwards compatibility, we could treat the second argument as skip if it's a number. I think adding more arguments might make the function confusing to read. Without looking at the documentation, it's hard to tell what the 0 here is doing or why it's necessary:

sourceCode.getTokenBefore(foo, 0, { includeComments: true });

[mysticatea]

👍 .
Actually, I have been thinking to refine TokenStore with ES2015 classes within this new year vacation, then merge tokensStore and tokensAndCommentsStore

[kaicataldo]

@not-an-aardvark I think it makes sense to combine all the additional options into a single object parameter when possible!

@mysticatea Sounds like we were thinking of the same thing!

[kaicataldo]

@mikesherov Would love your take on this since, according to this comment, it sounds like JSCS did something similar?

[mikesherov]

JSCS had standard token iterators with optional ability to include comments. I think it worked out well :-). Default IMO should be to include comments considering if you're in token land, you're typically going to care about comments interfering with style.

[kaicataldo]

@mikesherov Agreed that it would be nice if it was on by default, but I don't think that's a good idea in this case only because it could break plugins/tools in the ecosystem that currently rely on comments not being included. Even in a major version, I'm not convinced that it's worth the change at this point.

But glad to hear this approach worked well for JSCS!

[mikesherov]

That's fair, although I think you'll find it wouldn't break userland as much as you think, and the cases it does break would be a bugfix. But I'm fine with not changing default.

[not-an-aardvark]

I'm in agreement that we should not include comments by default. In fact, even disregarding backwards compatibility, I want to exclude comments in most of the cases where I use these functions.

A typical use-case is:

// suppose `node` is a BinaryExpression, e.g. `(1) * (2)`

// get the token for the '*' operator
const operatorToken = sourceCode
    .getTokensBetween(node.left, node.right)
    .find(token => token.value === node.operator);

[mikesherov]

@not-an-aadvark although we all agree to not include comments by default... as a thought exercise, can you explain how including comments in the code you posted would result in the wrong thing happening?

[not-an-aardvark]

@mikesherov If the list returned by getTokensBetween included comments, and the code looked like this:

foo /*+*/ + bar;

Then operatorToken would end up assigned to the block comment containing a +, rather than the + operator.

[mikesherov]

@not-an-aadvark that seems like it would be almost impossible for that to happen.

[not-an-aardvark]

I agree that comments like this are probably quite rare, but I don't think that matters -- if comments were included in the token store by default, the rule logic would have work around them to account for the possibility of a comment like this. The code is trying to use logic that is only applicable to tokens, not comments. This is the case the majority of the time I use tokenStore methods, which is why I think it's reasonable for the new combined methods to exclude comments by default.

[nzakas]

Just my 2 cents: I'd prefer not changing the exposed methods or their signatures. Those are exposed to rules, and I don't think there's a bug enough gain for the ecosystem to absorb a breaking change there.

That said, the implementation details of the existing methods can change to whatever makes the most sense. The token store isn't exposed as part of our API, so I think it's open season for anything you think would be an improvement.

[kaicataldo]

@nzakas Just to clarify, are you advocating that we keep the current API of separate methods for token iteration with and without comments, adding new ones as we need them?

[nzakas]

@kaicataldo precisely. Sorry for being unclear.

[kaicataldo]

@nzakas Having made the issue, I obviously disagree. Making all the currently available token iterators have the ability to include comments gives rule creators more tools. Right now, they would have to request that we add a new method to core every time they need new functionality, as I had to while working on the PR that led me to this.

What if we made this change in a non-breaking change (implementation details in the proposal)? The current two "tokenOrComment" methods aren't documented anywhere yet, so I think now is the time to remove these if that's something we want to do (we can deprecate them and still have them call the correct underlying token store method and then remove them in the next major version).

I think the alternative to this is to combine the two token stores and then to duplicate all the relevant methods in token store up front - one for tokens and one for tokensAndComments - rather than adding them in piecemeal as needed.

Though I'm advocating for the former, as I think it's a nicer API (and avoids having to duplicate a bunch of methods!), I'm not against the latter if it's easier to gain consensus on that. Either would be an improvement of the code and give us and rule authors clarity on how to iterate over comments :)

Thoughts?

[nzakas]

@kaicataldo sorry, I think I'm missing what you're disagreeing with.

Let me try to be clearer: I don't want to remove or change existing methods on SourceCode because that is part of the public API that rules rely on. Other than that, I pretty much have no objections. The implementation details of those methods can and should be changed when they can be improved. If you want to change how TokenStore works, or the methods on it, go for it, I just don't want to see the SourceCode API changed. Similarly, im pretty sure using two stores was a quick hack to get something working and not a necessary implementation detail, so that can definitely be changed.

[platinumazure]

@nzakas Does your point apply even to non-breaking API changes, like adding extra parameters? Because as far as I can tell, @kaicataldo's proposed implementation is non-breaking except for if any of the getTokenOrComment* methods are removed (and he has noted that those methods could delegate to the getToken* methods with { includeComments: true }.

I also like the general direction of his proposal because it would also allow for adding granularity options in the future (e.g., getTokenAfter(nodeOrToken, skip, { value: "(" }) to get the first left parenthesis token after the node plus any skips). That's not directly material to this proposal, of course, I'm just saying it feels like a step in the right direction for future extensibility.

[kaicataldo]

Sorry for not being clearer myself!

I think where I'm getting tripped up is thinking about what we would do in the event that we need to add more methods to include comments in the token store iterator methods. Giving the existing methods the ability to include comments feels like a better solution to me than adding a new duplicate method that includes comments each time the need arises.

[nzakas]

what we would do in the event that we need to add more methods to include comments in the token store iterator methods

Let's be clear, are you talking about TokenStore methods or SourceCode methods? I'm only concerned about the latter.

In general, I find the aesthetics of getNextTokenOrComment preferable to adding another parameter to getNextToken where there's only one known option in an options object (to me, it aids my understanding better vs. needing to look at the end of the call to see if it's doing something different). I also don't have any concerns about adding new methods if necessary.

[kaicataldo]

I was thinking of them as one and the same, since all the TokenStore methods are added to SourceCode, but I understand the confusion.

Thanks for the explanation. So, it sounds like no one is against refactoring SourceCode and TokenStore so that only one TokenStore is needed instead of two. However, it does sound like there is a discrepancy over what the API should be for traversing comments using SourceCode's public methods.

As I mentioned earlier, though I'm proposing changing the API, I'm not against keeping the current API and making the code clearer/updating the documentation. My end goal in this is to make it clear how to do token traversal including comments for rule authors.

[nzakas]

I was thinking of them as one and the same, since all the TokenStore methods are added to SourceCode, but I understand the confusion.

Yeah, there's a relationship of convenience there, but we can redefine this relationship at any time.

My biggest concern is not introducing a breaking change to our exposed API because of all the custom rules that exist and may rely on methods we don't quite want around. So we can either keep them and add more as needed or deprecate them and encourage using others. I just don't think we can remove any methods anytime soon.

All that said, you've done a lot of thinking on this, so I'll leave the final approach up to your judgment now that I've voiced my concerns.

[kaicataldo]

@eslint/eslint-team Any other thoughts?

@mysticatea Sounds like your task of refactoring doesn't need to be blocked by this (and I don't think needs to be tracked in this issue, if it truly is a refactor). We can always add more methods afterwards, if that's what the decision here is.