Re: [PATCH] C/C++: add fix-it hints for various missing symbols

[David Malcolm <dmalcolm at redhat dot com>]

On Mon, 2017-07-03 at 19:57 +0100, Richard Sandiford wrote:
> [Thanks for all your diagnostic work btw.]
> 
> David Malcolm <dmalcolm@redhat.com> writes:
> > clang can also print notes about matching opening symbols
> > e.g. the note here:
> > 
> >   missing-symbol-2.c:25:22: error: expected ']'
> >     const char test [42;
> >                        ^
> >   missing-symbol-2.c:25:19: note: to match this '['
> >     const char test [42;
> >                     ^
> > which, although somewhat redundant for this example, seems much
> > more
> > useful if there's non-trivial nesting of constructs, or more than a
> > few
> > lines separating the open/close symbols (e.g. showing a stray
> > "namespace {"
> > that the user forgot to close).
> > 
> > I'd like to implement both of these ideas as followups, but in
> > the meantime, is the fix-it hint patch OK for trunk?
> > (successfully bootstrapped & regrtested on x86_64-pc-linux-gnu)
> 
> Just wondering: how easy would it be to restrict the note to the
> kinds
> of cases you mention?  TBH I think clang goes in for extra notes too
> much, and it's not always that case that an "expected 'foo'" message
> really is caused by a missing 'foo'.  It'd be great if there was some
> way of making the notes a bit more discerning. :-)

My plan was to only do it for open/close punctuation, i.e.:
  * '(' and ')'
  * '{' and '}'
  * '[' and ']'
  * maybe '<' and '>' in C++

> Or maybe do something like restrict the extra note to cases in which
> the
> opening character is on a different line and use an underlined range
> when the opening character is on the same line?

Good idea: if it's on the same line, use a secondary range; if it's on
a different line, use a note.

The above example would look something like this (with the '[' as a
secondary range):

  missing-symbol-2.c:25:22: error: expected ']'
  const char test [42;
                  ~  ^
                     ]

which is more compact than the "separate note" approach, whilst (IMHO)
being just as readable.

FWIW diagnostic-show-locus.c can handle widely-separated secondary
ranges within one rich_location, provided they're in the same source
file (see calculate_line_spans, and the start_span callback within
diagnostic_context).

Consider the unclosed namespace here:

$ cat -n test.cc
     1	namespace ns {
     2	
     3	void test ()
     4	{
     5	}

for which we currently emit the rather unhelpful:

$ gcc test.cc
test.cc:5:1: error: expected ‘}’ at end of input
 }
 ^

Printing it via a secondary range using a single rich_location with
just an "error_at_rich_loc" call would print something like:

test.cc:5:1: error: expected ‘}’ at end of input
test.cc:1:14:
 namespace ns {
              ^
test.cc:5:1:
 }
  ^
  }

which works, but I'm not a fan of.

In constrast, with the "if it's on a different line, use a note" approach, we would print:

test.cc:5:1: error: expected ‘}’ at end of input
 }
  ^
  }
test.cc:1:14: note: to match this '{'
 namespace ns {
              ^

which I think is better (and supports the cases where they're in different files (e.g. you have a stray unclosed namespace in a header file, somewhere...), or macros are involved, etc)

So I'll have a go at implementing the "is it on a different line" logic you suggest.

For reference, clang prints the following for the above case:

test.cc:5:2: error: expected '}'
}
 ^
test.cc:1:14: note: to match this '{'
namespace ns {
             ^

Thinking aloud, maybe it would be better for the fix-it hint to suggest putting the '}' on a whole new line.  Might even be good to suggest adding

} // namespace ns

or similar (for this specific case), giving this output:

test.cc:5:1: error: expected ‘}’ at end of input
 }
+} // namespace ns
test.cc:1:14: note: to match this '{'
 namespace ns {
              ^

(only works if the proposed insertion point is on the end of a line, given the current restrictions on what our fix-it machinery is capable of - we don't currently support splitting a pre-existing line via a fix-it hint)

Thanks.
Dave