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

Improve pattern source information #282

Merged
merged 7 commits into from
Aug 10, 2021
Merged

Improve pattern source information #282

merged 7 commits into from
Aug 10, 2021

Conversation

RunDevelopment
Copy link
Collaborator

@RunDevelopment RunDevelopment commented Aug 8, 2021

This PR changes how rules determine the position of RegExpp nodes in the original JS source code.

Instead of analyzing the AST in each call of getRegexpRange, we now store the information about how the pattern string is constructed in a PatternSource instead.

PatternSource

A PatternSource is essentially just an array of all parts (concatenated) that make up the pattern string. Examples:

  • /foo/i: the instance is [/foo/i]
  • RegExp(/foo/i, "m"): the instance is [/foo/i]
  • RegExp("foo"): the instance is ["foo"]
  • RegExp("foo" + "bar"): the instance is ["foo", "bar"]

Each segment that makes up the pattern has information about its AST node, its string value, and its range within the complete pattern string.

Here is a complete instance for the code:

const space = "\t\n\r ";
new RegExp("([" + space + "]+)");

image

Since we know exactly what AST node contributed in what way to the final pattern string, we pinpoint the location of RegExpp AST nodes. This is great for error messages but it also allows us to fix a lot more.

Comparison

Old:
(nothing is fixable)

image

New:
(everything is fixable)

image

Semantic differences

The old getRegexpRange had 2 porpuses:

  1. Get AST ranges for error reports.
  2. Get AST ranges for fixes.

The new API by PatternSoruce explicitly differentiates these 2 use cases.

This is important because AST ranges for fixes have to be exact and they have to map to exactly one AST node that we can change. However, AST ranges for error reports don't have to be exact, they just have to be good enough for humans.

The new API:

  1. PatternSoruce#getAstRange for error reports. This implements a best-effort approach to provide an AST range that is useful to humans.
  2. PatternSoruce#getReplaceRange for fixes. The returned PatternReplaceRange contains all the information to edit source code.
    PatternReplaceRange also has methods to create fixes. These methods will automatically handle escaping (if necessary) and should be used instead of fixer.replaceTextRange.

By differentiating these to use cases, we can provide exact ranges for fixes and good approximate ranges for error reporting. This should improve the user experience a lot.

Rule changes

The above-mentioned semantic changes broke a few rules. All rules now use the new API.

This is quite nice because it means that all rules now benefit from PatternSource. This improvement can be seen in the test cases. A few test cases changed because they can now be fixed. The report range also changed on 1 test case.


This fixes half of #231.

@RunDevelopment RunDevelopment added the enhancement New feature or request label Aug 8, 2021
Copy link
Owner

@ota-meshi ota-meshi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Thank you for lot of work!

@ota-meshi ota-meshi merged commit 0eced9d into master Aug 10, 2021
@ota-meshi ota-meshi deleted the pattern-source branch August 10, 2021 05:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants