Proposal for review: suggested fixes

[JoshuaKGoldberg]

As @davidtheclark mentioned in #2059, this is a rough draft for review. Not a request to merge. As of fcc159a I think this is ready to be considered merge. Still should be in discussion.

The implementation details are simpler than I'd originally suggested. Rather than some contract for rule functions to expose a suggestFixes, they just provide suggestedFix as part of the violation POJO. IMO this fits much better with the existing architecture.

A couple caveats:

• Style guide is broken in a few places.
• I'm inexperienced with your CSS parser and utils. If there's a better way to do any of the fix suggestions that would be great!

[JoshuaKGoldberg]

A note on this function: it'll convert..

.foo { color: blue; }
@media screen { .bar { color: red; } }

...into something like:

.foo {
 color: blue; 
 }
@media screen {
 .bar {
 color: red;
 }
 }

It intentionally doesn't try to maintain any sort of sensible indentation standard. If a rule that complains about indentation standard is enabled, it would fix the indentations in a subsequent wave.

[JoshuaKGoldberg]

(index should be the index of the root CSS)

[Arcanemagus]

Is the type required? You have at least two types of fixes here, which makes the insertion parameter rather misleading in the text-swap for example.

How would you handle removal of text? text-swap?

For reference the fixes returned by ESLint are quite simple: A character position to start and stop replacing text, and text for that range. Insertion of text is done by setting the start and stop points to the same position. With the current implementation here more complex logic will need to be done in the client side to handle these fixes (changing behavior based on the type).

[JoshuaKGoldberg]

@Arcanemagus: type is required and has some docs at https://github.com/autolint/automutate#mutations. The types I'm proposing are:

• multiple - Container for multiple mutations.
• text-delete - Deletes a range of characters.
• text-insert - Inserts a string at a point.
• text-replace - Replaces characters matching a string or regular expression within a range.
• text-swap - Swaps a range of characters with a new string.

[davidtheclark]

Thanks @JoshuaKGoldberg. Just letting you know that it will be a few days before I take a good look at this, because of Thanksgiving.

[Arcanemagus]

@JoshuaKGoldberg Ah, I missed that this was integrating into another tool.

[davidtheclark]

To be honest, these examples are not really convincing me that there's much benefit to co-locating the linting rules and the fixing functions.

The at-rule-semicolon-newline-after example only addresses one of three primary options (and that rule has no secondary options). The block-no-single-line example adds a bunch of linting-independent logic. And no-missing-end-of-source-newline is too simple to be very informative.

By contrast, a stylefmt file like https://github.com/morishitter/stylefmt/blob/master/lib/formatRules.js seems to suggest that the code will be more concise and clear if the fixing functions are independent, and just move through the AST reformatting everything.

I'm not really sure what the best path is here, but I guess right now I'm leaning towards sticking with what we have now — stylefmt as an independent library — because it seems to make at least as much sense as this new alternative, and won't involve huge changes.

I'm still very interested in hearing what @morishitter or other stylefmt maintainers have to say about how that library has been going and whether this new suggestion would be an improvement.

[JoshuaKGoldberg]

Ping, @morishitter?

...these examples are not really convincing me that there's much benefit to co-locating the linting rules and the fixing functions.

It's hard to argue against that point when the file size nearly doubles for this extra logic... I think it's relevant to note that lesshint also decided on keeping auto-fix information out of the core project for the same general reason. autolesshint instead provides the suggestions via Suggester classes that map directly to the linter rules.

By contrast, a stylefmt file like https://github.com/morishitter/stylefmt/blob/master/lib/formatRules.js seems to suggest that the code will be more concise and clear if the fixing functions are independent, and just move through the AST reformatting everything.

The code is indeed more concise, but is it really more clear? I would argue that structuring fixes in a near-procedural way like that isn't a very scalable approach. That's not to bash stylefmt at all (morishitter's obviously done a great job with the library), but it seems relatively harder to parse through and relatively harder to refactor to accommodate linter rule changes.

More importantly, how would someone make stylefmt extensible? It has no support for stylelint plugins. I'm not seeing any way to do it in such a project structure without a major rewrite.

By contrast, the design of using linter complaints bound 1-1 with fix suggestions lends itself well to extensibility in fixing the complaints. The complaints can contain their own fixes (the approach in this PR, ESLint, and TSLint), and/or users can provide custom suggesters (automutate/autolesshint#51).

---

Since this PR hasn't been met with confetti emojis and open arms (and is super out of date), how about the following:

• I write a version of "autostylelint" conceptually similar to autolesshint
• We wait until autostylelint gets to (near-?) feature parity with stylefmt
• We re-evaluate whether the logic from autostylelint should be merged into stylelint itself or kept separate

---

Edit: @davidtheclark, just saw your comments in #2259 - would you consider this PR relevant to the roadmap / discussion there?

[davidtheclark]

@JoshuaKGoldberg: As discussed in #2259, I'm pretty convinced now that the most important Big New Feature stylelint can work on is autofixing. So let's get down to business and make this happen ...

First, thanks for pitching the idea, working on the proof-of-concept, and persisting. Here are some appropriate emojis: 🎊 🎉. It's awesome that you are interested in this problem and dedicated to making it happen by maintaining automutate and working on stylelint integration.

I have a clarifying question for you: Can you please explain a little more about the complexities of autofixing that are abstracted into automutate? The README is understandably brief. I'm curious what problems a naive implementation would bump up against — the problems you know of already that automutate will solve (besides just the general benefit of possibly sharing code with similar tools).

I think we should hammer out a single complete proof-of-concept to fully illustrate how this proposal would work. Here's what I think that would entail:

• One rule with complete autofixing, for every option.
• Demonstration of where automutate would be invoked during the stylelint process to actually change the file.
• Tests demonstrating that the autofixing works.

@JoshuaKGoldberg does that sound like a good plan? I think it could be more immediately productive for this conversation than you working on a new tool, since we are coming around to the idea of including autofixing logic within stylelint itself.

[JoshuaKGoldberg]

You know, I wasn't trying to fish for emojis, but now that they're there... they are nice. Thanks 😄

the complexities of autofixing that are abstracted into automutate

IMO the main benefits here would be a clean code base & conflict resolution.

Suppose you have a chunk of code with multiple linter complaints:

.foo{ color	: rgba(255, 0, 0) }

A sensible configuration might autofix it to something like:

.foo {
    color: red;
}

Doing that mutation requires, in order of starting position:

1. Add an endline (after { / at position 6)
2. Add three spaces (before color / at position 7)
3. Remove the tab (after color / at position 12)
4. Replace (rgba(255, 255, 0) / at position 15) with red
5. Add a semicolon (after rgba(255, 255, 0) / at position 30)
6. Add an endline (after rgba(255, 255, 0) / at position 30)
7. Remove the space (after rgba(255, 255, 0) / at position 30)

A bare-bones procedural walker like stylefmt would ironically have less trouble (though it's likely there would be edge cases to consider), but how would you resolve the varying pieces of logic within the nicely separated rules that stylelint uses? Two options:

1. Apply fixes while walking the AST, similar to what stylefmt does
2. Report the suggested fixes and later apply them all at once

Three problems with option 1:

• It increases the dependency complexity of linter code. Instead of being a nicely structured functional-programming-friendly (nodes+config) -> complaints setup, you're modifying the inputs.
• It's harder to test. You have a great setup with POJO reports; testing autofixing in a non-functional manner would only really be elegant with BDD-style file comparisons.
• It doesn't interop well with external utilities. One of my favorite features of VSCode's TSLint plugin is that it will offer to perform fixups with happy little popups. Integrating that kind of feature with tree modifications would require diffing the pre- and post-linter AST or something similarly difficult.

Option 2 solves all those issues. Sending suggestions back as POJO maintains the functional aspect, is easy to test, and is easy to integrate with externally.

One potential issue with 2 is conflicting changes. A POJO report that describes these operations would either contain the node(s) they apply to or the character positions. Either way could become invalid by the source mutating. In the example above, we're defining multiple operations on the rgba(255, 0, 0) node. Once that changes, the character positions of any following rules become invalid and the node itself disappears.

These types of suggestion conflicts are why it's necessary to re-run a suite of linters multiple times. There's no problem with dropping conflicting suggestions when they'll just be re-reported on a subsequent wave. Furthermore, this allows rules to only report their own fixes, and trust that subsequent runs will fix up whatever lesser complaints they create. See my code comment above. Note that this is an issue with option 1 above as well, so no matter what you'll be needing to re-run the linter.

You could build the logic for conflict resolution, applying fixes, and re-running directly into stylelint... or you could let another tool do it.

a single complete proof-of-concept

Agreed, and thanks for considering it! I think I'll start with color-hex-case for simplicity's sake.

[davidtheclark]

@JoshuaKGoldberg Thanks for that explanation! The need to re-run the linter multiple times adds some additional importance to @sergesemashko's work on caching (#2293).

[hudochenkov]

Thank you, @JoshuaKGoldberg, for ideas!

I was thinking about something similar to your option 2. My idea: a rule will have a fix function. This function will have (almost) the same logic as linting. How it will work:

1. Stylelint runs a rule. If rule report something, then rule adds its fix function to some fixIt array.
2. After linting happend as usual, stylelint will run every fix function from fixIt array over whole style sheet. It will be a little slower, but error-prone. User usually need only few fixes, if uses stylelint regularly.

I'm kind of do the same thing in postcss-sorting (run every specified option over whole style sheet one by one).

Also this approach can be easily applied and for plugins.

As for testing. We can run every rejected test over fix function and than lint result with the same config. It shouldn't throw warnings. Plus we'll add more tests if needed for fix testing only.

[JoshuaKGoldberg]

@davidtheclark is this what you were thinking of? 14a50e8 adds support for suggesting fixes; f0388d7 shows how a rule would add suggestions.

The suggestFixes generation and tests are a little verbose for the sake of visibility into what they're actually doing. You could always have utilities to generate those objects.

suggestedFix: fixes.swap("ababa", 22, 28);

I'm not envisioning automutate being invoked during this process at all. In my vision, your code wouldn't take on any new dependencies. There would be an external tool, autostylelint, that receives these suggested fixes and applies them using automutate.
To use automutate (or any other tool, homegrown or external) to apply waves of suggestions, it looks like you'd need to change cli.js and/or standalone.js to be able to keep linting and applying fixes. That's a lot of extra complexity and test overhead.

@hudochenkov that could work too, but it feels like a lot more work than directly passing suggested fixes around in the messages. How would you expose the results to external tools such as editor plugins? Would there be a separate blob of suggested fixes, or would they be added later?

As for testing that suggested fixes stop the violations, while that also could work (and is a nifty idea!), you'd miss out on behavior changes in the rule and potential negative side effects. Using the rgba(255, 0, 0)/red example from above, if someone messed up an update to the rule and made it output yellow instead of red, it would pass those tests. You'd end up having to still write direct tests for the suggested fixes... which would render the full-scale tests a little redundant IMO.

[davidtheclark]

There would be an external tool, autostylelint, that receives these suggested fixes and applies them using automutate.

Would autostylelint also print linting warnings (those that could not be fixed)? Or would the user run two separate tools — probably autostylelint and then stylelint?

Is autostylelint going to provide APIs that are exactly the same as stylelint's, but it will apply the fixes? What are you thinking about all the integrations (e.g. editors, gulp/grunt, webpack)?

For those reasons, I worry that putting the logic in a separate package might actually end up making things more complex than putting the logic here. If you still disagree, can you say more about what you envision the end-user experience to be?

[davidtheclark]

Adding a third argument actually opens up a wide range of possibilities ... Instead of limiting that range by making this string the config object, maybe the argument should an object with a config property. If we find other uses for this argument (e.g. we could attach the internal stylelint API to it), we could then add them to the object without a breaking change or more arguments.

[JoshuaKGoldberg]

Agreed!

[davidtheclark]

Should this be hexValue.length? I guess in this case it probably should not matter; but in other cases, we'll want to make sure we remove the whole original string when we replace it with another that might be of a different length.

[JoshuaKGoldberg]

Agreed!

[davidtheclark]

This can be cleaner when we redefine Flow types for the new third argument. We can do that later.

[hudochenkov]

How would you expose the results to external tools such as editor plugins?

@JoshuaKGoldberg I'm not familiar with much linters, only with ESLint. In CLI when it runs with --fix flag is fixes what it can and shows unfixable warnings. In Sublime Text there is ESLint-Formatter it just fixes what it can. So I'm not sure what other goal you want achieve other then fixing stylesheets.

[JoshuaKGoldberg]

Would autostylelint also print linting warnings
two separate tools
APIs identical to stylelint's

Now that you mention it, that is a bit ugly. On the one hand, keeping it all internally would require more of a rewrite. I wouldn't be a good person to do that (I can barely keep up with Flow in this limited form!). On the other hand, you're absolutely right that it could get hairy this way. In a perfect world, I think the best user experience would be to have it all be in stylelint as a --fix parameter or similar. You could even have individual settings for rules for whether or in what ways they're auto-fixable...

...but taking a step back a bit, would you be willing to move that discussion to a separate issue? You and the rest of the stylelint team are definitely far more qualified than me to answer that, and I don't think it will affect the design of how the fixes are suggested. No matter what stylelint will need to have some way of reporting the suggestions, and what's in the PR now should work for either of the above.

other goal you want achieve other than fixing stylesheets

Displaying what the fixes are without applying them is one.

I guess I'm not entirely clear on your proposal. Is the difference that you want to delay calling fix/suggestFix until after all rules are done reporting? Is there a benefit to this over calling them at the same time?

[davidtheclark]

@JoshuaKGoldberg: The example you have here with color-hex-case looks great to me. The addition is unobtrusive, clear, and easy to test. What do you other @stylelint/core members think?

You're right that the usage of automutate is a different concern than the description of fixes. But I don't want to merge this code into stylelint until we know how we're going to use it.

automutate does not seem to have a documented API — am I missing something? At some point soon I could investigate more about what you mean when you say we'd have to refactor stylelint APIs so we could "keep linting and applying fixes", but it's hard to do that without knowing how I'd use automutate.

[hudochenkov]

I guess I'm not entirely clear on your proposal. Is the difference that you want to delay calling fix/suggestFix until after all rules are done reporting? Is there a benefit to this over calling them at the same time?

I probably don't understand your ideas, because I'm not very experienced and these ideas are very new for me and complicated.

My idea is more simple, I believe. Lint as it was before, but for rules which have fix function we won't show warnings. We'll collect all fix functions which were reported by rule and than run every collected fix one by one over style sheet. So we won't show any suggestions, we just fix style sheet if we know we can fix it. I think I should make a new PR with my idea in action.

[davidtheclark]

@hudochenkov: Sure, if you feel make making a proof-of-concept, it would be interesting. I think that a PostCSS-based tool like stylelint has a major difference over other linters automutate might target: PostCSS is built especially for transformations, not just AST analysis. So your idea probably makes more sense for stylelint than it might for other linters.

However, I do think your proposal might less scalable for testing. By using a suggestions contract, as with automutate, the rule does not have to test anything but the object of suggested fixes. Then the fixer would test that it applies suggestions correctly. If, on the other hand, the fix is a function, every rule would need to test that the function it's provided actually fixes things correctly.

[Arcanemagus]

@hudochenkov Something to consider: What you are describing is similar to the model ESLint uses, but is missing one of the key aspects of it, and the discussion earlier in this thread: Rules can (and will!) conflict with each other. ESLint attempts to work around this by applying a first pass of fixes, with any rule that had it's desired range of change already modified being discarded. It then will run the lint again over the new code, apply fixes, and repeats this process until either no more fixes are returned or it has ran 10 times (to prevent conflicting rules from putting the code into an infinite loop). This also relates to the "move fixing to an external utility" discussion.

I'm not sure I'm sold on the automutate method of specifying the changes a rule would like to make as it still seems to me that all of the different "types" it proposes are just expansions of the text-swap type, but if that's the format that is decided on it's certainly usable.

[JoshuaKGoldberg]

@davidtheclark you're right, the documentation is pretty minimal. I've written up a basic onboarding page. Is the technical-implementation section what you were looking for? If not, I'm curious (for my own purposes too) about what other info is lacking.

As for a little more detail on refactoring to re-run internally, a few thoughts...

• standalone.js' export encapsulates all your linting logic. Awesome.
• You could create a new file + export, repeatedStandalone.js, that:
  • Takes in the same settings as standalone
  • Runs Automutate with a provider that itself runs standalone and returns the suggested fixes as mutation waves
• Change cli.js to call this repeatedStandalone if a --fix setting is passed

You wouldn't have to change any existing external files or APIs other than to add a --fix option.

@Arcanemagus this is true! I was on the fence about having so many variants that boil down to text-swap. What eventually swayed me over to keeping the multitude is the improved clarity the suggestions bring. It's a bit easier to understand that a suggestion wants to (add|remove|swap|etc.) text when its type explicitly says it. IMO it's a bit less overhead for developers to think through.
That being said, I'm absolutely open to discussing changing the spec if you have a proposal you prefer.

By the way, thanks for bringing up that ESLint has a maximum repetition in case of infinite loops. Automutate issue: 👇

[davidtheclark]

@JoshuaKGoldberg Thanks! Not sure when I'll be able to dig into this — maybe later in the week or this weekend.

[davidtheclark]

@JoshuaKGoldberg I took a look. First problem is that I can't run the automutate code in Node 4. It does not seem to be compiling away the unsupported syntax, like spread operators and default parameters.

So I didn't get very far in the experiment. But I will note that even with the documentation this seems very unidiomatic and difficult, probably more so than it needs to. If all I'm really specifying is a provider() function, could that just be an argument I pass to a function in the API, or an option for a single constructor? Why the more elaborate setup with multiple classes and in interface?

It's striking that there's not a realistic start-to-finish example to work from — right? Is it the responsibility of the provider() to overwrite the file? Would MyMutationsProvider#provide run endlessly because it's always reading the same file and never modifying it?

[JoshuaKGoldberg]

can't run the automutate code in Node 4

Should be fixed in 0.5.1. When I started on automutate, TypeScript didn't yet support async/await output downpiled to ES3/5...

If all I'm really specifying is a provider() function, could that just be an argument I pass to a function in the API, or an option for a single constructor?
Why the more elaborate setup with multiple classes and in interface?

Because I don't know how all the cases for how this'll be configured or used. Maybe settings for maximum iterations, or alternate file I/O (streams / virtual files), or ...?
You're right that it's a little verbose though. I've switched the onboarding doc to prefer composition over inheritance.

Is that what you were referring to?

Is it the responsibility of the provider() to overwrite the file?

automutate takes care of that. After each provide() call, the suggestions are applied to the files on disk. The next provide call will then work on the updated files. At that point either there'll be no more fixable complaints (program closes) or the next round will be applied, and the cycle goes again.
I've put some more info on that in the readme#how-it-works. Thanks for bringing it up.

start-to-finish example to work from

I've started work on a separate autostylelint and will post it here when it's workable. That should be pretty soon.

I'm also glad you're pushing for an actual start-to-finish example, because that's exposed a flaw in my assumptions from earlier. The position field on complaints is the column - not the overall character position in the file. I'll adjust the PR files...

Edit 2/25: https://github.com/automutate/autostylelint. Running is done via its runner.ts::run; linting & suggestions by its stylelintMutationsProvider.ts.

[Arcanemagus]

applied to the files on disk

This is one of the primary reasons I'm against having this as an external tool. You should avoid disk I/O if at all possible. If this is going to be an external tool then it should be using the API and passing text around until the process is completed and only then writing the file to disk.

[JoshuaKGoldberg]

All right:

• Updated the PR to output an object with line and column for the begin and end suggestion positions
• Added processing logic to autostylelint to convert those lines and columns to their absolute positions.

automutate doesn't allow for taking in lines & columns because I don't want to restrict it to files with traditional line/column concepts. Logic in https://github.com/automutate/autostylelint/tree/master/src/processing instead converts from local (line/column) to absolute (position).

avoid disk I/O if at all possible

Indeed! getPostcssResult.js uses fs to read the file from disk. You'd have to do something around the lines of making a file manager that wraps the file operations and can modify the in-memory file for the suggested changes. automutate has a hook to deal with such a thing, it that helps.

[davidtheclark]

We've decided to launch down an alternative path, started by #2427 and #2467.