Pull request 1083 in include-what-you-use: Avoid autocast assurance for function definition

[notifi...@include-what-you-use.org]

New pull request 1083 by bolshakov-a: Avoid autocast assurance for function definition
https://github.com/include-what-you-use/include-what-you-use/pull/1083

A function may just transmit passed-by-reference parameter to somewhere.
Requirement to explicitly write forward declaration in the same file (.cpp-file) to avoid `#include` suggestion is impractical when that type is already fwd-declared in the corresponding header.

I'm not sure about this change. Autocast to parameter of defined function still makes sense when the function definition is in header:
```cpp
inline void ProcessString(const StringWrapper& sw)
{
stringProcessor.process(sw); // passed by reference
}
```
Maybe, to check whether the function is inline? Or there may be some better heuristics?

[notifi...@include-what-you-use.org]

Comment #0 on pull request 1083 by kimgr: Avoid autocast assurance for function definition
https://github.com/include-what-you-use/include-what-you-use/pull/1083

Hmm, yeah. This is confusing.

I don't like the short-circuiting of analysis in `VisitFunctionDecl` (though I happen to have some unfinished major rewrites of `FunctionDecl` handling somewhere that could result in something similar).

Did the last autocast change trigger warnings on simple forwarding functions? In that case, I wonder if we should maybe revert it and look for some other solution.

[notifi...@include-what-you-use.org]

Comment #2 on pull request 1083 by bolshakov-a: Avoid autocast assurance for function definition
https://github.com/include-what-you-use/include-what-you-use/pull/1083

> Did the last autocast change trigger warnings on simple forwarding functions?

No, they are unrelated changes. This one is about function declaration site handling, and that one — about call site. And changed tests are different, as you may see.

[notifi...@include-what-you-use.org]

Comment #2 on pull request 1083 by bolshakov-a: Avoid autocast assurance for function definition
https://github.com/include-what-you-use/include-what-you-use/pull/1083

> Did the last autocast change trigger warnings on simple forwarding functions?

No, they are unrelated changes. This one is about function definition site handling, and that one — about call site. And changed tests are different, as you may see.

[notifi...@include-what-you-use.org]

Comment #3 on pull request 1083 by bolshakov-a: Avoid autocast assurance for function definition
https://github.com/include-what-you-use/include-what-you-use/pull/1083

>And changed tests are different, as you may see.

The previous version of the test case in `badinc` just test that autocast possibility triggers `#include`-suggestion _in function definition_ on parameter _passed by reference_. I don't like it because I often forget to mark my one-arg constructors as `explicit` (which is surely bad) and I often write parameter-transmitting factory methods. But I can wait if you intend to refactor here.

[notifi...@include-what-you-use.org]

Comment #4 on pull request 1083 by kimgr: Avoid autocast assurance for function definition
https://github.com/include-what-you-use/include-what-you-use/pull/1083

Thanks, that makes sense!

Hmm. So I guess those two work in tandem --

The callsite check tries to determine if the caller or the callee is responsible for auto-cast types. But it can't suggest any edits for the callee, because that's probably in an unrelated header associated with another translation unit.

The declaration-side check, then, needs to pick up the slack and guess that a parameter might be subject to auto-cast from some callsite, and take ownership.

I guess the latter is bound to have insufficient information, but I wonder if we could use some other heuristic to help it make better decisions. Off the top of my head, this patch is too heavy-handed.

> But I can wait if you intend to refactor here.

I don't think my planned changes will have immediate effects, but they might make it possible to use more information in the decision-making process. I'll see if I can breathe life into that again.

[notifi...@include-what-you-use.org]

Comment #5 on pull request 1083 by bolshakov-a: Avoid autocast assurance for function definition
https://github.com/include-what-you-use/include-what-you-use/pull/1083

> I guess the latter is bound to have insufficient information

I think that in the case when a function is declared in header and defined in .cpp (as it usually is), autocast analysis should be performed only on header (it is meaningless for .cpp in that case). When function is local for .cpp-file, autocast possibility assurance seems also redundant (at least, for me).
The only doubtful case is a function _defined_ in header. Such functions should usually be marked as `inline` (not doing so is almost always an error; methods defined inside a class declaration are also "inline" in fact). So, I think that `inline` checking should work (hoping that clang provides such possibility). Or, maybe, to check whether the file is .h or .cpp directly?

[notifi...@include-what-you-use.org]

Comment #6 on pull request 1083 by bolshakov-a: Avoid autocast assurance for function definition
https://github.com/include-what-you-use/include-what-you-use/pull/1083

Ah, it seems like I realize now, that if declaration-site analysis is changed, call-site analysis should be changed as well...

[notifi...@include-what-you-use.org]

Comment #7 on pull request 1083 by bolshakov-a: Avoid autocast assurance for function definition
https://github.com/include-what-you-use/include-what-you-use/pull/1083

But it doesn't seem a big problem to check visibility of the definition from the call site. Or, maybe, just test the first visible declaration whether it also is the definition.
This PR should probably be reworked, indeed, but I'm trying to find the right direction...

[notifi...@include-what-you-use.org]

Comment #8 on pull request 1083 by kimgr: Avoid autocast assurance for function definition
https://github.com/include-what-you-use/include-what-you-use/pull/1083

> Ah, it seems like I realize now, that if declaration-site analysis is changed, call-site analysis should be changed as well...

Can you explain this a bit more? Call-site analysis seems simpler, unless, I guess, the call-site is in the same file as the definition.

I think I understand what you want to achieve, but I don't have a clear idea how to do that. The challenge is to guess whether the declaration (possibly definition) we're processing should really obey the autocast rules, or if there's another declaration visible that already does, right?

I wonder if it would be possible to enumerate the redeclarations, and see if there's any one of them that has an author-intent forward decl hint...?

[notifi...@include-what-you-use.org]

Comment #9 on pull request 1083 by bolshakov-a: Avoid autocast assurance for function definition
https://github.com/include-what-you-use/include-what-you-use/pull/1083

> Can you explain this a bit more?

The problem is that for such code:
```cpp
#include "S.h" // S has implicit ctor accepting int

static void fn(const S&) {
}

int main() {
fn(5);
}
```
IWYU from this branch suggests to remove `#include "S.h"`. This is an error not present on master branch, so this PR needs work.

> The challenge is to guess whether the declaration (possibly definition) we're processing should really obey the autocast rules

I think that if a declaration isn't a definition, it should provide autocast possibility, no problem with that. Of course, there are cases like
```cpp
void fn1(const S&); // prototype

void fn2(const S& s) {
fn1(s);
}

void fn1(const S& s) { // definition
...
}
```
when one probably don't want to `#include "S.h"`, but these cases seem to be rare.

[notifi...@include-what-you-use.org]

Comment #10 on pull request 1083 by kimgr: Avoid autocast assurance for function definition
https://github.com/include-what-you-use/include-what-you-use/pull/1083

Thanks for the added detail, those examples are very helpful.

My question was more in regards to your mention of 'call-site analysis should be changed as well' -- I don't really see that; this looks to me like a decl/defn-side problem entirely.

I actually think searching the redecls might be a nice solution. Something like:
```
if current node is a function decl and has a parameter with implicit conversion ctor:
for redecl in fn_decl.redecls():
if code author wants just a forward declare(redecl):
# yipee!
```

Oh, hm. `GetCallerResponsibleTypesForAutocast` already does almost that: https://github.com/include-what-you-use/include-what-you-use/blob/master/iwyu.cc#L1470. But it requires that _all_ declarations have the author-intent signal, rather than just one of them.

So let's say from a call-expr perspective, it needs to be all, but from a decl/defn perspective it could be any single one. I think that would make sense.

[notifi...@include-what-you-use.org]

Comment #11 on pull request 1083 by bolshakov-a: Avoid autocast assurance for function definition
https://github.com/include-what-you-use/include-what-you-use/pull/1083

>My question was more in regards to your mention of 'call-site analysis should be changed as well' -- I don't really see that; this looks to me like a decl/defn-side problem entirely.

But if you change the one, you must change the other, in order to keep decl/defn-handling logic and call site handling logic complementary. Otherwise you will get a bug like aforementioned bug in this branch. In fact, one can say that there isn't any problem at all on master branch. Just one more reason to mark constructors as `explicit`. This PR suggests just slight analysis improvement.

> But it requires that all declarations have the author-intent signal, rather than just one of them.

It seems reasonable, because if any of included headers with function declaration includes the header which contains parameter type, there is no need to include it in the calling file.
I just concern of local for .cpp-file functions, which usually have only one declaration coinciding with definition. I don't intend to change behavior for other cases.

[notifi...@include-what-you-use.org]

Comment #12 on pull request 1083 by bolshakov-a: Avoid autocast assurance for function definition
https://github.com/include-what-you-use/include-what-you-use/pull/1083

> I just concern of local for .cpp-file functions, which usually have only one declaration coinciding with definition

Sorry, I was wrong. When a function is declared in header and defined in .cpp, IWYU suggests `#include` for parameter autocast in the .cpp-file-definition too. Forward declaration in the corresponding header doesn't help. I already wrote about it, but forgot.
My idea is that function declaration handling shouldn't be changed, only definition handling.
All of that doesn't seem to be a big problem, but it's better to fix it somehow in the future.

[notifi...@include-what-you-use.org]

Comment #13 on pull request 1083 by bolshakov-a: Avoid autocast assurance for function definition
https://github.com/include-what-you-use/include-what-you-use/pull/1083

> `GetCallerResponsibleTypesForAutocast` already does almost that

Btw, [I've removed that](https://github.com/bolshakov-a/include-what-you-use/commit/ca09b8b50ba44c1ae2e8f14090e06ca480599c91). I inverted logic of the function and decided to remove that piece of code rather than support it. No one test failed.

[notifi...@include-what-you-use.org]

Comment #14 on pull request 1083 by kimgr: Avoid autocast assurance for function definition
https://github.com/include-what-you-use/include-what-you-use/pull/1083

Ah, yes. The `GetCallerResponsibleTypesForAutocast` function is almost a double-negative, so I always get confused about what it's trying to decide. Your naming makes more sense there, I think :-)

For some reason I feel uneasy short-circuiting `VisitFunctionDecl` for definitions, but I can't really see any obvious reason why it would be wrong. I have a bunch of scratch branches for #105 lying around (#605 among others, but that's not currently my favorite), I should probably try to finish that work; it seems like the same policy holds there, i.e. the special rules only apply to declarations.

Until that day comes, let's try this and see what falls out.

One thing: I try not to add to `badinc.cc`, since it's too large and unmaintainable already. Could you extract the new example to a separate test? Something that shows the difference between declaration and definition? `iwyu_stricter_than_cpp.*` are sort of related, but also a little bit too big and interconnected. A new testcase is probably/maybe better.