Edit linting: use line mapping to rule out irrelevant errors #3649
Conversation
Fix a potential issue that might lead to file corruption when edit linting is enabled #3124 introduces a feature for editing: running linter twice before and after the change and only extract new errors introduced by the agent. This has some potential issues and I am working on #3649 to address them, but I feel like I am not gonna finish it in the next few days, and that PR has become harder and harder to review, thus this PR, which only focuses on a small improvement. So what's the issue? When we run linters on the original file before our edits, we need to copy the original file and use a temporary file to lint, because linting may have side-effect (e.g. modifying the file in-place). I used the word "may" because: Flake8 has no side-effect, so not a problem as of now. We don't enforce this or document this "no side-effect" as a requirement for linter implementation, so side-effect is allowed. Regardless, the "after-edit-linting" uses the same approach: backup the file before linting to avoid data corruption. We should keep our "before-edit-linting" consistent. Why no new unittest that reproduces the issue? Well, as I have mentioned earlier, flake8 has no side-effect, so technically it's not a bug but a flaw. Therefore, there's no way to write a test that reproduces the issue.
|
Great job on this tricky enhancement! |
|
Great work! Since the tests run fine, I'd say LGTM. |
|
sounds good! I've finally got some of the eval pipeline working properly! Will start an eval on this PR today |
|
Just fyi, I did 15 different Aider bench instances and don't see new issues from this PR. 👍 |
|
Weird enough, this PR actually brings a lot of degradation.. The current |
|
Why do you get those "mv: cannot stat" errors? 🤔 |
|
@tobitege those are un-related eval scripts things 🤣 |
|
@li-boxuan please buzz us again once this is ready |
Short description of the problem this fixes or functionality that this introduces. This may be used for the CHANGELOG
Please read #3412 for more detail. To simply put, previous work uses the diff of linting error messages from pre-edit-lint and post-edit-lint to rule out "irrelevant" linting errors. The biggest flaw is that error messages could be generic - leading to both false positives and false negatives. This PR uses a more robust approach: line numbers.
Give a summary of what the PR does, explaining any non-trivial design decisions
Before edit, we run linting once, and record the error messages from the linter. After edit, we run linting once again, and also record the error messages. We then compare the line numbers from both pre-edit-lint and post-edit-lint. We rule out errors that are irrelevant to the edit (i.e. we remove errors that exist in both pre-edit-lint and post-edit-lint). This is non-trivial because line numbers might change. Luckily, since we only allow edits at a single place at a time, it is not too hard to figure out line mapping.
Link of any specific issues this addresses
#3412