Bug 468605 - auto-correction incorrectly changes dot to arrow in floating-point literals when using uniform initialization
Summary: auto-correction incorrectly changes dot to arrow in floating-point literals w...
Status: RESOLVED FIXED
Alias: None
Product: kdevelop
Classification: Applications
Component: Code completion (show other bugs)
Version: 5.10.221203
Platform: Arch Linux Linux
: NOR normal
Target Milestone: ---
Assignee: kdevelop-bugs-null
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2023-04-17 10:47 UTC by Alessandro Carinelli
Modified: 2023-09-15 12:13 UTC (History)
1 user (show)

See Also:
Latest Commit:
Version Fixed In: 5.13.231200


Attachments
Patch to print completion info and assist in investigation (1.70 KB, patch)
2023-04-26 05:16 UTC, Igor Kushnir
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Alessandro Carinelli 2023-04-17 10:47:07 UTC
When using uniform initialization (C++ 11 style) of an object (struct or class), auto-correction changes dot to arrow ("." to "->") when trying to type floating-point literal.

Code:
struct Test {
    int a;
    float b;
};

int main(int argc, char **argv) {
    Test foo{1, 2};
    return 0;
}

If I try to type a dot after the 2 as to assign a floating-point value to member b, such dot gets replaced by an arrow.

SOFTWARE/OS VERSIONS
OS: Arch Linux, kernel: 6.2.11-zen1-1-zen (64 bit)
KDE Plasma Version: 5.27.4
KDE Frameworks Version: 5.105.0
Qt Version: 5.15.9
Comment 1 Igor Kushnir 2023-04-17 16:24:42 UTC
Just reproduced this bug in latest KDevelop master. This bug is very similar to the recently fixed Bug 460870, but here the floating-point literal starts with a digit, not a dot.
Comment 2 Alessandro Carinelli 2023-04-25 12:34:48 UTC
The substitution in object happens at line 1000 in file plugins/clang/codecompletion/context.cpp, before that (line 984) the value of m_results->NumResults is checked to be different from 0. I noticed that in the case of this bug such value is always 1 while in other cases it is bigger.
Checking that m_results->Numresults is also different from 1 on line 984 seems to fix the issue but I have no idea about what this variable represents and I found no comments about it so I don't know if this breaks anything else.
Comment 3 Igor Kushnir 2023-04-25 12:55:30 UTC
(In reply to Alessandro Carinelli from comment #2)
> Checking that m_results->Numresults is also different from 1 on line 984
> seems to fix the issue but I have no idea about what this variable
> represents and I found no comments about it so I don't know if this breaks
> anything else.

Starting at the line 966 of the file plugins/clang/codecompletion/context.cpp, KDevelop takes a code fragment that leads up to and includes '.' (stored in the variable trimmedText); temporarily replaces this last dot with '->'; and then asks libclang to try and come up with possible completions after that arrow. If libclang finds a way to complete the arrow (NumResults [= the number of completion results] is positive), KDevelop assumes that the code with the arrow would compile given some more typing or completion after the arrow, decides that the dot should indeed be replaced with an arrow and does just that. So no, checking that m_results->Numresults is also different from 1 does not seem to be a good idea, at least not on the surface.
You could check what is that strange completion result that libclang finds after "2." and see if this completion is something KDevelop should ignore here. You can start with debug-printing (via e.g. qCritical()) str.c_str(). The string is clearly not equal to "Parse Issue", but maybe it is something else, which should also be ignored.
Comment 4 Igor Kushnir 2023-04-25 13:02:48 UTC
Also take a look at related functions in include/clang-c/CXDiagnostic.h (or include/clang-c/Index.h). For example, I think clang_getDiagnosticSpelling() can potentially clarify the meaning of the single completion result libclang comes up with for "2->".
Comment 5 Alessandro Carinelli 2023-04-25 20:46:27 UTC
Thanks for the help. Unfortunately both clang_getDiagnosticCategoryText and clang_getDiagnosticSpelling seem to return empty strings in this case (while in the case of a floating point literal with a leading '.' they are respectively "Parse Issue" and "expected expression").
I noticed that when reproducing the bug as soon as the '.' gets replaced with "->" an autocompletion suggestion pops up containing only the name of the struct and this makes sense with the single completion result provided by clang. 
Only after a while an error shows up saying "Expected unqualified-id". Is it possible that this lag is the reason why it looks like everything is fine with the arrow and the program proceeds with the substitution?

I had an idea: after we get trimmedText we move backwards starting from the character right before the '.' checking every character until we find something different from a letter (upper or lowercase), an underscore or a digit, in this way we should find the first character in the identifier or literal that precedes the '.', if such first character is a digit we know that we are dealing with a floating point literal and skip the substitution.
I tried it and it seems to work without noticeable performance issues.
Comment 6 Igor Kushnir 2023-04-26 05:16:15 UTC
Created attachment 158431 [details]
Patch to print completion info and assist in investigation
Comment 7 Igor Kushnir 2023-04-26 05:22:05 UTC
When I apply the attached patch, I get the following output while reproducing the bug:
RESULTS: 1 ; kind: OverloadCandidate
kind: 2 chunk text: "Test"
kind: 10 chunk text: "{"
kind: 3 chunk text: "int a"
kind: 14 chunk text: ", "
kind: 5 chunk text: "float b"
kind: 11 chunk text: "}"

I don't quite understand what's the meaning and purpose of this completion. Someone will have to investigate this further and maybe experiment/test whether this completion cursor kind is ever useful. If not, KDevelop could also return early from ClangCodeCompletionContext() when there is a single completion result after the tentative arrow and its cursor kind is OverloadCandidate.
Comment 8 Igor Kushnir 2023-04-26 05:26:33 UTC
(In reply to Alessandro Carinelli from comment #5)
> I had an idea: after we get trimmedText we move backwards starting from the
> character right before the '.' checking every character until we find
> something different from a letter (upper or lowercase), an underscore or a
> digit, in this way we should find the first character in the identifier or
> literal that precedes the '.', if such first character is a digit we know
> that we are dealing with a floating point literal and skip the substitution.
> I tried it and it seems to work without noticeable performance issues.
This proposed workaround is too narrow for my liking. What if the user types a name of a constant of a built-in type instead of an integer literal? But I guess if there is no more general fix that is acceptable, this workaround might do.
Comment 9 Bug Janitor Service 2023-07-28 16:41:58 UTC
A possibly relevant merge request was started @ https://invent.kde.org/kdevelop/kdevelop/-/merge_requests/474
Comment 10 Igor Kushnir 2023-09-15 12:13:31 UTC
Git commit 7cd74b5863e5367910a32ffeafed6e35bb879b94 by Igor Kushnir.
Committed on 15/09/2023 at 13:49.
Pushed by igorkushnir into branch 'master'.

clang: ignore overload candidate completions after ->

CXCursor_OverloadCandidate apparently signifies candidates in an
overload set. Such completions are not useful after a pointer to an
object and an arrow. libclang probably offers them only by mistake.
FIXED-IN: 5.13.231200

M  +15   -2    plugins/clang/codecompletion/context.cpp
M  +13   -0    plugins/clang/tests/test_codecompletion.cpp

https://invent.kde.org/kdevelop/kdevelop/-/commit/7cd74b5863e5367910a32ffeafed6e35bb879b94