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

[Patrick Palka <ppalka@redhat.com>]

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

On Wed, 24 Apr 2024, Patrick Palka wrote:

> 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.

I'm testing the following which seems to fix it:

diff --git a/gcc/cp/error.cc b/gcc/cp/error.cc
index 7074845154e..f38a0d9906a 100644
--- a/gcc/cp/error.cc
+++ b/gcc/cp/error.cc
@@ -3793,7 +3793,10 @@ print_instantiation_partial_context_line (diagnostic_context *context,
 		   : _("required from here\n"));
     }
   gcc_rich_location rich_loc (loc);
+  char *saved_prefix = pp_take_prefix (context->printer);
+  pp_set_prefix (context->printer, NULL);
   diagnostic_show_locus (context, &rich_loc, DK_NOTE);
+  pp_set_prefix (context->printer, saved_prefix);
 }
 
 /* Same as print_instantiation_full_context but less verbose.  */

> 
> > 
> > > 
> > > > 
> > > > 
> > > > 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
> > > > 
> > > >