[clang-tidy] Check for uncessary copies as return value for getter functions

[ArnaudBienner]

The following code:

std::string GetValue() const { return mValue; }

(with mValue being a std::string member)

is not optimal since it might perform an unnecessary copy of the string object, which might not be needed.

The following code would be best:

const std::string& GetValue() const { return mValue; }

Similar to performance-unnecessary-value-param¶ I think it would be nice to have a check for this.

What do you think?

Probably only if the method is not virtual, since I can imagine cases where children classes might not always be able to return a const reference.

I guess some people might still prefer to return a copy, to avoid callers to potentially mess up things using const_cast, but for most users, I think it would be nice to have this check.

[llvmbot]

@llvm/issue-subscribers-clang-tidy

Author: None (ArnaudBienner)

The following code: ``` std::string GetValue() const { return mValue; } ``` (with `mValue` being a `std::string` member)

is not optimal since it might perform an unnecessary copy of the string object, which might not be needed.

The following code would be best:

const std::string& GetValue() const { return mValue; }

Similar to performance-unnecessary-value-param¶ I think it would be nice to have a check for this.

What do you think?

Probably only if the method is not virtual, since I can imagine cases where children classes might not always be able to return a const reference.

I guess some people might still prefer to return a copy, to avoid callers to potentially mess up things using const_cast, but for most users, I think it would be nice to have this check.

[ArnaudBienner]

For the record, I contributed a few patches to clang-tidy in the past, and I'm fine to try again to implement this, but would appreciate getting feedback on whether it's worth it or not :)

[PiotrZSL]

using clang::ast_matchers::isTemplateInstantiation;

namespace {
    AST_MATCHER(QualType, isTrivialType) {
        return Node.isTrivialType(Finder->getASTContext()) or
               Node.isTriviallyCopyableType(Finder->getASTContext());
    }

    AST_MATCHER(QualType, isInstantiationDependent) {
        return Node.getTypePtr()->isInstantiationDependentType();
    }

    AST_MATCHER_P(QualType, hasSizeBellow, unsigned, MaxSize)
    {
        return CheckBase::getTypeSize(Node, Finder->getASTContext()) < MaxSize;
    }
}


void PerformanceHeavyCopyOfMemberInReturnCheck::registerMatchers(MatchFinder *Finder) {

  auto returnsMember = returnStmt(has(memberExpr(hasObjectExpression(cxxThisExpr()), member(fieldDecl(hasType(qualType(hasCanonicalType(qualType(equalsBoundNode("rtype"))))))))));
  auto typesToIgnore = qualType(anyOf(voidType(),
                                      referenceType(),
                                      pointerType(),
                                      builtinType(),
                                      decltypeType(),
                                      autoType(),
                                      allOf(isTrivialType(), hasSizeBellow(m_minTrivialSize))));
  auto templateTypeToIgnore = classTemplateSpecializationDecl(hasSpecializedTemplate(matchesName(m_templateClassesRegexp)),
                                                              unless(hasAnyTemplateArgument(unless(anyOf(unless(refersToType(qualType())), refersToType(hasCanonicalType(qualType(typesToIgnore))))))));

  auto typesToIgnoreWithTemplate = qualType(anyOf(typesToIgnore,
                                                  hasDeclaration(templateTypeToIgnore)));

  Finder->addMatcher(traverse(TK_IgnoreUnlessSpelledInSource,
                              cxxMethodDecl(unless(isExpansionInSystemHeader()),
                                            isDefinition(),
                                            isUserProvided(),
                                            unless(isCopyAssignmentOperator()),
                                            unless(isMoveAssignmentOperator()),
                                            unless(isTemplateInstantiation()),
                                            ofClass(unless(matchesName(m_ignoreClassesRegex))),
                                            returns(qualType(unless(isInstantiationDependent()), hasCanonicalType(qualType(unless(typesToIgnoreWithTemplate)).bind("rtype")))),
                                            hasBody(stmt(hasDescendant(returnStmt(returnsMember).bind("return")),
                                                         unless(hasDescendant(returnStmt(unless(returnsMember))))))
                     ).bind("method")), this);
}

void PerformanceHeavyCopyOfMemberInReturnCheck::check(const MatchFinder::MatchResult &Result) {
  const auto *t = Result.Nodes.getNodeAs<QualType>("rtype");

  auto n = t->getAsString();
  if (m_ignoreTypes.match(n)) return;

  const auto *m = Result.Nodes.getNodeAs<CXXMethodDecl>("method");
  const auto *r = Result.Nodes.getNodeAs<ReturnStmt>("return");

  auto size = CheckBase::getTypeSize(*t, *Result.Context);

  diag(m->getLocation(), "function %0 returns heavy member of type '%1' (size: %2) by value, consider returning reference (const &, std::span, ...) to avoid unnecessary copy")
      << m << n << size;
  diagNote(r->getBeginLoc(), "return is here");
}

  void storeOptions(ClangTidyOptions::OptionMap &Opts) override {
      Options.store(Opts, "MinTrivialSize", m_minTrivialSize);
      Options.store(Opts, "IgnoreTypesRegex", m_ignoreTypesRegex);
      Options.store(Opts, "IgnoreClassesRegex", m_ignoreClassesRegex);
      Options.store(Opts, "TemplateClassesRegexp", m_templateClassesRegexp);
  }

@ArnaudBienner In short, yes it's worth it but you need to ignore some types and only take into account more complicated ones, things to ignore: basic types, pointers, references, trivial types of small size, some custom metods, some custom types, auto& types, template dependent types/methods

I have this check implemented, just didn't had time to push it upstream. I paste code. If you want to base on it, you may need to mark me as co-author, if you want just to use it as an hint, then you don't need to.

[firewave]

We also implemented this in Cppcheck about a year ago and only report it in very narrow cases - see https://trac.cppcheck.net/ticket/12546.

[vbvictor]

Be aware of dangling references if your check will automatically transform T to cosnt T& in return value of a getter-methods.
e.g. perfectly fine code:

class A {
 public:
  A(int a) { /* some non-trivial logic that compute s */ };
  std::string getS() { return s; };
 private:
  std::string s;
}

void process(const std::string& res);

int main() {
  const std::string& result = A(42).getS(); // it's okay - lifetime extension for lvalue ref work
  process(result);
}

can turn into dangerous:

class A {
 public:
  A(int a) { /* some non-trivial logic that compute s */ };
  const std::string& getS() const { return s; };
 private:
  std::string s = "fdsfsd";
};

void process(const std::string& res);

int main() {
  const std::string& result = A(42).getS(); // bad - we get dangling reference
  process(result);
}

This particular case can be caught with [[clang::lifetimebound]] attribute. See https://godbolt.org/z/TWY1bTTdo.
So my point is you either need to check for A(42).getS() patterns and don't provide fix-its for this case, or provide fix-its as a NOTE and warn users about how dangerous this transformation can be.

[ArnaudBienner]

Thanks all for your feedback. It's very interesting :)

@PiotrZSL thanks for the code snippet. I'll let you know if I start working on this, and of course add you as co-author in case I reuse what you've done.

@firewave thanks. It's nice to see cppheck supports this, and how it was implemented.

@vbvictor good point. I believe, however, that this might be outside of the scope of this request. I think most C++ developers are already doing this (returning const& for getters) so I think, independently from my request, it might be good to better track lifetime of objects and have a warning in cases like the one you suggested. Are you aware of any work on this? (i.e. to catch those bugs without the need to have [[clang::lifetimebound]] on methods).

[vbvictor]

AFAIK for gcc there is -Wdangling-reference flag that catch error in my example. For clang there is flag -Wdangling which did not catch error in my example, only with [[clang::lifetimebound]] it can catch this case.
Running clang-tidy with all checks enabled also did not find any mistakes, https://godbolt.org/z/rvhqMK48K.

My main concern is that users "just run" this check over the whole codebase expecting "free performance" but can get bugs.
The check is good, but with fixes enabled via --fix-notes

[firewave]

AFAIK for gcc there is -Wdangling-reference flag that catch error in my example.

That warning appears to be a bit overreaching - see #125542 and #125544. That might lead to people just turning it off (forever). Unfortunately I have not looked into this yet. So it looks like it swings both ways.

My main concern is that users "just run" this check over the whole codebase expecting "free performance" but can get bugs.

As soon as you have suggestions and fix-its that will always be the case. If those do not exist and people need to think about the changes... that might actually not change anything. But let's stay on topic. That's why I outlined such a narrow corridor of detection in terms of Cppcheck. But we also had a couple of false positives: https://trac.cppcheck.net/query?status=accepted&status=assigned&status=closed&status=new&status=reopened&keywords=~returnByReference&order=priority.

BTW this appears to be a duplicate of #25256.

[vbvictor]

As soon as you have suggestions and fix-its that will always be the case. If those do not exist and people need to think about the changes...

Looking at other checks, particularly in performance- category, it's kind of easy to verify correctness of applied fix-its because they change some local part of code that is not hard to review. However, when we change the return value of a getter, we need to review every single place where that getter was called to avoid dangling ref, and that is a hard task.

[firewave]

However, when we change the return value of a getter, we need to review every single place where that getter was called to avoid dangling ref, and that is a hard task.

That is valid but should only apply in cases you were assigning the result to a reference (which I always found silly and very misleading - there should be a check for that). In case of unnecessary copies it should(?) trigger performance-unnecessary-copy-initialization and you would hope that it is not blindly changed. And that's where the aforementioned [[clang::lifetimebound]] would come in.

[denzor200]

Be aware of dangling references if your check will automatically transform T to cosnt T& in return value of a getter-methods.

I suppose this is a job for "bugprone-return-const-ref-from-parameter" check ideally. This is a strange that this check can't find such mistakes, so I created issue about that: #140473

[vbvictor]

I suppose this is a job for "bugprone-return-const-ref-from-parameter" check ideally. This is a strange that this check can't find such mistakes, so I created issue about that: #140473

"bugprone-return-const-ref-from-parameter" doesn't catch this cases because it wasn't intended to do so.
const std::string& getS() const { return s; }; doesn't return any const& parameters.
IMO some general "dangling ref catching" mechanism should be a part of static analyzer.