Bug 255706 - KDevelop does not rename a variable in method declaration, definition, and function body
Summary: KDevelop does not rename a variable in method declaration, definition, and fu...
Status: CONFIRMED
Alias: None
Product: kdevelop
Classification: Applications
Component: Language Support: CPP (old) (show other bugs)
Version: 4.1.60
Platform: Arch Linux Linux
: NOR minor
Target Milestone: ---
Assignee: Olivier.jg
URL:
Keywords:
: 295865 (view as bug list)
Depends on:
Blocks:
 
Reported: 2010-10-31 00:41 UTC by Nathaniel
Modified: 2021-03-16 19:36 UTC (History)
3 users (show)

See Also:
Latest Commit:
Version Fixed In:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Nathaniel 2010-10-31 00:41:45 UTC
Version:           4.1.60 (using KDE 4.5.2) 
OS:                Linux

Say you make a method Bar(int baz) on object Foo. foo.h contains the prototype:
Bar(int baz);
And foo.cpp contains the definition:
Foo::Bar(int baz) {do_stuff(baz);}

Now, change the name of baz to Baz in the prototype, and KDevelop helpfully offers to change it in the definition as well. Tell it to do so, and it will change the definition to:
Foo::Bar(int Baz) {do_stuff(baz);}
It did not change the name in the body of the function, only the parameter list.

Now, instead of renaming it in the prototype, rename it in the definition. KDevelop again offers to rename it, so let it do so, and it renames it in the function body, but not in the prototype in the header.

Obviously, renamed variables should be synced across the parameter list in the prototype, definition and function body.

Reproducible: Always
Comment 1 Olivier.jg 2010-10-31 02:37:48 UTC
Yes, there are two different assistants at play here. One is the rename assistant, that will rename any declaration's uses. The other is the signature assistant, that will offer to update the other side of a function definition/declaration.

The reason that renaming in the definition doesn't rename in the declaration is because variable names in the declaration are unfortunately little better than comments, which could be nonexistant, or just completely different (they aren't strictly related).

Two things need to happen:
1. The signature assistant needs to be shown after the rename assistant (getting around to this)
2. The variables used in the function definition need to be renamed when you update it with the signature assistant.

#2 Is much trickier, since the signature assistant currently doesn't have a foolproof mapping of old param -> new param, just heuristics.

Fixing this properly means updating the assistant UI, and making the signature assistant much smarter.
Comment 2 Ciprian Ciubotariu 2010-10-31 14:13:24 UTC
From the C++ point of view there is no correspondence between the name of a 
parameter in the class method declaration and the name of the same parameter 
in the class method definition. The following code is valid C++:

class A { int foo(int bar); };

int A::foo(int another_bar) { return another_bar; }

Matching those names is only a matter of code style. Actually the name can be 
elided in the method declaration and the code can still remain valid:

class A { int foo(int); };

Also, names can be elided in the definition if not needed (sometimes I do this 
to avoid warnings about unused parameters).

class A { int foo(int a, int b); };

int A::foo(int a, int /* b */) { return a; }

From this perspective the rename assistant(s) should only offer the choice to 
update the corresponding declaration/definition of a method and not enforce it. 
Besides this wish, my post is about expecting the names to be missing 
altogether.

By the way, the same rules apply to non-member functions and their prototypes 
:)
Comment 3 Nathaniel 2010-10-31 14:46:17 UTC
I appreciate that the names in the prototype do not matter, but since KDevelop offers to sync them, it should do a good job of it. I marked this bug as minor because it is mostly an issue of cosmetics. However, if you change the name in the prototype and sync, it will break your code, as the function itself still uses the old name, but the parameter has been renamed. That is not good. :P
Comment 4 Olivier.jg 2012-03-17 01:05:29 UTC
*** Bug 295865 has been marked as a duplicate of this bug. ***
Comment 5 Olivier.jg 2012-03-17 01:34:59 UTC
Git commit 68735ddac47092c06af8a53cbd15ec25409450ef by Olivier JG.
Committed on 07/03/2012 at 09:27.
Pushed by olivierjg into branch 'master'.

New (sigassist): when updating definition to match decl, rename also.
Move rename and adaptsignature actions into .h and .cpp files
Cleanup and comments in the signature assistant.
REVIEW: 103817

M  +2    -0    languages/cpp/CMakeLists.txt
A  +180  -0    languages/cpp/codegen/adaptsignatureaction.cpp     [License: LGPL (v2)]
A  +68   -0    languages/cpp/codegen/adaptsignatureaction.h     [License: LGPL (v2)]
A  +66   -0    languages/cpp/codegen/renameaction.cpp     [License: LGPL (v2)]
A  +45   -0    languages/cpp/codegen/renameaction.h     [License: LGPL (v2)]
M  +1    -48   languages/cpp/codegen/renameassistant.cpp
M  +149  -329  languages/cpp/codegen/signatureassistant.cpp
M  +17   -20   languages/cpp/codegen/signatureassistant.h

http://commits.kde.org/kdevelop/68735ddac47092c06af8a53cbd15ec25409450ef
Comment 6 Olivier.jg 2012-03-17 01:40:49 UTC
As of that last patch, the situation is:
When you change variable names in the function declaration, and update the definition sig, it will also rename the uses in the function definition, which is most important.
When you change variable names in the function definition, it will give you the rename assistant, and not the signature assistant.
The plan is to support multiple active assistants (probably "stacked") to fix this, first showing the rename assistant and then showing the sigassist.

As a workaround for the second problem until I get around to it...
When renaming a variable in the definition, take advantage of the rename assistant when it comes, and then make and undo a non-rename change in the definition signature (ie, add a space and backspace), and it'll give you the signature assistant.
Comment 7 Piotr Mierzwinski 2012-03-23 23:27:30 UTC
I downloaded source of kdevelop-4.3.0 and kdevplatform-1.3, compiled and installed. I make a test and it still doesn't work. I think that your fix didn't entered to the stable version of kdevelop4. Why? I checked your commit: 68735dd (http://quickgit.kde.org/?p=kdevelop.git) in downloaded source of stable version and I not found changes. There are before 'Merge branch '4.3''. Why your changes not entered into the stable version?
Comment 8 Piotr Mierzwinski 2012-03-24 00:03:53 UTC
Update to the last comment.
It's partially works.
When I change parameter name in definition then I giving 'Rename assistant'. It works for function's definition, but I must manually change name into header file.
In the opposite direction. When I change parameter name in declaration it doesn't work. I can rename only parameters in declaration and in definition of function, but not uses in body of function.
By the way, name of parameter in doxygen's documentation of function (in header file) isn't updated (when renaming parameter in declaration).
Another problem there is with 'Rename declaration'.
When I call 'Rename declaration' (Alt+Ctrl+R) in header file for parameter of function then I can see on the list only one use of this parameter (from declaration), whereas in definition there are few uses. Why? I want to rename all instances.
When I'm doing the same action in cpp file then I can see uses of this parameter only in cpp  file. Why? I want to rename all instances.
Comment 9 Olivier.jg 2012-03-24 05:51:43 UTC
(In reply to comment #8)
> Update to the last comment.
> It's partially works.
> When I change parameter name in definition then I giving 'Rename assistant'.
> It works for function's definition, but I must manually change name into
> header file.
> In the opposite direction. When I change parameter name in declaration it
> doesn't work. I can rename only parameters in declaration and in definition
> of function, but not uses in body of function.

This is the original behavior, my patch is in master, didn't make it into 4.3

> By the way, name of parameter in doxygen's documentation of function (in
> header file) isn't updated (when renaming parameter in declaration).

There is another bug open for this. I think it's reasonable to implement, but it's a technically unrelated feature.

> Another problem there is with 'Rename declaration'.
> When I call 'Rename declaration' (Alt+Ctrl+R) in header file for parameter
> of function then I can see on the list only one use of this parameter (from
> declaration), whereas in definition there are few uses. Why? I want to
> rename all instances.
> When I'm doing the same action in cpp file then I can see uses of this
> parameter only in cpp  file. Why? I want to rename all instances.

This is technically correct behavior. The parameter name in the declaration (.h) is technically no better than a comment, it's not actually a use of the seperate parameter declaration on the other side and doesn't have to be the same (or even present). Of course, it's still a reasonable feature to add, feel free to add another bug report (wishlist) for it.
Comment 10 Piotr Mierzwinski 2012-03-24 10:53:11 UTC
> > Update to the last comment.
> > It's partially works.
> > When I change parameter name in definition then I giving 'Rename assistant'.
> > It works for function's definition, but I must manually change name into
> > header file.
> > In the opposite direction. When I change parameter name in declaration it
> > doesn't work. I can rename only parameters in declaration and in definition
> > of function, but not uses in body of function.

> This is the original behavior, my patch is in master, didn't make it into 4.3
I noticed that your patch is in git version. I thought that master == stable version or released version. I cloned kdevelop and kdevplatorm repository and I will test it. Is maintainer considers putting your patch in stable version?

> > By the way, name of parameter in doxygen's documentation of function (in
> > header file) isn't updated (when renaming parameter in declaration).

> There is another bug open for this. I think it's reasonable to implement, but it's a technically 
> unrelated feature.
I requested this bug: 296198. Maybe it is. Unfortunately has status 'Unconfirmed'.

> > Another problem there is with 'Rename declaration'.
> > When I call 'Rename declaration' (Alt+Ctrl+R) in header file for parameter
> > of function then I can see on the list only one use of this parameter (from
> > declaration), whereas in definition there are few uses. Why? I want to
> > rename all instances.
> > When I'm doing the same action in cpp file then I can see uses of this
> > parameter only in cpp  file. Why? I want to rename all instances.

> This is technically correct behavior. The parameter name in the declaration (.h) is technically
> no better than a comment, it's not actually a use of the seperate parameter declaration on the > other side and doesn't have to be the same (or even present). Of course, it's still a reasonable 
> feature to add, feel free to add another bug report (wishlist) for it.
Your technically right, but nice to have an order in parameter's names. And not in header one name and in cpp other name.
Yes, I know that I can always make a request. Maybe someone, someday fix it or not.
I requested few bugs two weeks a go, but they still have status 'Unconfirmed'.
Comment 11 Olivier.jg 2012-03-24 12:05:44 UTC
> I requested few bugs two weeks a go, but they still have status
> 'Unconfirmed'.

Kdevelop still has a relatively small team, bugs are generally noted, but no one has time to go through the hundreds of bugs to mark them as confirmed (or not). Generally speaking, unless it's closed after a short period of time, you can consider it a confirmed bug.
Comment 12 Justin Zobel 2021-03-09 22:48:37 UTC
Thank you for the bug report.

As this report hasn't seen any changes in 5 years or more, we ask if you can please confirm that the issue still persists.

If this bug is no longer persisting or relevant please change the status to resolved.
Comment 13 Piotr Mierzwinski 2021-03-10 19:19:19 UTC
In the newest stable version of KDevelop (5.6.2) this seems to be broken.
What happens in current version when we rename argument in the prototype from baz to baz1?

1. problem
- is adding prefix of class name to the method, so here it would be: 'Foo::' and method would looks like:
Foo::Bar(int baz1)
;
all of this in prototype (header) file.

2. problem
usage of argument is renamed in definition of method, so fine, but unfortunately doesn't in its declaration in method definition (header of definition). 

3. problem.
when we over renamed argument by mouse cursor in definition of method then all occurrences are highlighted, even this not renamed in header of definition.

Fortunately to restore old names we can use Ctrl+Z.
Comment 14 Nathaniel 2021-03-16 19:36:59 UTC
I can confirm Piotr's results. In version 5.6.2, the renaming behavior seems improved, but it's still not optimal. Working with the following code:

```c++
struct Foo {
    void bar(int baz);
};

void Foo::bar(int baz) {
    int quux = baz;
}
```

We have three different kinds of reference to `baz`: declaration, definition, and usage. Optimal behavior would be that invoking auto-rename on any of the three would rename the other two without any extra steps.

Observed behavior is that renaming the definition or usage immediately renames the other of those two, i.e. renaming definition renames usage and vice versa. In both cases, a short delay later there is an offer to update the declaration as well. This is sub-optimal, as there is a delay and extra step involved, but it is not a bug so much as a usability issue.

I suppose there may be a use case for having the declaration not match the definition/usage, but if they were the same before, they should definitely be the same after without further question.

Renaming the *declaration* leads to issues, though. If I rename the declaration to `bazz`, initially it simply renames the declaration and not the definition or usage, which leaves the code in a valid-but-eccentric state. A short delay later comes the offer to adapt the signature, and if accepted, the code comes out like so:

```c++
struct Foo {
    void Foo::bar(int bazz)
;
};

void Foo::bar(int baz) {
    int quux = bazz;
}
```

So the declaration is munged and no longer valid, and the usage is updated, but the definition is not, and the code is no longer valid. This is a bug.

It's interesting to note that the declaration now looks like a valid definition, which, for a method, is not a valid declaration, so the signature assistant may be mistaking the site of invocation as being the definition rather than the declaration?

In any event, the issue does persist.