{"action":"created","issue":{"url":"https://api.github.com/repos/eslint/eslint/issues/5329","repository_url":"https://api.github.com/repos/eslint/eslint","labels_url":"https://api.github.com/repos/eslint/eslint/issues/5329/labels{/name}","comments_url":"https://api.github.com/repos/eslint/eslint/issues/5329/comments","events_url":"https://api.github.com/repos/eslint/eslint/issues/5329/events","html_url":"https://github.com/eslint/eslint/issues/5329","id":134715189,"number":5329,"title":"Fixing autofix","user":{"login":"nzakas","id":38546,"avatar_url":"https://avatars.githubusercontent.com/u/38546?v=3","gravatar_id":"","url":"https://api.github.com/users/nzakas","html_url":"https://github.com/nzakas","followers_url":"https://api.github.com/users/nzakas/followers","following_url":"https://api.github.com/users/nzakas/following{/other_user}","gists_url":"https://api.github.com/users/nzakas/gists{/gist_id}","starred_url":"https://api.github.com/users/nzakas/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/nzakas/subscriptions","organizations_url":"https://api.github.com/users/nzakas/orgs","repos_url":"https://api.github.com/users/nzakas/repos","events_url":"https://api.github.com/users/nzakas/events{/privacy}","received_events_url":"https://api.github.com/users/nzakas/received_events","type":"User","site_admin":false},"labels":[{"url":"https://api.github.com/repos/eslint/eslint/labels/breaking","name":"breaking","color":"e11d21"},{"url":"https://api.github.com/repos/eslint/eslint/labels/core","name":"core","color":"c7def8"},{"url":"https://api.github.com/repos/eslint/eslint/labels/needs%20bikeshedding","name":"needs bikeshedding","color":"f7c6c7"}],"state":"open","locked":false,"assignee":null,"milestone":{"url":"https://api.github.com/repos/eslint/eslint/milestones/14","html_url":"https://github.com/eslint/eslint/milestones/v3.0.0","labels_url":"https://api.github.com/repos/eslint/eslint/milestones/14/labels","id":1459693,"number":14,"title":"v3.0.0","description":"","creator":{"login":"nzakas","id":38546,"avatar_url":"https://avatars.githubusercontent.com/u/38546?v=3","gravatar_id":"","url":"https://api.github.com/users/nzakas","html_url":"https://github.com/nzakas","followers_url":"https://api.github.com/users/nzakas/followers","following_url":"https://api.github.com/users/nzakas/following{/other_user}","gists_url":"https://api.github.com/users/nzakas/gists{/gist_id}","starred_url":"https://api.github.com/users/nzakas/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/nzakas/subscriptions","organizations_url":"https://api.github.com/users/nzakas/orgs","repos_url":"https://api.github.com/users/nzakas/repos","events_url":"https://api.github.com/users/nzakas/events{/privacy}","received_events_url":"https://api.github.com/users/nzakas/received_events","type":"User","site_admin":false},"open_issues":4,"closed_issues":0,"state":"open","created_at":"2015-12-14T17:09:40Z","updated_at":"2016-02-18T23:06:09Z","due_on":"2017-04-30T07:00:00Z","closed_at":null},"comments":12,"created_at":"2016-02-18T22:27:15Z","updated_at":"2016-02-22T23:06:56Z","closed_at":null,"body":"There have been a few issues recently with people asking to add more autofixing. This is definitely something we want to address, but we can't do it with the way autofixing works right now. This (unfortunately long) issue is meant to outline what we currently have, its limitations, and start towards thinking about how to fix autofix so we can use autofix in as many places as possible.\r\n\r\n## Intro\r\n\r\nWhen ESLint first implemented autofixing, our belief was that it was primarily useful for making small changes, mostly whitespace and anything that could be contained within a single token. Most of the complaints people had about ESLint rules was that they hated going through and fixing alignment or adding/removing semicolons but loved the error catches. So naturally, I thought if we could get the whitespace and other small autofixes in, that would make people happy. It did, it's just that then we started getting requests to autofix more rules and we realized that the current system doesn't scale well.\r\n\r\n## How it Works Today\r\n\r\nToday's autofix is based on text and is latched onto how ESLint has always worked: \r\n\r\n1. ESLint makes a single traversal through the AST of the file.\r\n1. Rules listen for different nodes and evaluate the AST as it goes, producing messages when it finds something wrong.\r\n1. Rules can optionally specify a way to fix each message it reports. The fix is indicated by range offsets (the same as used in the `range` object on AST nodes). This fix is *not* guaranteed to be applied and will not even be attempted until after the complete traversal is finished.\r\n1. After the traversal is complete, all of the proposed fixes are sorted in descending order of range location. So a fix at location 10 comes before a fix at 5.\r\n1. The fixer attempts to apply the fixes in this order. If two fixes overlap in range, then only the first is applied. This continues until all potential fixes have been processed.\r\n1. If a fix is applied, then that message is removed from the lint results of the file.\r\n1. The remaining lint results are presented to the user.\r\n\r\nBecause of the single traversal, we can't apply the fixes as we go because it would affect other rules as it goes. For instance, if we changed a `let` to a `const`, that then pushes the ranges of everything in the tree by two, so when does that calculation happen? Also, each rule is triggered multiple times at different nodes, and many of the rules try to track things in between those triggers - the built-in assumption is that the tree looks the same as the first time the rule is triggered.\r\n\r\n### Problems\r\n\r\nDue the single traversal mode, we have several problems:\r\n\r\n1. It's possible that not all fixes will be applied.\r\n1. It's possible for a fix to be applied that violates a different rule. For instance, one rule could insert a comma while another requires a space after a comma. The rule requiring the space has no idea that a new comma was added because it's done after the rule has already executed.\r\n1. It's possible for a fix to change scope evaluation. For instance, changing `var` to `const` has implications based on the functions or block statements using it. That can invalidate a whole host of rules because scope calculations are done once, before the traversal.\r\n1. It's possible for a fix to introduce an error into the program by changing the meaning of the code.\r\n\r\nAs a result of these problems, we decided to limit what we'd allow as fixes in core rules to whitespace and other small changes. Semicolons were a big pain point, so even though they aren't whitespace, we determined the value of fixing these was high enough to be worth any potential other problems. The same with string literals. We actually removed autofixing for `===` after we realized it would change the meaning of the condition and cause a runtime error. We then decided that any new autofixes would just be whitespace to avoid any other complications.\r\n\r\n## Where I Went Wrong\r\n\r\nLooking back, I think I made several design decisions that were incorrect. These are the things I think I got wrong (and I use \"I\" here since I came up with the design):\r\n\r\n1. Autofix mode should just do fixes and not worry about unfixable rules. Right now it acts just like linting, outputting any messages it didn't fix. That seems unnecessary.\r\n1. The text-based fixes were a naive approach to autofixing that was targeted at whitespace fixes as a primary use case. It's clear text-based fixes aren't scaleable.\r\n1. Trying to fit autofixing into the same single traversal system as the linting is too limiting.\r\n\r\n## The Future\r\n\r\nSo now that I've explained the current state of stuff, I think we have an opportunity to fix it (no pun intended). This will likely be a big breaking change, but hopefully will be better for what people really want to use autofix for. I don't know the implementation details, but I have a high-level of idea of where I'd like to see autofix go.\r\n\r\n### Requirements\r\n\r\n1. Autofix mode just does fixes and does not do any unfixable rule linting at all. (That means `--fix` doesn't output any linting problems.)\r\n1. Autofix logic for each rule is contained with the rule file that does linting, not it another file.\r\n1. Autofix logic in rules is declarative and doesn't force rules to do something like `if (mode === \"fix\") { fix() }`. Basically, we can't throw away the way rules work today, there are too many core rules and plugins with rules to force everyone to rewrite in a dramatic way.\r\n1. Multiple autofix passes can be done on the same file to ensure as many fixes as possible are applied.\r\n1. Regardless of fixes, ESLint must never output a file with illegal syntax as the final result.\r\n1. Autofix should work for custom nodes that aren't part of the ESTree spec (I'm thinking about Flow, TypeScript, and other derivatives that add custom nodes into an ESTree AST). \r\n\r\n### Ideas: Config\r\n\r\nOne of my ideas is that you should be able to specify which rules you want autofix to apply for, so in your config file, you'd do something like:\r\n\r\n```yaml\r\nrules:\r\n    semi: \"fix\"\r\n    quotes: [\"fix\", \"double\"]\r\n```\r\n\r\nThis would be an extension of the work done in https://github.com/eslint/eslint/issues/3626 to allow \"off\", \"warn\", and \"error\" as options instead of 0, 1, and 2. \r\n\r\nI think this would be a big win because it gives us a good idea about how to save time: during fixing, we just load the rules marked as \"fix\" and ignore the rest. That should cut down on the runtime (so long as not every rule is fixable!)\r\n\r\n**Open Question:** When not running in fix mode, what does \"fix\" mean? Is it \"warn\" or \"error\"?\r\n\r\n### Concerns\r\n\r\nSome other concerns I have with changes are:\r\n\r\n1. Is it possible to do multiple fixes in a single traversal? Or do we need to do one rule's fixes at a time?\r\n1. Do we need to do nonwhitespace rules first and whitespace rules second? Or do we just bite the bullet and say we always need at least two passes per file to get as many fixes as possible? (Or just keep doing passes until all fixes are applied?)\r\n1. How will this work with code path analysis? Changing nodes around affects code paths dramatically.\r\n1. How will this work with escope and scope evaluation? Will we have to run scope analysis multiple times? How will that affect performance\r\n\r\n## Next Steps\r\n\r\nThis is just the beginning of the conversation, there are a ton of details that need to be hashed out. I just wanted to share this to ensure everyone that we are thinking about it and we do want to make improvements.\r\n"},"comment":{"url":"https://api.github.com/repos/eslint/eslint/issues/comments/187430271","html_url":"https://github.com/eslint/eslint/issues/5329#issuecomment-187430271","issue_url":"https://api.github.com/repos/eslint/eslint/issues/5329","id":187430271,"user":{"login":"nzakas","id":38546,"avatar_url":"https://avatars.githubusercontent.com/u/38546?v=3","gravatar_id":"","url":"https://api.github.com/users/nzakas","html_url":"https://github.com/nzakas","followers_url":"https://api.github.com/users/nzakas/followers","following_url":"https://api.github.com/users/nzakas/following{/other_user}","gists_url":"https://api.github.com/users/nzakas/gists{/gist_id}","starred_url":"https://api.github.com/users/nzakas/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/nzakas/subscriptions","organizations_url":"https://api.github.com/users/nzakas/orgs","repos_url":"https://api.github.com/users/nzakas/repos","events_url":"https://api.github.com/users/nzakas/events{/privacy}","received_events_url":"https://api.github.com/users/nzakas/received_events","type":"User","site_admin":false},"created_at":"2016-02-22T23:06:56Z","updated_at":"2016-02-22T23:06:56Z","body":"@mikesherov are you asking me if you're correct or suggesting that ESLint might do that?\r\n\r\n"}}