Re: [PATCH] c++: print source code in print_instantiation_partial_context_line

[Patrick Palka <ppalka@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 18165 bytes --]

On Tue, 9 Apr 2024, Patrick Palka wrote:

> On Thu, 19 Oct 2023, Patrick Palka wrote:
> 
> > On Tue, 3 Oct 2023, David Malcolm wrote:
> > 
> > > As mentioned in my Cauldron talk, this patch adds a call to
> > > diagnostic_show_locus to the "required from here" messages
> > > in print_instantiation_partial_context_line, so that e.g., rather
> > > than the rather mystifying:
> > > 
> > > In file included from ../x86_64-pc-linux-gnu/libstdc++-v3/include/memory:78,
> > >                  from ../../src/demo-1.C:1:
> > > ../x86_64-pc-linux-gnu/libstdc++-v3/include/bits/unique_ptr.h: In instantiation of ‘std::__detail::__unique_ptr_t<_Tp> std::make_unique(_Args&& ...) [with _Tp = bar; _Args = {}; __detail::__unique_ptr_t<_Tp> = __detail::__unique_ptr_t<bar>]’:
> > > ../../src/demo-1.C:15:32:   required from here
> > > ../x86_64-pc-linux-gnu/libstdc++-v3/include/bits/unique_ptr.h:1066:30: error: no matching function for call to ‘bar::bar()’
> > >  1066 |     { return unique_ptr<_Tp>(new _Tp(std::forward<_Args>(__args)...)); }
> > >       |                              ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > > ../../src/demo-1.C:10:3: note: candidate: ‘bar::bar(int)’
> > >    10 |   bar (int);
> > >       |   ^~~
> > > ../../src/demo-1.C:10:3: note:   candidate expects 1 argument, 0 provided
> > > ../../src/demo-1.C:7:7: note: candidate: ‘constexpr bar::bar(const bar&)’
> > >     7 | class bar : public foo
> > >       |       ^~~
> > > ../../src/demo-1.C:7:7: note:   candidate expects 1 argument, 0 provided
> > > ../../src/demo-1.C:7:7: note: candidate: ‘constexpr bar::bar(bar&&)’
> > > ../../src/demo-1.C:7:7: note:   candidate expects 1 argument, 0 provided
> > > 
> > > we emit:
> > > 
> > > In file included from ../x86_64-pc-linux-gnu/libstdc++-v3/include/memory:78,
> > >                  from ../../src/demo-1.C:1:
> > > ../x86_64-pc-linux-gnu/libstdc++-v3/include/bits/unique_ptr.h: In instantiation of ‘std::__detail::__unique_ptr_t<_Tp> std::make_unique(_Args&& ...) [with _Tp = bar; _Args = {}; __detail::__unique_ptr_t<_Tp> = __detail::__unique_ptr_t<bar>]’:
> > > ../../src/demo-1.C:15:32:   required from here
> > >    15 |   return std::make_unique<bar> ();
> > >       |          ~~~~~~~~~~~~~~~~~~~~~~^~
> > > ../x86_64-pc-linux-gnu/libstdc++-v3/include/bits/unique_ptr.h:1066:30: error: no matching function for call to ‘bar::bar()’
> > >  1066 |     { return unique_ptr<_Tp>(new _Tp(std::forward<_Args>(__args)...)); }
> > >       |                              ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > > ../../src/demo-1.C:10:3: note: candidate: ‘bar::bar(int)’
> > >    10 |   bar (int);
> > >       |   ^~~
> > > ../../src/demo-1.C:10:3: note:   candidate expects 1 argument, 0 provided
> > > ../../src/demo-1.C:7:7: note: candidate: ‘constexpr bar::bar(const bar&)’
> > >     7 | class bar : public foo
> > >       |       ^~~
> > > ../../src/demo-1.C:7:7: note:   candidate expects 1 argument, 0 provided
> > > ../../src/demo-1.C:7:7: note: candidate: ‘constexpr bar::bar(bar&&)’
> > > ../../src/demo-1.C:7:7: note:   candidate expects 1 argument, 0 provided
> > > 
> > > which shows the code that's leading to the error (the bad call to
> > > std::make_unique).
> > 
> > This is great!  I noticed however that the source code gets printed in a
> > surprising way in some contexts.  Consider:
> > 
> > template<class T> void f(typename T::type);
> > 
> > int main() {
> >   f<int>(0);
> > }
> > 
> > For this testcase we emit:
> > 
> > testcase.C: In function ‘int main()’:
> > testcase.C:4:9: error: no matching function for call to ‘f<int>(int)’
> >     4 |   f<int>(0);
> >       |   ~~~~~~^~~
> > testcase.C:1:24: note: candidate: ‘template<class T> void f(typename T::type)’
> >     1 | template<class T> void f(typename T::type);
> >       |                        ^
> > testcase.C:1:24: note:   template argument deduction/substitution failed:
> > testcase.C: In substitution of ‘template<class T> void f(typename T::type) [with T = int]’:
> > testcase.C:4:9:   required from here
> > testcase.C:1:24: note:     4 |   f<int>(0);
> > testcase.C:1:24: note:       |   ~~~~~~^~~
> > testcase.C:1:24: error: ‘int’ is not a class, struct, or union type
> >     1 | template<class T> void f(typename T::type);
> >       |                        ^
> > 
> > In particular the source code part following the "required from here" line
> > 
> > testcase.C:4:9:   required from here
> > testcase.C:1:24: note:     4 |   f<int>(0);
> > testcase.C:1:24: note:       |   ~~~~~~^~~
> > 
> > seems off, I would have expected it be
> > 
> > testcase.C:4:9:   required from here
> >     4 |   f<int>(0);
> >       |   ~~~~~~^~~
> > 
> > i.e. without the "testcase.C:1:24: note:  " prefix.  Does this look
> > expected?  (I also wonder if we might want to omit printing the source
> > code altogether in this case, since we already printed that same line
> > earlier during the "no matching function" error?)
> 
> Sorry for forgetting to ping this earlier, but I wonder what if anything
> we should do about this for GCC 14?

Here's another example of the source code rendering issue using a test
from the libstdc++ testsuite:


$ g++ -D_GLIBCXX_DEBUG  -Wsystem-headers -Wsign-compare -std=c++23 -I libstdc++-v3/testsuite/util libstdc++-v3/testsuite/std/ranges/adaptors/enumerate/1.cc
In file included from /scratchpad/gcc-build/x86_64-pc-linux-gnu/libstdc++-v3/include/bits/version.h:49,
                 from /scratchpad/gcc-build/x86_64-pc-linux-gnu/libstdc++-v3/include/concepts:36,
                 from /scratchpad/gcc-build/x86_64-pc-linux-gnu/libstdc++-v3/include/ranges:37,
                 from libstdc++-v3/testsuite/std/ranges/adaptors/enumerate/1.cc:4:
/scratchpad/gcc-build/x86_64-pc-linux-gnu/libstdc++-v3/include/bits/basic_string.tcc: In instantiation of ‘constexpr void std::__cxx11::basic_string<_CharT, _Traits, _Alloc>::resize_and_overwrite(size_type, _Operation) [with _Operation = std::__format::__formatter_fp<wchar_t>::format<float, std::__format::_Sink_iter<wchar_t> >(float, std::basic_format_context<std::__format::_Sink_iter<wchar_t>, wchar_t>&) const::<lambda(char*, std::size_t)>&; _CharT = char; _Traits = std::char_traits<char>; _Alloc = std::allocator<char>; size_type = long unsigned int]’:
[ ... snip ... ]
/scratchpad/gcc-build/x86_64-pc-linux-gnu/libstdc++-v3/include/format:4038:23:   required from ‘constexpr void std::__format::_Formatting_scanner<_Out, _CharT>::_M_format_arg(std::size_t) [with _Out = std::__format::_Sink_iter<wchar_t>; _CharT = wchar_t; std::size_t = long unsigned int]’
 4038 |         std::visit_format_arg([this](auto& __arg) {
      |         ~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~
 4039 |           using _Type = remove_reference_t<decltype(__arg)>;
      |           ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
 4040 |           using _Formatter = typename _Context::template formatter_type<_Type>;
      |           ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
 4041 |           if constexpr (is_same_v<_Type, monostate>)
      |           ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
 4042 |             __format::__invalid_arg_id_in_format_string();
      |             ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
 4043 |           else if constexpr (is_same_v<_Type, handle>)
      |           ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
 4044 |             __arg.format(this->_M_pc, this->_M_fc);
      |             ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
 4045 |           else if constexpr (is_default_constructible_v<_Formatter>)
      |           ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
 4046 |             {
      |             ~
 4047 |               _Formatter __f;
      |               ~~~~~~~~~~~~~~~
 4048 |               this->_M_pc.advance_to(__f.parse(this->_M_pc));
      |               ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
 4049 |               this->_M_fc.advance_to(__f.format(__arg, this->_M_fc));
      |               ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
 4050 |             }
      |             ~
 4051 |           else
      |           ~~~~
 4052 |             static_assert(__format::__formattable_with<_Type, _Context>);
      |             ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
 4053 |         }, _M_fc.arg(__id));
      |         ~~~~~~~~~~~~~~~~~~~
/scratchpad/gcc-build/x86_64-pc-linux-gnu/libstdc++-v3/include/format:4033:7:   required from here
 4033 |       _M_format_arg(size_t __id) override
      |       ^~~~~~~~~~~~~
/scratchpad/gcc-build/x86_64-pc-linux-gnu/libstdc++-v3/include/bits/basic_string.tcc:609:7: warning: comparison of integer expressions of different signedness: ‘long int’ and ‘const std::__cxx11::basic_string<char>::size_type’ {aka ‘const long unsigned int’} [-Wsign-compare]
  609 |       _GLIBCXX_DEBUG_ASSERT(__r >= 0 && __r <= __n);
      |       ^~~~~~~~~~~~~~~~~~~~~
/scratchpad/gcc-build/x86_64-pc-linux-gnu/libstdc++-v3/include/bits/basic_string.tcc: In instantiation of ‘constexpr void std::__cxx11::basic_string<_CharT, _Traits, _Alloc>::resize_and_overwrite(size_type, _Operation) [with _Operation = std::__format::__formatter_fp<wchar_t>::format<double, std::__format::_Sink_iter<wchar_t> >(double, std::basic_format_context<std::__format::_Sink_iter<wchar_t>, wchar_t>&) const::<lambda(char*, std::size_t)>&; _CharT = char; _Traits = std::char_traits<char>; _Alloc = std::allocator<char>; size_type = long unsigned int]’:
[ ... snip ... ]
/scratchpad/gcc-build/x86_64-pc-linux-gnu/libstdc++-v3/include/format:4038:23:   required from ‘constexpr void std::__format::_Formatting_scanner<_Out, _CharT>::_M_format_arg(std::size_t) [with _Out = std::__format::_Sink_iter<wchar_t>; _CharT = wchar_t; std::size_t = long unsigned int]’
/scratchpad/gcc-build/x86_64-pc-linux-gnu/libstdc++-v3/include/bits/basic_string.tcc:609:7: warning:  4038 |         std::visit_format_arg([this](auto& __arg) {
/scratchpad/gcc-build/x86_64-pc-linux-gnu/libstdc++-v3/include/bits/basic_string.tcc:609:7: warning:       |         ~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~
    4039 |           using _Type = remove_reference_t<decltype(__arg)>;
/scratchpad/gcc-build/x86_64-pc-linux-gnu/libstdc++-v3/include/bits/basic_string.tcc:609:7: warning:       |           ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
    4040 |           using _Formatter = typename _Context::template formatter_type<_Type>;
/scratchpad/gcc-build/x86_64-pc-linux-gnu/libstdc++-v3/include/bits/basic_string.tcc:609:7: warning:       |           ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
    4041 |           if constexpr (is_same_v<_Type, monostate>)
/scratchpad/gcc-build/x86_64-pc-linux-gnu/libstdc++-v3/include/bits/basic_string.tcc:609:7: warning:       |           ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
    4042 |             __format::__invalid_arg_id_in_format_string();
/scratchpad/gcc-build/x86_64-pc-linux-gnu/libstdc++-v3/include/bits/basic_string.tcc:609:7: warning:       |             ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
    4043 |           else if constexpr (is_same_v<_Type, handle>)
/scratchpad/gcc-build/x86_64-pc-linux-gnu/libstdc++-v3/include/bits/basic_string.tcc:609:7: warning:       |           ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
    4044 |             __arg.format(this->_M_pc, this->_M_fc);
/scratchpad/gcc-build/x86_64-pc-linux-gnu/libstdc++-v3/include/bits/basic_string.tcc:609:7: warning:       |             ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
    4045 |           else if constexpr (is_default_constructible_v<_Formatter>)
/scratchpad/gcc-build/x86_64-pc-linux-gnu/libstdc++-v3/include/bits/basic_string.tcc:609:7: warning:       |           ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
    4046 |             {
/scratchpad/gcc-build/x86_64-pc-linux-gnu/libstdc++-v3/include/bits/basic_string.tcc:609:7: warning:       |             ~
    4047 |               _Formatter __f;
/scratchpad/gcc-build/x86_64-pc-linux-gnu/libstdc++-v3/include/bits/basic_string.tcc:609:7: warning:       |               ~~~~~~~~~~~~~~~
    4048 |               this->_M_pc.advance_to(__f.parse(this->_M_pc));
/scratchpad/gcc-build/x86_64-pc-linux-gnu/libstdc++-v3/include/bits/basic_string.tcc:609:7: warning:       |               ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
    4049 |               this->_M_fc.advance_to(__f.format(__arg, this->_M_fc));
/scratchpad/gcc-build/x86_64-pc-linux-gnu/libstdc++-v3/include/bits/basic_string.tcc:609:7: warning:       |               ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
    4050 |             }
/scratchpad/gcc-build/x86_64-pc-linux-gnu/libstdc++-v3/include/bits/basic_string.tcc:609:7: warning:       |             ~
    4051 |           else
/scratchpad/gcc-build/x86_64-pc-linux-gnu/libstdc++-v3/include/bits/basic_string.tcc:609:7: warning:       |           ~~~~
    4052 |             static_assert(__format::__formattable_with<_Type, _Context>);
/scratchpad/gcc-build/x86_64-pc-linux-gnu/libstdc++-v3/include/bits/basic_string.tcc:609:7: warning:       |             ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
    4053 |         }, _M_fc.arg(__id));
/scratchpad/gcc-build/x86_64-pc-linux-gnu/libstdc++-v3/include/bits/basic_string.tcc:609:7: warning:       |         ~~~~~~~~~~~~~~~~~~~
/scratchpad/gcc-build/x86_64-pc-linux-gnu/libstdc++-v3/include/format:4033:7:   required from here
/scratchpad/gcc-build/x86_64-pc-linux-gnu/libstdc++-v3/include/bits/basic_string.tcc:609:7: warning:  4033 |       _M_format_arg(size_t __id) override
/scratchpad/gcc-build/x86_64-pc-linux-gnu/libstdc++-v3/include/bits/basic_string.tcc:609:7: warning:       |       ^~~~~~~~~~~~~
/scratchpad/gcc-build/x86_64-pc-linux-gnu/libstdc++-v3/include/bits/basic_string.tcc:609:7: warning: comparison of integer expressions of different signedness: ‘long int’ and ‘const std::__cxx11::basic_string<char>::size_type’ {aka ‘const long unsigned int’} [-Wsign-compare]
  609 |       _GLIBCXX_DEBUG_ASSERT(__r >= 0 && __r <= __n);
      |       ^~~~~~~~~~~~~~~~~~~~~
[ ... snip ... ]

The instantiation context source code for the first warning get rendered fine,
but for subsequent warnings they contain a spurious "file:line:col: warning: "
prefix which seems to be inherited from the warning before it.

> 
> > 
> > > 
> > > 
> > > Successfully bootstrapped & regrtested on x86_64-pc-linux-gnu.
> > > 
> > > OK for trunk?
> > > 
> > > 
> > > gcc/cp/ChangeLog:
> > > 	* error.cc (print_instantiation_partial_context_line): Call
> > > 	diagnostic_show_locus.
> > > 
> > > gcc/testsuite/ChangeLog:
> > > 	* g++.dg/diagnostic/static_assert3.C: Add directives for
> > > 	additional source printing.
> > > 	* g++.dg/template/error60.C: New test.
> > > 
> > > Signed-off-by: David Malcolm <dmalcolm@redhat.com>
> > > ---
> > >  gcc/cp/error.cc                               |  2 +
> > >  .../g++.dg/diagnostic/static_assert3.C        |  7 +++-
> > >  gcc/testsuite/g++.dg/template/error60.C       | 37 +++++++++++++++++++
> > >  3 files changed, 45 insertions(+), 1 deletion(-)
> > >  create mode 100644 gcc/testsuite/g++.dg/template/error60.C
> > > 
> > > diff --git a/gcc/cp/error.cc b/gcc/cp/error.cc
> > > index ef96e140f24..767478cf5fd 100644
> > > --- a/gcc/cp/error.cc
> > > +++ b/gcc/cp/error.cc
> > > @@ -3774,6 +3774,8 @@ print_instantiation_partial_context_line (diagnostic_context *context,
> > >  		   ? _("recursively required from here\n")
> > >  		   : _("required from here\n"));
> > >      }
> > > +  gcc_rich_location rich_loc (loc);
> > > +  diagnostic_show_locus (context, &rich_loc, DK_NOTE);
> > >  }
> > >  
> > >  /* Same as print_instantiation_full_context but less verbose.  */
> > > diff --git a/gcc/testsuite/g++.dg/diagnostic/static_assert3.C b/gcc/testsuite/g++.dg/diagnostic/static_assert3.C
> > > index 5d363884508..4ec53f17120 100644
> > > --- a/gcc/testsuite/g++.dg/diagnostic/static_assert3.C
> > > +++ b/gcc/testsuite/g++.dg/diagnostic/static_assert3.C
> > > @@ -5,6 +5,11 @@
> > >  template <typename T, typename U> struct is_same { static constexpr bool value = false; };
> > >  template <typename T> struct is_same<T, T> { static constexpr bool value = true; };
> > >  
> > > +/* { dg-begin-multiline-output "" }
> > > +  f(0, 1.3);
> > > +  ~^~~~~~~~
> > > +   { dg-end-multiline-output "" } */
> > > +
> > >  template <typename T, typename U>
> > >  void f(T, U)
> > >  {
> > > @@ -32,5 +37,5 @@ void f(T, U)
> > >  
> > >  void g()
> > >  {
> > > - f(0, 1.3);
> > > + f(0, 1.3); // { dg-message " required from here" }
> > >  }
> > > diff --git a/gcc/testsuite/g++.dg/template/error60.C b/gcc/testsuite/g++.dg/template/error60.C
> > > new file mode 100644
> > > index 00000000000..8c2139b207c
> > > --- /dev/null
> > > +++ b/gcc/testsuite/g++.dg/template/error60.C
> > > @@ -0,0 +1,37 @@
> > > +// { dg-options "-fdiagnostics-show-caret" }
> > > +
> > > +template <typename Foo>
> > > +struct my_pointer
> > > +{
> > > +  my_pointer (Foo *ptr) // { dg-message " initializing argument 1" }
> > > +  : m_ptr (ptr)
> > > +  {}
> > > +
> > > +  Foo *m_ptr;
> > > +};
> > > +
> > > +template <typename Foo>
> > > +void test (Foo val)
> > > +{
> > > +  my_pointer<Foo> ptr (val); // { dg-error "invalid conversion from 'int' to 'int\\*'" }
> > > +}
> > > +
> > > +void usage ()
> > > +{
> > > +  test<int> (42); // { dg-message " required from here" }
> > > +  /* { dg-begin-multiline-output "" }
> > > +   test<int> (42);
> > > +   ~~~~~~~~~~^~~~
> > > +     { dg-end-multiline-output "" } */
> > > +}
> > > +
> > > +  /* { dg-begin-multiline-output "" }
> > > +   my_pointer (Foo *ptr)
> > > +               ~~~~~^~~
> > > +     { dg-end-multiline-output "" } */
> > > +  /* { dg-begin-multiline-output "" }
> > > +   my_pointer<Foo> ptr (val);
> > > +                        ^~~
> > > +                        |
> > > +                        int
> > > +     { dg-end-multiline-output "" } */
> > > -- 
> > > 2.26.3
> > > 
> > >