search

llvm / llvm-project

The LLVM Project is a collection of modular and reusable compiler and toolchain technologies.
http://llvm.org
Other
28.71k stars 11.87k forks source link

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

Open llvmbot opened 4 years ago

llvmbot commented 4 years ago
Bugzilla Link 45150
Version unspecified
OS Linux
Reporter LLVM Bugzilla Contributor
CC @sylvestre

Extended Description

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.

llvmbot commented 4 years ago

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

llvmbot commented 4 years ago

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

llvmbot 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.

llvmbot 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

llvmbot commented 4 years ago

Bug llvm/llvm-project#31360 has been marked as a duplicate of this bug.

llvmbot 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

llvmbot 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.