Re-export types to overridden method signatures #675

kimgr · 2019-03-31T18:54:04Z

When a method overrides a virtual method in a base class, all types
mentioned in the signature will already be covered for by the base's
header.

As we must derive from a base to override methods, and derivation
requires a complete type, we already know that we've included the base
header to be able to override a method.

Exploit this and ignore all uses in overridden method signatures as they
must already be satisifed for the code to compile.

For example:

// base.h
#include <string>
struct Base {
  std::string method() const;
};

// derived.h
#include "base.h"
struct Derived : public Base {
  // No warning about <string> here.
  std::string method();
};

But this does not extend to the all uses of the type. For example, if we
use the type in the body of an overridden method, the definition file
must still include the type:

// derived.cc
#include "derived.h"

// This mention of std::string is ignored, as above.
std::string Derived::method() {
  // But this requires <string>.
  return std::string("abc");
}

This reduces the number of required includes in headers significantly.

kimgr · 2019-03-31T18:54:18Z

Should close #105.

jru · 2019-04-29T06:41:17Z

Will this also work if the return type (or param) of a base method is only forward-declared in base.h? A test showing that would be good.

kimgr · 2019-04-29T07:01:30Z

Thanks, I'll look into it. There's also the question of the meaning of "works" here 🙂. Forward-declaring the return type is a hint that users are responsible, so I need to make sure the transitivity works all the way through use of derived to derived to base.

kimgr · 2019-07-09T12:03:17Z

I added some tests for forward-declares, which led to a slight improvement -- the compiler disallows a method definition if the return type is incomplete, even if the type is not mentioned in the body. So I adjusted the check to only cover pure declarations.

This seems to work according to expectations, please take another look.

jru

Hey! Some minor things and a suggestion about how to better handle override methods with definition. Great stuff!

iwyu_ast_util.cc

iwyu.cc

iwyu_ast_util.cc

iwyu_output.cc

With range loops, auto and judicious use of a temporary, this became much easier to read.

When a method overrides a virtual method in a base class, all types mentioned in the signature will already be covered for by the base's header. As we must derive from a base to override methods, and derivation requires a complete type, we already know that we've included the base header to be able to override a method. Exploit this and ignore all uses in overridden method signatures as they must already be satisifed for the code to compile. For example: // base.h #include <string> class Foo; struct Base { virtual std::string method(); virtual Foo foo_method(); }; // derived.h #include "base.h" struct Derived : public Base { // No warnings about <string> or Foo here. std::string method(); Foo foo_method(); }; But this does not extend to the all uses of the type. For example, if we use the type in the definition of an overridden method, the definition file must still include the type: // derived.cc #include "derived.h" // This requires <string>. std::string Derived::method() { return std::string("abc"); } // This requires the complete type of Foo, // even if it's not mentioned in the body. Foo Derived::foo_method() { abort(); } This reduces the number of required includes in headers significantly. Fixes issue include-what-you-use#105.

kimgr · 2020-05-31T20:58:28Z

Updated. I think this is even better:

Definitions are now processed the same way, i.e. the signature of an override is ignored, but any uses in the body are not
Renamed tests
Renamed and commented the most central function, IsNodeInsideCXXMethodOverrideSignature

If someone has time to take a look, I'd be delighted. Will probably merge this soonish as is if I don't hear anything.

jru · 2020-06-04T21:07:34Z

Hey! I will take a look :)

jru

Looking good! Left a few minor comments.

jru · 2020-06-06T16:03:56Z

iwyu.cc

}
+

It has been a while and not recall exactly, but could it be that there is only a FunctionProtoType without a corresponding FunctionDecl? Because in that case the new code won't find the prototype.

jru · 2020-06-06T16:07:32Z

iwyu_ast_util.cc

+  if (HasCovariantReturnType(method)) {
+    // Covariant return types require that the full return type is available,
+    // so don't special-case the return type for functions with covariance.
+  } else {


I find this a bit confusing. Why not invert the condition and explain what we do for non-covariant return types?

jru · 2020-06-06T16:14:04Z

iwyu_ast_util.cc

+
+    // The return type could be a type loc, though never seen this in practice.
+    const TypeSourceInfo* tsi = method->getTypeSourceInfo();
+    const TypeLoc retloc = tsi->getTypeLoc();


Doesn't this refer to the type of the method itself?

jru · 2020-06-06T16:25:26Z

tests/cxx/reexport_overridden.h

+  // Unused argument available from Base, in inline method.
+  void ArgumentUnusedInline(const IndirectClass& x) override {
+  }
+};


Not sure if really needed, but it could be interesting to see that a method not overridden from the parent requires its types to be included directly.

mactalla · 2020-09-21T19:21:36Z

I just tried out the branch and it works nicely. Thanks @kimgr ! It doesn't seem to re-export the return type, but it does do the parameter types and that cuts down on quite a lot of noise form IWYU.

What's needed to get this PR merged?

kimgr · 2020-09-21T19:28:32Z

@mactalla Thank you, good to hear! I've been experimenting with alternative solutions (lots of them!) offline, but still don't have anything I'm entirely happy with.

Maybe I should pick a favorite and get it merged 🙂.

I'm a little pressed for time right now, but I'll make an effort to cobble something together soon.

kimgr · 2020-11-22T13:07:23Z

Just to make sure there's some update: I'm still working on/investigating solutions for this offline. It's a surprisingly tricky problem.

My current take is:

Override TraverseFunctionProtoTypeLoc -- it has all the relevant information available (return type, parameter type, exception spec types, any associated FunctionDecl can be figured out)
Traverse return type separately, taking IWYU's policy into account
Traverse parameter types separately, taking IWYU's policy into account
Traverse exception spec and noexcept expressions as normal

There's quite a bit of reporting machinery to get the (for fn return type) and (for autocast) comments in place, as well as maintaining forward-declare context, so unfortunately I haven't been able to make this agree with some of the template tests.

So I think there's some design shortcut for templates that needs to be resolved first, and then this might be easier.

I'll keep at it.

bolshakov-a · 2022-09-04T13:39:49Z

@kimgr, you may take a look on my variant. I block method declaration traversal except default arguments which may differ from declaration in base class (not sure about exception specifications). Covariant return types are handled in VisitCXXMethodDecl as before. Method definitions are traversed as before, hence return type is reported even if it isn't mentioned in the method body (your variant don't treat it properly, as your test shows).
The drawback is that not only #includes are pulled from .h into .cpp, but also forward declarations of unused parameters passed by reference or pointer. But I think that forward declarations could be suppressed in .cpp-files at all. You either need a full type, or you already should have (forward-)declaration from one of the headers included. Otherwise, you just have an unused garbage in your code, and it would be fine if compiler will point on it. I haven't succeeded in finding any counterexample.

bolshakov-a · 2022-09-04T13:45:38Z

But, maybe, just adding base class headers into associated headers is better. It would also cover type reporting for public and protected field members, which is also redundant for derived classes. On the other hand, private members are base class implementation details, and subclasses should not rely on presence their type info in base class header. Not sure, how acceptable it would be...

kimgr force-pushed the 105-override branch from db764a0 to 88543d8 Compare July 9, 2019 11:59

jru reviewed Jul 13, 2019

View reviewed changes

iwyu_ast_util.cc Outdated Show resolved Hide resolved

iwyu.cc Outdated Show resolved Hide resolved

iwyu_ast_util.cc Outdated Show resolved Hide resolved

iwyu_output.cc Outdated Show resolved Hide resolved

kimgr force-pushed the 105-override branch 2 times, most recently from 95885b0 to 118cd9e Compare July 15, 2019 19:58

jru approved these changes Jul 18, 2019

View reviewed changes

kimgr force-pushed the 105-override branch from 118cd9e to e5351d2 Compare December 27, 2019 21:37

kimgr force-pushed the 105-override branch from e5351d2 to 3cb329c Compare May 31, 2020 20:49

kimgr added 2 commits May 31, 2020 22:49

Clean up VisitType

f04ef13

With range loops, auto and judicious use of a temporary, this became much easier to read.

kimgr force-pushed the 105-override branch from 3cb329c to c9d84a1 Compare May 31, 2020 20:49

jru reviewed Jun 6, 2020

View reviewed changes

kimgr mentioned this pull request Sep 3, 2022

Avoid reporting type for "autocast" for function definition #1083

Merged

bolshakov-a mentioned this pull request Sep 25, 2022

IWYU pragma: export not effective for forward declared enum class #1115

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Re-export types to overridden method signatures #675

Re-export types to overridden method signatures #675

kimgr commented Mar 31, 2019

kimgr commented Mar 31, 2019

jru commented Apr 29, 2019

kimgr commented Apr 29, 2019

kimgr commented Jul 9, 2019

jru left a comment

kimgr commented May 31, 2020

jru commented Jun 4, 2020

jru left a comment

jru Jun 6, 2020

jru Jun 6, 2020

jru Jun 6, 2020

jru Jun 6, 2020

mactalla commented Sep 21, 2020

kimgr commented Sep 21, 2020

kimgr commented Nov 22, 2020

bolshakov-a commented Sep 4, 2022

bolshakov-a commented Sep 4, 2022 •

edited

Re-export types to overridden method signatures #675

Are you sure you want to change the base?

Re-export types to overridden method signatures #675

Conversation

kimgr commented Mar 31, 2019

kimgr commented Mar 31, 2019

jru commented Apr 29, 2019

kimgr commented Apr 29, 2019

kimgr commented Jul 9, 2019

jru left a comment

Choose a reason for hiding this comment

kimgr commented May 31, 2020

jru commented Jun 4, 2020

jru left a comment

Choose a reason for hiding this comment

jru Jun 6, 2020

Choose a reason for hiding this comment

jru Jun 6, 2020

Choose a reason for hiding this comment

jru Jun 6, 2020

Choose a reason for hiding this comment

jru Jun 6, 2020

Choose a reason for hiding this comment

mactalla commented Sep 21, 2020

kimgr commented Sep 21, 2020

kimgr commented Nov 22, 2020

bolshakov-a commented Sep 4, 2022

bolshakov-a commented Sep 4, 2022 • edited

bolshakov-a commented Sep 4, 2022 •

edited