[Bug c++/106393] New: Add warnings for common dangling problems

[Bug c++/106393] New: Add warnings for common dangling problems

[redi at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=106393

            Bug ID: 106393
           Summary: Add warnings for common dangling problems
           Product: gcc
           Version: 13.0
            Status: UNCONFIRMED
          Keywords: diagnostic
          Severity: enhancement
          Priority: P3
         Component: c++
          Assignee: unassigned at gcc dot gnu.org
          Reporter: redi at gcc dot gnu.org
            Blocks: 87403
  Target Milestone: ---

It would be great if G++ was able to warn about the dangling problems below,
some of which are easier than others. Some might be too hard to do without
-fanalyze support for C++, some might be practical to do by leveraging the new
__reference_converts_from_temporary machinery to detect problems, some might
need entirely new ideas.


Simple:

const int& f(const int& i) { return i; }
const int& i = f(10);

This creates a temporary from the literal, returns a reference to the
temporary, but the temporary is destroyed at the end of the full expression.
We've had numerous bugs reported for this case when using std::max and
std::min.


Hard:

std::string_view s = std::string("blah");

This creates a temporary std::string that copies the literal "blah" to its own
memory (in this case in an internal buffer, but for longer strings into heap
buffers), then calls a member function of std::string that creates a string
view that holds a pointer to that memory, then destroys the std::string.

The analyzer should be able to tell that the pointer in the string view refers
to something that either goes out of scope with the string, or is deallocated
in the string's dtor. Doing it in the FE or ME without -fanalyze might be hard.

Maybe we could have a heuristic that warns when:

- constructing a local variable whose type is a borrowed view
- from an rvalue, non-view, non-borrowed range.

(Thanks to Ville V and Barry R for brainstorming this).
There will be some edge cases where the non-view range has iterators that do
outlive the range itself, and borrowing them is safe, but that will be rare.

The FE would either have to check the std::ranges::view and
std::ranges::borrowed_range concepts (and if they're not in scope, just don't
check ... it's probably not a view or a range if <ranges> isn't included!) or
as a simpler heuristic, just check for the value of the enable_view and
enable_borrowed_range bool variable templates.


Galaxy Brain:

#include <ranges>
#include <string>
#include <iostream>

std::string f() { return "dangle dangle dangle"; }

int main()
{
  auto v = std::string_view(f()) | std::views::transform([](char c) {
    if (c == 'l') return 'e';
    if (c == 'e') return 'r';
    return c;
  });
  for (char c : v)
    std::cout << c;
  std::cout << '\n';
}

Constructing a temporary string view from a std::string is fine, but then we
create another view of that temporary string view that outlives the thing the
string view refers to (the temporary std::string). By the time we iterate over
the range, the std::string is long gone.


Referenced Bugs:

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=87403
[Bug 87403] [Meta-bug] Issues that suggest a new warning

[Bug c++/106393] Add warnings for common dangling problems

[mpolacek at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=106393

Marek Polacek <mpolacek at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Assignee|unassigned at gcc dot gnu.org      |mpolacek at gcc dot gnu.org
             Status|UNCONFIRMED                 |ASSIGNED
     Ever confirmed|0                           |1
                 CC|                            |mpolacek at gcc dot gnu.org
   Last reconfirmed|                            |2022-07-21

[Bug c++/106393] Add warnings for common dangling problems

[cvs-commit at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=106393

--- Comment #1 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The trunk branch has been updated by Marek Polacek <mpolacek@gcc.gnu.org>:

https://gcc.gnu.org/g:d2249cd9adf5ae638577139177a50f7e62d8abd9

commit r13-3511-gd2249cd9adf5ae638577139177a50f7e62d8abd9
Author: Marek Polacek <polacek@redhat.com>
Date:   Fri Oct 14 10:05:57 2022 -0400

    c++: Implement -Wdangling-reference [PR106393]

    This patch implements a new experimental warning (enabled by -Wall) to
    detect references bound to temporaries whose lifetime has ended.  The
    primary motivation is the Note in
    <https://en.cppreference.com/w/cpp/algorithm/max>:

      Capturing the result of std::max by reference produces a dangling
reference
      if one of the parameters is a temporary and that parameter is returned:

      int n = 1;
      const int& r = std::max(n-1, n+1); // r is dangling

    That's because both temporaries for n-1 and n+1 are destroyed at the end
    of the full expression.  With this warning enabled, you'll get:

    g.C:3:12: warning: possibly dangling reference to a temporary
[-Wdangling-reference]
        3 | const int& r = std::max(n-1, n+1);
          |            ^
    g.C:3:24: note: the temporary was destroyed at the end of the full
expression 'std::max<int>((n - 1), (n + 1))'
        3 | const int& r = std::max(n-1, n+1);
          |                ~~~~~~~~^~~~~~~~~~

    The warning works by checking if a reference is initialized with a function
    that returns a reference, and at least one parameter of the function is
    a reference that is bound to a temporary.  It assumes that such a function
    actually returns one of its arguments!  (I added code to check_return_expr
    to suppress the warning when we've seen the definition of the function
    and we can say that it can return a variable with static storage
    duration.)

    It warns when the function in question is a member function, but only if
    the function is invoked on a temporary object, otherwise the warning
    would emit loads of warnings for valid code like obj.emplace<T>({0}, 0).
    It does detect the dangling reference in:

      struct S {
        const S& self () { return *this; }
      };
      const S& s = S().self();

    It warns in member initializer lists as well:

      const int& f(const int& i) { return i; }
      struct S {
        const int &r;
        S() : r(f(10)) { }
      };

    I've run the testsuite/bootstrap with the warning enabled by default.
    There were just a few FAILs, all of which look like genuine bugs.
    A bootstrap with the warning enabled by default passed as well.

    When testing a previous version of the patch, there were many FAILs in
    libstdc++'s 22_locale/; all of them because the warning triggered on

      const test_type& obj = std::use_facet<test_type>(std::locale());

    but this code looks valid -- std::use_facet doesn't return a reference
    to its parameter.  Therefore I added a #pragma and code to suppress the
    warning.

            PR c++/106393

    gcc/c-family/ChangeLog:

            * c.opt (Wdangling-reference): New.

    gcc/cp/ChangeLog:

            * call.cc (expr_represents_temporary_p): New, factored out of...
            (conv_binds_ref_to_temporary): ...here.  Don't return false just
            because a ck_base is missing.  Use expr_represents_temporary_p.
            (do_warn_dangling_reference): New.
            (maybe_warn_dangling_reference): New.
            (extend_ref_init_temps): Call maybe_warn_dangling_reference.
            * cp-tree.h: Adjust comment.
            * typeck.cc (check_return_expr): Suppress -Wdangling-reference
            warnings.

    gcc/ChangeLog:

            * doc/invoke.texi: Document -Wdangling-reference.

    libstdc++-v3/ChangeLog:

            * include/bits/locale_classes.tcc: Add #pragma to disable
            -Wdangling-reference with std::use_facet.

    gcc/testsuite/ChangeLog:

            * g++.dg/cpp23/elision4.C: Use -Wdangling-reference, add
dg-warning.
            * g++.dg/cpp23/elision7.C: Likewise.
            * g++.dg/warn/Wdangling-pointer-2.C: Use -Wno-dangling-reference.
            * g++.dg/warn/Wdangling-reference1.C: New test.
            * g++.dg/warn/Wdangling-reference2.C: New test.
            * g++.dg/warn/Wdangling-reference3.C: New test.

[Bug c++/106393] Add warnings for common dangling problems

[mpolacek at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=106393

Marek Polacek <mpolacek at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Assignee|mpolacek at gcc dot gnu.org        |unassigned at gcc dot gnu.org
             Status|ASSIGNED                    |NEW

--- Comment #2 from Marek Polacek <mpolacek at gcc dot gnu.org> ---
The first test in the original post is now diagnosed.  The rest may have to be
implemented in the analyzer.  I'm not sure if I should close the BZ.