search

Quuxplusone / LLVMBugzillaTest

0 stars 0 forks source link

export-fixes to yaml adds extra newlines and breaks offsets #44120

Open Quuxplusone opened 4 years ago

Quuxplusone commented 4 years ago
Bugzilla Link PR45150
Status NEW
Importance P normal
Reported by Alexander Lanin (llvm@alex.lanin.de)
Reported on 2020-03-09 07:34:54 -0700
Last modified on 2020-03-25 02:44:50 -0700
Version unspecified
Hardware PC Linux
CC alexfh@google.com, djasper@google.com, klimek@google.com, N.James93@hotmail.co.uk, sylvestre@debian.org
Fixed by commit(s)
Attachments
Blocks
Blocked by
See also

Summary: It seems that the --export-fixes option is adding unexpected extra newlines to the generated output. Even worse those added characters are not counted when calculating following FileOffsets, so files with >= 2 fixes get completely broken. As far as I can tell this is not verified anywhere in the test suit, as all lit tests are always using -fix directly.

export-fixes is used e.g. by run-clang-tidy which is the one way to run clang-tidy with auto-fix option according to all sources on the internet.

Reproduction: These two produce different results:

bin/clang-tidy -checks="-*,readability-braces-around-statements" --fix ../clang-tools-extra/test/clang-tidy/checkers/readability-braces-around-statements.cpp

mkdir temp && bin/clang-tidy -checks="-*,readability-braces-around-statements" --export-fixes=temp/fixes.yaml ../clang-tools-extra/test/clang-tidy/checkers/readability-braces-around-statements.cpp && bin/clang-apply-replacements temp

Breaking code: Given void f(){if(1) if(2) return;} run mkdir temp && in/clang-tidy -checks="-*,readability-braces-around-statements" --export-fixes=temp/fixes.yaml ../clang-tools-extra/test/clang-tidy/checkers/readability-braces-around-statements.cpp && bin/clang-apply-replacements temp And you will end up with: `void f(){if(1) { if(2) { return;

}} (there is one}` missing).

This does not happen when running clang-tidy with -fix option.

Quuxplusone commented 4 years ago

Part 1 is mostly fixable by reverting 080014ee6 from https://reviews.llvm.org/D63482. It's not quite clear to me why that change was introduced.

Maybe this is even unrelated to issue 2.

Quuxplusone commented 4 years ago 
Surely there is an issue that its not escaping the text as well.
https://godbolt.org/z/F9qP6x
I'm not sure how clang-apply-replacements works but I feel they should both
consistently escape and un-escape the yaml text
Quuxplusone commented 4 years ago

_Bug 32012 has been marked as a duplicate of this bug._

Quuxplusone commented 4 years ago

This https://reviews.llvm.org/rG7693a9b9314083eafd9b5b1e19b02aac06962eb2 should fix the issue. If it works please close this. The extra new lines shouldn't cause concern as they are currently removed when parsing the yaml file. Though a patch is in works to escape them

Quuxplusone commented 4 years ago 
@njames93 did you link the wrong diff here? Or the correct diff to the wrong
bug?
To be sure I retested this bug with current master (5f5fb56c68e). No change.

This bug will be fixed via D76037 and D76054 once they are merged.
Quuxplusone commented 4 years ago

I did, whoops. I haven't even committed the correct fix, will sort that out now.

Quuxplusone commented 4 years ago

Here you go https://reviews.llvm.org/rG6538b4393dc3e7df9fee2b07eba148d4cf603a24