Pull request 1072 in include-what-you-use: Fix autocast to reference

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

New pull request 1072 by bolshakov-a: Fix autocast to reference
https://github.com/include-what-you-use/include-what-you-use/pull/1072

CastExpr is absent in the AST in such a case.

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

Comment #1 on pull request 1072 by kimgr: Fix autocast to reference
https://github.com/include-what-you-use/include-what-you-use/pull/1072

Nice, thanks!

I'm not sure about the approach -- the way I understand it, `Traverse` overrides are only necessary to customize the actual AST _traversal_ (e.g. visiting more nodes than the base RAV does, or skipping over nodes that it normally visits), which shouldn't be the case here.

The AST from the test looks like this:
```
tests/cxx/iwyu_stricter_than_cpp-d2.h:17:34: (2) [ CompoundStmt ] CompoundStmt 0x5572d77b9a28
|-ExprWithCleanups 0x5572d77b9860 'void'
| `-CallExpr 0x5572d77b94c0 'void'
| |-ImplicitCastExpr 0x5572d77b94a8 'void (*)(struct IndirectStruct2)' <FunctionToPointerDecay>
| | `-DeclRefExpr 0x5572d77b9458 'void (struct IndirectStruct2)' lvalue Function 0x5572d77b7710 'TwiceDeclaredFunction' 'void (struct IndirectStruct2)'
| `-CXXConstructExpr 0x5572d77b9830 'struct IndirectStruct2' 'void (struct IndirectStruct2 &&) noexcept' elidable
| `-MaterializeTemporaryExpr 0x5572d77b9708 'struct IndirectStruct2' xvalue
| `-ImplicitCastExpr 0x5572d77b95e0 'struct IndirectStruct2' <ConstructorConversion>
| `-CXXConstructExpr 0x5572d77b95b0 'struct IndirectStruct2' 'void (int)'
| `-IntegerLiteral 0x5572d77b9118 'int' 1
`-ExprWithCleanups 0x5572d77b99e0 'void'
`-CallExpr 0x5572d77b9940 'void'
|-ImplicitCastExpr 0x5572d77b9928 'void (*)(const struct IndirectStruct2 &)' <FunctionToPointerDecay>
| `-DeclRefExpr 0x5572d77b98e0 'void (const struct IndirectStruct2 &)' lvalue Function 0x5572d77b7858 'TwiceDeclaredRefFunction' 'void (const struct IndirectStruct2 &)'
`-MaterializeTemporaryExpr 0x5572d77b99c8 'const struct IndirectStruct2' lvalue
`-CXXConstructExpr 0x5572d77b9998 'const struct IndirectStruct2' 'void (int)'
`-IntegerLiteral 0x5572d77b98c0 'int' 1
```

so it seems to me there are two tree shapes we need to take into account:

* For `TwiceDeclaredFunction`: `CallExpr` / `CXXConstructExpr` / `MaterializeTemporaryExpr` / `ImplicitCastExpr(ConstructorConversion)` / `CXXConstructExpr` (handled today in `VisitCastExpr`)
* For `TwiceDeclaredRefFunction`: `CallExpr` / `MaterializeTemporaryExpr` / `CXXConstructExpr`

I wonder if it would make sense to work on all this in `VisitCXXConstructExpr` instead, and look back up the tree for both shapes? I'm not saying that's the way to go, I'm just thinking out loud. I don't have a good idea for how to look for tree shapes other than by looping up the `ASTNode` tree doing `IsA<NodeType>`, but it seems it would be nice to be able to express things like that as a single expression.

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

Comment #2 on pull request 1072 by bolshakov-a: Fix autocast to reference
https://github.com/include-what-you-use/include-what-you-use/pull/1072

Yes, I was in doubt whether `Traverse` should be used or `Visit` along with `isa<CXXConstructExpr>`. I can rewrite into the second variant.
I'm not sure whether checking actual tree structure makes sense. It seems like handling `CXXConstructExpr` inside `CallExpr` covers all the (test) cases. I thought that if there really are some cases when exact AST structure should be examined, such cases could be handled in a separate PR in the future.

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

Comment #3 on pull request 1072 by kimgr: Fix autocast to reference
https://github.com/include-what-you-use/include-what-you-use/pull/1072

Ah _now_ I understand the mention of `CXXTemporaryObjectExpr`. If you want to limit to exactly `CXXConstructExpr` (and not derivatives), you need to check with something like `expr->getStmtClass() == CXXConstructExprClass`.

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

Comment #3 on pull request 1072 by kimgr: Fix autocast to reference
https://github.com/include-what-you-use/include-what-you-use/pull/1072

Ah _now_ I understand the mention of `CXXTemporaryObjectExpr`. If you want to limit to exactly `CXXConstructExpr` (and not derivatives), you need to check with something like `expr->getStmtClass() == CXXConstructExprClass`.

And yes, `CXXConstructExpr` inside `CallExpr` might be a reasonable approximation. Just note that there are two `CXXConstructExpr`s in the subtree for the expr with the implicit conversion, and we probably don't want to signal both.

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

Comment #4 on pull request 1072 by bolshakov-a: Fix autocast to reference
https://github.com/include-what-you-use/include-what-you-use/pull/1072

> Just note that there are two CXXConstructExprs in the subtree for the expr with the implicit conversion, and we probably don't want to signal both.

Yes, thanks, I see now that it's really a double reporting. But I wonder why it should be concerned? What consequences it has?

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

Comment #5 on pull request 1072 by kimgr: Fix autocast to reference
https://github.com/include-what-you-use/include-what-you-use/pull/1072

On further thought, I think it will be discarded anyway, since the outer construct-expr does not match the implicit-conversion-constructor rule. I think.

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

Comment #6 on pull request 1072 by bolshakov-a: Fix autocast to reference
https://github.com/include-what-you-use/include-what-you-use/pull/1072

Sorry, I haven't understood. The suggested code doesn't have such a rule, just checking of exact node class instead, and that is `CXXConstructExpr` in both cases, not `CXXTemporaryObjectExpr`.

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

Comment #7 on pull request 1072 by kimgr: Fix autocast to reference
https://github.com/include-what-you-use/include-what-you-use/pull/1072

@bolshakov-a `GetCallerResponsibleTypesForAutocast` calls `HasImplicitConversionConstructor` to collect all parameter types that have an autocast-ing constructor.

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

Comment #8 on pull request 1072 by bolshakov-a: Fix autocast to reference
https://github.com/include-what-you-use/include-what-you-use/pull/1072

Yes, but `GetCallerResponsibleTypesForAutocast` unwinds the stack at first to get `CallExpr` and works with it, hence calls for inner and outer `CXXConstructExpr` should be equivalent.

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

Comment #9 on pull request 1072 by kimgr: Fix autocast to reference
https://github.com/include-what-you-use/include-what-you-use/pull/1072

@bolshakov-a Ah, I see that now. Makes sense. Let's get this merged.

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

Comment #10 on pull request 1072 by bolshakov-a: Fix autocast to reference
https://github.com/include-what-you-use/include-what-you-use/pull/1072

So, no problem with double-reporting?

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

Comment #11 on pull request 1072 by kimgr: Fix autocast to reference
https://github.com/include-what-you-use/include-what-you-use/pull/1072

Oops, maybe I was too quick. I lost track of why I asked.

I did get some ideas for simplifying refactors, so maybe I'll try and tackle that at the same time.