Bug 369546

Summary: code browser popup shows wrong function declaration
Product: [Developer tools] kdevplatform Reporter: RJVB <rjvbertin>
Component: contextbrowserAssignee: kdevelop-bugs-null
Severity: normal CC: kossebau, ped
Priority: NOR Keywords: junior-jobs
Version: git master   
Target Milestone: ---   
Platform: Compiled Sources   
OS: All   
Latest Commit: Version Fixed In:
Attachments: screenshot showing the code browser glitch. NB: INFINITE is defined to be -1
Wrong default arguments in code browser help

Description RJVB 2016-09-29 23:13:08 UTC
I've noticed a case where the code browser popup that appears when hovering the mouse curser over a function shows an incorrect function declaration, systematically and both Linux and OS X; see the screenshot.

Reproducible: Always

Steps to Reproduce:
1. Open a file containing an affected function declaration or definition
2. Hover the mouse cursor over the function name

Actual Results:  
A window pops up with a different function declaration

Expected Results:  
The popup window should show the same declaration as the source code
Comment 1 RJVB 2016-09-29 23:13:59 UTC
Created attachment 101352 [details]
screenshot showing the code browser glitch. NB: INFINITE is defined to be -1
Comment 2 Peter Ped Helcmanovsky 2019-04-26 10:23:57 UTC
Created attachment 119648 [details]
Wrong default arguments in code browser help

I have very likely the same issue, attaching screenshot as well, the on-hover help window and code browser shows wrong default argument values.

Actually I was wondering where it even picks such values, then I got some suspicion, and modified the original:
`void BinIncFile(char* fname, int offset = 0, int length = INT_MAX);` (being parsed by KDevelop as `void BinIncFile(char* fname, int offset, int length = 0);` )

to `void BinIncFile(char* fname, int offset = 12, int length = INT_MAX);` and it got immediately updated in code browser to `void BinIncFile(char* fname, int offset, int length = 12);`

I.e. it looks to me the Code browser has some +-1 index issue to assign default value to correct argument. Maybe it's just trivial "off by 1" typo? (at first I was suspecting cache holding old data, but clearing cache didn't help, and after this 0 -> 12 test it's obvious caching and parsing itself does refresh data well, but either stores wrong data, or shows them in wrong way).
Comment 3 RJVB 2019-04-26 12:47:24 UTC
Seems you're on to something. Have you tried replacing the INT_MAX token by a numeric value to see if the symbolic default argument has something  to do with the issue? What happens when you assign a default argument to the fname argument; does that one shift too (even when it's NULL or nullptr instead of 0)?
Comment 4 Peter Ped Helcmanovsky 2019-04-27 23:05:57 UTC
(In reply to RJVB from comment #3)
> Seems you're on to something. Have you tried replacing the INT_MAX token by
> a numeric value to see if the symbolic default argument has something  to do
> with the issue?

`void BinIncFile(char* fname, int offset = INT_MAX, int length = 12);` -> shows -> `void BinIncFile(char* fname, int offset, int length = 12)`

`void BinIncFile(char* fname, int offset = INT_MAX, int length = INT_MAX);` -> shows -> `void BinIncFile(char* fname, int offset, int length);`

`void BinIncFile(char* fname = NULL, int offset = 1, int length = 0);` -> `void BinIncFile(char* fname, int offset = 1, int length = 0);`

`void BinIncFile(char* fname = nullptr, int offset = 0, int length = INT_MAX);` -> `void BinIncFile(char* fname, int offset = nullptr, int length = 0);`

So it seems it's not trivial +1 index, but the default values are assigned from back to front (not hard link to particular argument), and if some "didn't parse", it's missing in the assignment, creating effect of +1,2,... index bug depending how many are missing. `nullptr` is understood and shown.
Comment 5 Kevin Funk 2020-01-17 14:29:28 UTC
Note: Relevant code is in plugins/clang/util/clangutils.cpp: See the ClangUtils::getDefaultArguments() function.
Comment 6 Friedrich W. H. Kossebau 2020-01-18 08:52:59 UTC
To fix at least the wrong maoping of default values to args there is now
https://invent.kde.org/kde/kdevelop/merge_requests/88 up for review.

Still needs a full fix to support default values based on preprocessor macros, but what I found so far (like INT_MAX or QStringLiteral).
Comment 7 RJVB 2020-01-19 09:46:28 UTC
How the !@#$ do I get a patch file from that marvelous new review system (without checking out anything I do not need)?!
Comment 8 Kevin Funk 2020-01-20 08:41:29 UTC
What about Download (icon next to "Check out Branch") -> Plain Diff?

Or https://invent.kde.org/kde/kdevelop/merge_requests/88.diff
Comment 9 RJVB 2020-01-20 09:10:49 UTC
Usually I find this sort of thing, possibly by clicking on anything that seems clickable, but this one I missed. Maybe because I went for the diff display directly and -against what one could expect- there's no download option there.

I did try a few direct download URLs but all under .../88/ .
Comment 10 RJVB 2020-01-20 10:07:26 UTC
(In reply to Friedrich W. H. Kossebau from comment #6)
> Still needs a full fix to support default values based on preprocessor
> macros, but what I found so far (like INT_MAX or QStringLiteral).

Yep, in my example above it now shows `dwMilliSeconds = <KDEV TODO>`. I don't know how complicated it would be to show the actual macro but as far as I'm concerned this could already go in "as is". I'd just replace "KDEV TODO" with something (translatable) like

<CPP Macro (X)>

where X is the actual value. Or just show the actual value. There's use for being able to see what value or substitution code is being inserted without having to hunt down that value or code through possibly multiple headerfiles.

OTOH, that expansion could be limited to hovering over the actual definition instead of getting it also when you hover over each and every invocation (if you see what I mean...)
Comment 11 Peter Ped Helcmanovsky 2020-01-20 11:47:56 UTC
I believe there are use cases where both original macro name and/or evaluated value at compile time would be helpful, but the actual value is always build-configuration dependent, so I guess showing the original string directly from source is both simpler, and will work best.

I.e. in your original example or mine I would expect `dwMilliSeconds = INFINITE` and `length=INT_MAX` ... it is either already enough info for programmer, or in case he's curious, he can hunt down the actual definition of macro (for current build config) and see how it expands. But these macros are usually in place to prevent magic numbers in source, and preventing magic numbers in hover info then makes sense to me. (for example `swMilliSeconds = -1.123` seems counter productive, not conveying the original intention of the default value).
Comment 12 Friedrich W. H. Kossebau 2020-01-20 12:07:20 UTC
General remark: please do not share cursing & sarcasm with others here, it does not help the case & the quality of the discussion. I guess not only me prefers a place to spend free time in where positive energy rules. Thus I also had not replied before despite knowing an answer to the question how to get the diff, the note went straight to /dev/null.

To the topic:
The challenge (for me) is to get the actual strings used for the default values. The current code calling into clang does not deliver those in these cases. So no  "INT_MAX" etc. string is available right now. Otherwise my linked patch happily would just use those instead of that placeholder, as I agree those are pretty fine, I would expect them to be shown instead of being resolved.

To fix this, this needs someone experienced with the clang API.

That's why my linked patch is labelled "partial workaround". It just fixes a symptom of the problem.

It is also unclear to me whether this is only broken with macros, could be also other things. All I know is that the used "clang_getCursorExtent" reports a string which does not include the substring noting the default value.
That's why I propose a generic term as placeholder for now.
Comment 13 RJVB 2020-01-20 13:11:13 UTC
I agree it makes sense not to expand in popups over invocations of a function, but only when you trigger the popup over the function declaration (or definition) itself. That is, when it is possible to show the actual macro.

Until that time a placeholder is fine, but why not just admit it stands for an <Unknown Macro>? And as long as a placeholder is used it could be helpful to show the value, if it's a number. I forgot to add this temporary aspect to my previous remark about this. The popup is addressed at devs and I think most of us will understand what 0, -1, 65535, 3.1415 or 2.718 (etc) stand for. 

As to showing popups with the expanded code: maybe this actually calls for another kind of popup that will show what an expression containing macro(s) expands to. As said, it can be cumbersome to hunt down the exact expansion, esp. with nested macros or those that depend on conditional tokens that get set automatically. Being able to expand a line of block of code in a popup (with auto-formatting) sure beats having to generate the preprocessor output by hand and then find the bit you're interested in.
Comment 14 Milian Wolff 2020-01-20 14:06:37 UTC
Git commit 54585bcd0de5ca4ce0618accfa39967f6c7d22f0 by Milian Wolff.
Committed on 20/01/2020 at 14:06.
Pushed by mwolff into branch 'master'.

Fix ClangUtils::getDefaultArguments when encountering macros

Use ClangUtils::getRawContents instead of manual tokenization. This
is apparently more reliable and gives us the user-written text which
is imo the best we can hope for. I.e. now the user will see e.g.

    foo(int a = INT_MAX)

Which is way more expressive then us displaying the macro value.
We may want to replace more usages of clang_getTokenSpelling with

M  +9    -0    plugins/clang/tests/files/defaultparameters.cpp
M  +1    -5    plugins/clang/util/clangutils.cpp

Comment 15 Milian Wolff 2020-01-20 14:57:41 UTC
Git commit e3883a89853ffe6456e2fb595b69bd50668f6e85 by Milian Wolff.
Committed on 20/01/2020 at 14:55.
Pushed by mwolff into branch 'master'.

Don't get confused when encountering parse errors in default args

When we fail to get a useful string for a default arg, show at least
an explanatory "<parse error>" string in its place.

Sadly, it's not trivial to get the raw string in this case. Doable
maybe, but requires more manual work since the cursor extent won't
include e.g. a mistyped macro name or such.

M  +9    -0    plugins/clang/tests/files/invalid.cpp
M  +8    -0    plugins/clang/util/clangutils.cpp