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

[Patrick Palka <ppalka@redhat.com>]

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

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?

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