[kdevelop] [Bug 365437] Rename local variable using "Rename" assistant breaks code

[Igor Kushnir]

https://bugs.kde.org/show_bug.cgi?id=365437

--- Comment #15 from Igor Kushnir  ---
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   -2plugins/astyle/astyle_formatter.cpp
M  +2-0plugins/astyle/astyle_formatter.h
M  +3-0plugins/astyle/astyle_preferences.cpp
M  +11   -1plugins/astyle/astyle_preferences.ui

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

-- 
You are receiving this mail because:
You are watching all bug changes.

[kdevelop] [Bug 365437] Rename local variable using "Rename" assistant breaks code

[Igor Kushnir]

https://bugs.kde.org/show_bug.cgi?id=365437

Igor Kushnir  changed:

   What|Removed |Added

 Status|ASSIGNED|RESOLVED
 Resolution|--- |FIXED
   Version Fixed In||5.13.231200
  Latest Commit||https://invent.kde.org/kdev
   ||elop/kdevelop/-/commit/0a0a
   ||309f3f1f0b9d17dada2aa9e9b14
   ||46990ff9d

--- Comment #14 from Igor Kushnir  ---
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-7kdevplatform/util/formattinghelpers.h
M  +85   -0kdevplatform/util/tests/test_formattinghelpers.cpp

https://invent.kde.org/kdevelop/kdevelop/-/commit/0a0a309f3f1f0b9d17dada2aa9e9b1446990ff9d

-- 
You are receiving this mail because:
You are watching all bug changes.

[kdevelop] [Bug 365437] Rename local variable using "Rename" assistant breaks code

[Piotr Mierzwinski]

https://bugs.kde.org/show_bug.cgi?id=365437

--- Comment #13 from Piotr Mierzwinski  ---
(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.

-- 
You are receiving this mail because:
You are watching all bug changes.

[kdevelop] [Bug 365437] Rename local variable using "Rename" assistant breaks code

[Igor Kushnir]

https://bugs.kde.org/show_bug.cgi?id=365437

Igor Kushnir  changed:

   What|Removed |Added

   Assignee|kdevelop-bugs-n...@kde.org  |igor...@gmail.com

-- 
You are receiving this mail because:
You are watching all bug changes.

[kdevelop] [Bug 365437] Rename local variable using "Rename" assistant breaks code

[Igor Kushnir]

https://bugs.kde.org/show_bug.cgi?id=365437

Igor Kushnir  changed:

   What|Removed |Added

 Status|CONFIRMED   |ASSIGNED

--- Comment #12 from Igor Kushnir  ---
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.

-- 
You are receiving this mail because:
You are watching all bug changes.

[kdevelop] [Bug 365437] Rename local variable using "Rename" assistant breaks code

[Igor Kushnir]

https://bugs.kde.org/show_bug.cgi?id=365437

--- Comment #11 from Igor Kushnir  ---
(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.

-- 
You are receiving this mail because:
You are watching all bug changes.

[kdevelop] [Bug 365437] Rename local variable using "Rename" assistant breaks code

[Piotr Mierzwinski]

https://bugs.kde.org/show_bug.cgi?id=365437

--- Comment #10 from Piotr Mierzwinski  ---
(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.

-- 
You are receiving this mail because:
You are watching all bug changes.

[kdevelop] [Bug 365437] Rename local variable using "Rename" assistant breaks code

[Igor Kushnir]

https://bugs.kde.org/show_bug.cgi?id=365437

--- Comment #9 from Igor Kushnir  ---
(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.

-- 
You are receiving this mail because:
You are watching all bug changes.

[kdevelop] [Bug 365437] Rename local variable using "Rename" assistant breaks code

[Piotr Mierzwinski]

https://bugs.kde.org/show_bug.cgi?id=365437

--- Comment #8 from Piotr Mierzwinski  ---
(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.

-- 
You are receiving this mail because:
You are watching all bug changes.

[kdevelop] [Bug 365437] Rename local variable using "Rename" assistant breaks code

[Igor Kushnir]

https://bugs.kde.org/show_bug.cgi?id=365437

--- Comment #7 from Igor Kushnir  ---
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.

-- 
You are receiving this mail because:
You are watching all bug changes.

[kdevelop] [Bug 365437] Rename local variable using "Rename" assistant breaks code

[Igor Kushnir]

https://bugs.kde.org/show_bug.cgi?id=365437

Igor Kushnir  changed:

   What|Removed |Added

 CC||igor...@meta.ua

--- Comment #6 from Igor Kushnir  ---
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?

-- 
You are receiving this mail because:
You are watching all bug changes.

[kdevelop] [Bug 365437] Rename local variable using "Rename" assistant breaks code

[Piotr Mierzwinski]

https://bugs.kde.org/show_bug.cgi?id=365437

Piotr Mierzwinski  changed:

   What|Removed |Added

Version|5.1.1   |5.2.3

-- 
You are receiving this mail because:
You are watching all bug changes.

[kdevelop] [Bug 365437] Rename local variable using "Rename" assistant breaks code

[Piotr Mierzwinski]

https://bugs.kde.org/show_bug.cgi?id=365437

--- Comment #5 from Piotr Mierzwinski  ---
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 :(.

-- 
You are receiving this mail because:
You are watching all bug changes.

[kdevelop] [Bug 365437] Rename local variable using "Rename" assistant breaks code

[Piotr Mierzwinski]

https://bugs.kde.org/show_bug.cgi?id=365437

--- Comment #4 from Piotr Mierzwinski  ---
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.

-- 
You are receiving this mail because:
You are watching all bug changes.

[kdevelop] [Bug 365437] Rename local variable using "Rename" assistant breaks code

[Piotr Mierzwinski]

https://bugs.kde.org/show_bug.cgi?id=365437

--- Comment #3 from Piotr Mierzwinski  ---
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;
}

-- 
You are receiving this mail because:
You are watching all bug changes.

[kdevelop] [Bug 365437] Rename local variable using "Rename" assistant breaks code

[Piotr Mierzwinski]

https://bugs.kde.org/show_bug.cgi?id=365437

Piotr Mierzwinski  changed:

   What|Removed |Added

Version|5.0.0   |5.1.1

--- Comment #2 from Piotr Mierzwinski  ---
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.

-- 
You are receiving this mail because:
You are watching all bug changes.

[kdevelop] [Bug 365437] Rename local variable using "Rename" assistant breaks code

[Anton Anikin via KDE Bugzilla]

https://bugs.kde.org/show_bug.cgi?id=365437

Anton Anikin  changed:

   What|Removed |Added

 Status|UNCONFIRMED |CONFIRMED
 CC||anton.ani...@htower.ru
 Ever confirmed|0   |1

-- 
You are receiving this mail because:
You are watching all bug changes.

[kdevelop] [Bug 365437] Rename local variable using "Rename" assistant breaks code

[Piotr Mierzwinski via KDE Bugzilla]

https://bugs.kde.org/show_bug.cgi?id=365437

Piotr Mierzwinski  changed:

   What|Removed |Added

Version|git master  |5.0.0

-- 
You are receiving this mail because:
You are watching all bug changes.

[kdevelop] [Bug 365437] Rename local variable using "Rename" assistant breaks code

[Piotr Mierzwinski via KDE Bugzilla]

https://bugs.kde.org/show_bug.cgi?id=365437

Piotr Mierzwinski  changed:

   What|Removed |Added

 CC||piotr.mierzwin...@gmail.com

--- Comment #1 from Piotr Mierzwinski  ---
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();
}
}

-- 
You are receiving this mail because:
You are watching all bug changes.