Bug 365437 - Rename local variable using "Rename" assistant breaks code
Summary: Rename local variable using "Rename" assistant breaks code
Status: RESOLVED FIXED
Alias: None
Product: kdevelop
Classification: Applications
Component: Language Support: CPP (Clang-based) (show other bugs)
Version: 5.2.3
Platform: Arch Linux Linux
: NOR normal
Target Milestone: ---
Assignee: Igor Kushnir
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2016-07-11 21:52 UTC by Piotr Mierzwinski
Modified: 2023-09-15 09:17 UTC (History)
3 users (show)

See Also:
Latest Commit:
Version Fixed In: 5.13.231200


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Piotr Mierzwinski 2016-07-11 21:52:04 UTC
There is following code:
void MyClass::foo( bool param )
{
	bool stat;
	QString string;
	if (param) {
		if (string.isEmpty())
			stat = true;
		else
			stat = false;
	}
}

I rename "stat" variable to "stat2". Choosing "Rename stat to stat2" makes that in code are inserted two (opening) curly brackets.

Reproducible: Always

Steps to Reproduce:
1. Rename local variable what is used inside of block of code like above
2. Choose "Rename old_name to new_name"
3.

Actual Results:  
void MyClass::foo( bool param )
{
	bool stat2;
	QString string;
	if (param) {
		if (string.isEmpty())
			 {
            stat2 = true;
		else
			 {
            stat2 = false;
	}
}

Expected Results:  
void MyClass::foo( bool param )
{
	bool stat2;
	QString string;
	if (param) {
		if (string.isEmpty())
			stat2 = true;
		else
			stat2 = false;
	}
}


KDevelop and KDevPlatform cloned and built from branch 5.0, at 08-07.2016.
Plasma 5.7.0, KFrameworks 5.23, Qt-5.7.0
Comment 1 Piotr Mierzwinski 2016-07-11 21:55:16 UTC
Second example:
The same, as described above, is happened when I try to rename argument "e" in below function to "e2"

void MyClass::foo2( QEvent *e )
{
	bool stat = true;
	int a = 1;
	if (stat) {
		if (a == 1)
			e->accept();
		else
			e->ignore();
	}
}

After using Rename assistant I get following code:
void MyClass::foo2( QEvent *e2 )
{
	bool stat = true;
	int a = 1;
	if (stat) {
		if (a == 1)
			 {
            e2->accept();
		else
			 {
            e2->ignore();
	}
}
Comment 2 Piotr Mierzwinski 2017-09-01 20:50:12 UTC
This also happens after using refactoring function ("Rename declaration  Ctrl+
Shift+R"), so is possible that refactoring will break the code :/.

Sometimes inside of function is put closing curly bracket when not exists matching opening bracket - "lost closing curly bracket".
For example such code:

	if (condition)
		one_line_of_code;
	else
		different one_line_of_code;

	    }

	renamed_member;


couple lines above might be block of code with curly bracket like:
	if (condition) {
		one_line_of_code;
		second_line_of_code;
	}

Lonely curly bracket above is "lost closing curly bracket".
Additionally this curly bracket has indention made by spaces where in code all are made by tabs.
Of course mentioned curly bracket is not necessary, only breaks code.
Comment 3 Piotr Mierzwinski 2017-09-01 20:58:52 UTC
Some clarification for last post.
I met this issue also when mentioned previously code with curly bracket not happened, so wasn't there this code:
	if (condition) {
		one_line_of_code;
		second_line_of_code;
	}

function (after refactoring) looks like this:

void MyClass:foo()
{
	if (condition)
		one_line_of_code;

	second_line_of_code;
	third_line_of_code;


	if (condition)
		one_line_of_code;
	else
		different one_line_of_code;

	    }

	global_renamed_member += value;

	other_line_of_code;
}
Comment 4 Piotr Mierzwinski 2017-09-01 21:08:22 UTC
One more example where refactoring breaks code, sorry for so many comments, but "Renaming declaration" destroyed my code in many places :(.

		if (condition1) {
			one_line_of_code;

			if (condition2) {
				if (! condition3)
					one_line_of_code;

//				one_line_of_code;
			}
			else {
				if (condition4)
					one_line_of_code;
			}
			}
			global_renamed_member += member * (condition ? 1 : -1);
		}

Of course curly bracket put above "global_renamed_member" is not necessary and breaks code.
Comment 5 Piotr Mierzwinski 2017-09-01 21:19:40 UTC
Being in Review mode I made corrections for this "lost curly bracket". I remove curly bracket jumping "Next", remove curly bracket, "Next" and after using "Prev" KDevelop just crashed generating not usable backtrace :(.
Comment 6 Igor Kushnir 2020-03-20 05:15:42 UTC
I have experienced similar Rename bugs in KDevelop 5.5.0:
    1) it always inserted spaces between the renamed variable and parentheses;
    2) sometimes it inserted out-of-place braces.

After some debugging I determined that Source Formatter kicks in during the Rename variable refactoring in DocumentChangeSetPrivate::generateNewText():

    if (formatter && (formatPolicy == DocumentChangeSet::AutoFormatChanges
                              || formatPolicy == DocumentChangeSet::AutoFormatChangesKeepIndentation)) {
... }


GrepOutputModel sets formatPolicy to DocumentChangeSet::NoAutoFormat, so Find/Replace in Files does not cause such bugs. Neither does simple KTextEditor's Replace within a file.

It turned out that my Source Formatter for the C++ Language was set to the default Artistic Style->1TBS. After switching to Artistic Style->Qt, at least the easily reproducible space insertion issue is gone. Not sure about the spurious braces yet.

Piotr, have you tried reproducing this bug and Bug 317299 with different C++ Source Formatters configured in KDevelop settings?
Comment 7 Igor Kushnir 2020-03-20 09:50:29 UTC
I see that my braces bug is the same as Piotr's. It is triggered by a brace inserted by a formatter just before the renamed variable. The bug is in extractFormattedTextFromContext():
    endOfLeftContext = matchPrefixIgnoringWhitespace(formattedMergedText, leftContext, fuzzyCharacters);
The inserted brace ends up after endOfLeftContext and sneaks into code along with the renamed variable. The bug does not depend on the source formatter - happens with all of them.

I'll try to fix this bug by skipping the brace and associated extra line break, etc.
Comment 8 Piotr Mierzwinski 2020-03-20 21:18:41 UTC
(In reply to Igor Kushnir from comment #6)
> I have experienced similar Rename bugs in KDevelop 5.5.0:
>     1) it always inserted spaces between the renamed variable and
> parentheses;
>     2) sometimes it inserted out-of-place braces.
> 
> After some debugging I determined that Source Formatter kicks in during the
> Rename variable refactoring in DocumentChangeSetPrivate::generateNewText():
> 
>     if (formatter && (formatPolicy == DocumentChangeSet::AutoFormatChanges
>                               || formatPolicy ==
> DocumentChangeSet::AutoFormatChangesKeepIndentation)) {
> ... }
> 
> 
> GrepOutputModel sets formatPolicy to DocumentChangeSet::NoAutoFormat, so
> Find/Replace in Files does not cause such bugs. Neither does simple
> KTextEditor's Replace within a file.
> 
> It turned out that my Source Formatter for the C++ Language was set to the
> default Artistic Style->1TBS. After switching to Artistic Style->Qt, at
> least the easily reproducible space insertion issue is gone. Not sure about
> the spurious braces yet.

Could you tell me where you found another code formaters for KDevelop? I have only these provided in kdevelop package?
And because of this I used only such.

Second thing here.
Applying of Formatter is another issue in KDevelop, which nobody fixed since several years. There couple bugs which I reported about this. Check following: #358798, #358801, also 4 years ago. Shortly. Applying Formatter doesn't effect, at least in configuration window. Whatever I  select in configuration, when I back then always is the same, so: "C" and "Gnu Indent: Kernighan & Ritchie".


> Piotr, have you tried reproducing this bug and Bug 317299 with different C++
> Source Formatters configured in KDevelop settings?

To be honest, I don't remember if I changed Formatter and made tests more that one test couple or only I experiences issue with one.
I started test with my "second example", reported at 2016-07-11 23:55:16 CEST.
I selected Formatter: "C++", "Gnu Indent: Kernighan & Ritchie".
After using "Rename" option with parameter name I didn't get broken code by curly brackets. After this I user "Update declaration" and here I got unwanted space. 

In definition of used Formatter I can see declaration of method looking like this:
"void bar(int foo) {"

My original code looked like this:
"void bar( QAction *pAction );"

after I used "Update declaration" I got:
"void bar (QAction * pAction2);"

As you can see there is unwanted space.
I made another test renaming pAction2 with pAction2 in the same method.
In result. Code has not been broken by curly brackets and after I use "Update declaration" I got:
"void bar(QAction * pAction2);"

As you can see there is unwanted space disappeared.
My conclusion is that Formatter doesn't work correctly
Second thing is that, when I opened configuration I found "C", "Gnu Indent: Kernighan & Ritchie".

I tried reproduce issue (with broken code by curly brackets) with first example and also nothing wrong happened.

To be honest just coding since several months I just avoid this function to don't have broken code.
I will test more "Update declaration" and "Rename", maybe part of the fault lies with Source Formater as you said.
Comment 9 Igor Kushnir 2020-03-21 12:18:13 UTC
(In reply to Piotr Mierzwinski from comment #8)
> Could you tell me where you found another code formaters for KDevelop? I
> have only these provided in kdevelop package?
There are 12 Style options if you select "Artistic Style" in the Formatter combobox.

> There couple bugs which I reported about this. Check
> following: #358798, #358801, also 4 years ago. Shortly. Applying Formatter
> doesn't effect, at least in configuration window. Whatever I  select in
> configuration, when I back then always is the same, so: "C" and "Gnu Indent:
> Kernighan & Ritchie".
I just replied under Bug 358798 and Bug 317299.

> I will test more "Update declaration" and "Rename", maybe part of the fault
> lies with Source Formater as you said.
No need to test this for now. I've tested and debugged Artistic Style formatters myself and identified a few bugs that I'm going to fix. One is *this* very bug, which turned out to only be consistently reproducible with the 1TBS Artistic Style. Another bug that I'm going to fix made me think that *this* bug does not depend on the selected source formatter: selecting the 1TBS Artistic Style irreversibly sets the setAddBracketsMode Artistic Style option to true, so *this* bug becomes reproducible with any Artistic Style until KDevelop is restarted. I can't reproduce *this* bug with any of the Custom Script Formatters.
Comment 10 Piotr Mierzwinski 2020-03-24 21:49:26 UTC
(In reply to Igor Kushnir from comment #9)
> (In reply to Piotr Mierzwinski from comment #8)
> > Could you tell me where you found another code formaters for KDevelop? I
> > have only these provided in kdevelop package?
> There are 12 Style options if you select "Artistic Style" in the Formatter
> combobox.
> 
> > There couple bugs which I reported about this. Check
> > following: #358798, #358801, also 4 years ago. Shortly. Applying Formatter
> > doesn't effect, at least in configuration window. Whatever I  select in
> > configuration, when I back then always is the same, so: "C" and "Gnu Indent:
> > Kernighan & Ritchie".
> I just replied under Bug 358798 and Bug 317299.
> 
> > I will test more "Update declaration" and "Rename", maybe part of the fault
> > lies with Source Formater as you said.
> No need to test this for now. I've tested and debugged Artistic Style
> formatters myself and identified a few bugs that I'm going to fix. One is
> *this* very bug, which turned out to only be consistently reproducible with
> the 1TBS Artistic Style. Another bug that I'm going to fix made me think
> that *this* bug does not depend on the selected source formatter: selecting
> the 1TBS Artistic Style irreversibly sets the setAddBracketsMode Artistic
> Style option to true, so *this* bug becomes reproducible with any Artistic
> Style until KDevelop is restarted. I can't reproduce *this* bug with any of
> the Custom Script Formatters.

I look forward to your fix. I'm grateful that you interested this and are going to fix it. Broken code was really annoying thing what I met after refactoring.
Comment 11 Igor Kushnir 2020-03-25 09:02:27 UTC
(In reply to Piotr Mierzwinski from comment #10)
> I look forward to your fix. I'm grateful that you interested this and are
> going to fix it. Broken code was really annoying thing what I met after
> refactoring.

If you are curious, take a look at my merge request https://invent.kde.org/kde/kdevelop/-/merge_requests/118 that fixes this bug but fails to generally fix the broader issue. I hope someone suggests an idea how to fix the entire bug, because I don't see how that can be done.

There is also my branch that fixes the other bug I described (setAddBracketsMode in 1TBS). It is referenced as a dependent branch in https://invent.kde.org/kde/kdevelop/-/merge_requests/116.
Comment 12 Igor Kushnir 2023-06-22 11:08:49 UTC
I have implemented a much more reliable (and unfortunately much more verbose) version of extractFormattedTextFromContext(). Currently adding some tests to increase test coverage. Will update my old merge request soon.
Comment 13 Piotr Mierzwinski 2023-08-02 15:09:57 UTC
(In reply to Igor Kushnir from comment #12)
> I have implemented a much more reliable (and unfortunately much more
> verbose) version of extractFormattedTextFromContext(). Currently adding some
> tests to increase test coverage. Will update my old merge request soon.

Thanks a lot.
Comment 14 Igor Kushnir 2023-09-15 08:54:44 UTC
Git commit 0a0a309f3f1f0b9d17dada2aa9e9b1446990ff9d by Igor Kushnir.
Committed on 15/09/2023 at 10:25.
Pushed by igorkushnir into branch 'master'.

formattinghelpers: implement a more advanced algorithm

1TBS Artistic Style enables the add-braces option, which adds braces to
unbraced one-line conditional statements. When a user renames a
variable, each file that references this variable is reformatted by the
current formatter. KDevelop::extractFormattedTextFromContext() then
attempts to extract only the variable and the whitespace that surround
it from the formatted file. But this function's implementation does not
expect or handle an opening or closing brace inserted just before the
renamed variable (Bug 365437).

I have implemented an alternative workaround 3 years ago, which looked
for a non-whitespace character inserted at the left-context-text
boundary, and, if found, gave up formatting and returned unformatted
text from extractFormattedTextFromContext(). However, such a limited
workaround cannot address the much more difficult right context issue
described and tested in the parent commit.

The existing algorithm simply ignores fuzzy character changes hoping
that the result would come up correct. Usually it does. But in some
cases encountered by end users in practice, the unsophisticated
algorithm breaks code by inserting or removing an unmatched brace. As
formatters grow more sophisticated and insert/remove more non-whitespace
characters, the algorithm would cause issues more often.

The new algorithm tracks opening/closing brackets, double quotes and
comments to prevent breaking code. A long comment in the definition of
extractFormattedTextFromContext() describes the algorithm in more
detail. While not 100% reliable, I expect the new algorithm to make
practical formatting issues much more rare. As new bugs are discovered,
the more advanced algorithm can be adapted, as subsequent commits show.

At this commit the test added in the parent commit no longer fails.
FIXED-IN: 5.13.231200

M  +811  -49   kdevplatform/util/formattinghelpers.cpp
M  +6    -7    kdevplatform/util/formattinghelpers.h
M  +85   -0    kdevplatform/util/tests/test_formattinghelpers.cpp

https://invent.kde.org/kdevelop/kdevelop/-/commit/0a0a309f3f1f0b9d17dada2aa9e9b1446990ff9d
Comment 15 Igor Kushnir 2023-09-15 09:17:32 UTC
Git commit 82f2316385c617c2fe6c66c8fbd42c22162fa83d by Igor Kushnir.
Committed on 15/09/2023 at 11:02.
Pushed by igorkushnir into branch 'master'.

astyle: allow configuring add-braces option

I had to implement this new feature in order to prevent 1TBS Artistic
Style from affecting styles selected afterwards until KDevelop restart.

When 1TBS Artistic Style is selected, setAddBracketsMode(true) is
called. But this option is not set for any other style, is not (re)set
in AStyleFormatter::updateFormatter() and AStyleFormatter::resetStyle().
So after switching the current style from 1TBS to some other Artistic
Style, the user has to restart KDevelop in order to reset the
AddBracketsMode option to false.

Call setAddBracesMode() instead of setAddBracketsMode().
setAddBracketsMode() was deprecated in astyle 3.0 in favor of
setAddBracesMode(). KDevelop requires astyle 3.1.

Use "braces" rather than "brackets" in UI, option key, variable and
function names to follow the upstream astyle renaming decision.

M  +18   -2    plugins/astyle/astyle_formatter.cpp
M  +2    -0    plugins/astyle/astyle_formatter.h
M  +3    -0    plugins/astyle/astyle_preferences.cpp
M  +11   -1    plugins/astyle/astyle_preferences.ui

https://invent.kde.org/kdevelop/kdevelop/-/commit/82f2316385c617c2fe6c66c8fbd42c22162fa83d