bug#25987: 25.2; support gcc fixit notes

[David Malcolm <dmalcolm@redhat.com>]

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

On Tue, 2020-10-13 at 17:37 +0300, Eli Zaretskii wrote:
> > From: David Malcolm <dmalcolm@redhat.com>
> > Cc: 25987@debbugs.gnu.org
> > Date: Mon, 12 Oct 2020 18:27:29 -0400
> > 
> > I like how -fdiagnostics-parseable-fixits adds extra lines of
> > output,
> > with a prefix that's ought to be easy to detect.
> 
> Yes, I think that's preferable, indeed.
> 
> > BTW, does Emacs set anything in the environment that GCC could
> > detect?
> 
> Yes, Emacs sets INSIDE_EMACS when it runs a sub-process.
> 
> > Does Emacs have an automated test suite that could test this
> > feature?
> 
> Yes, see the tests in the test/ subdirectory of the Emacs tree.
> 
> > Another complication to consider: the locations in a fix-it hint
> > refer
> > to the original source file, before any changes are made.  If the
> > user
> > interface supports the user e.g. clicking on fix-it hints and
> > selectively apply them one by one, then after each fix-it hint is
> > applied, all locations after that point potentially need to be
> > "fixed
> > up" somehow, to reflect the changes to the buffer.
> 
> That could be handled automatically by defining a marker at each
> hint's location, then they will move as text is edited.

Thanks Eli and Andrea.

I've implemented a proof-of-concept of this.

I'm attaching the patch for discussion/review purposes (it probably
doesn't quite apply cleanly to gcc master as I had some other
diagnostic stuff in that working copy; it also hasn't yet been
subjected to the usual testing for a gcc patch).

I'm also attaching the stderr with
  GCC_EXTRA_DIAGNOSTIC_OUTPUT=fixits-v2
in the env when using the patched gcc to compile the demo.c file from
message #82.

Hopefully the output looks consumable by Emacs.

One possible issue: in the final diagnostic, there's a fix-it hint with
non-ASCII replacement text, replacing "two_pi" with "two_π" (where the
final char in the latter is GREEK SMALL LETTER PI, U+03C0)

This replacement currently expressed as encoded bytes i.e:

fix-it:"demo.c":{51:10-51:16}:"two_\317\200"

where \317\200 is the octal-escaped representation of the two bytes of
the UTF-8 encoding of the character.

Is this going to work for Emacs?

Dave

[-- Attachment #2: demo.out --]
[-- Type: text/plain, Size: 2624 bytes --]

demo.c: In function ‘test_1’:
demo.c:14:10: warning: implicit declaration of function ‘malloc’ [-Wimplicit-function-declaration]
   14 |   return malloc (4096);
      |          ^~~~~~
demo.c:3:1: note: include ‘<stdlib.h>’ or provide a declaration of ‘malloc’
    2 | #include <stdio.h>
  +++ |+#include <stdlib.h>
    3 | 
fix-it:"demo.c":{3:1-3:1}:"#include <stdlib.h>\n"
demo.c:14:10: warning: incompatible implicit declaration of built-in function ‘malloc’ [-Wbuiltin-declaration-mismatch]
   14 |   return malloc (4096);
      |          ^~~~~~
demo.c:14:10: note: include ‘<stdlib.h>’ or provide a declaration of ‘malloc’
demo.c: In function ‘test_2a’:
demo.c:20:21: error: ‘struct vert’ has no member named ‘colour’; did you mean ‘color’?
   20 |   /* 🙂 🙂 🙂 */ v->colour = 0;
      |                     ^~~~~~
      |                     color
fix-it:"demo.c":{20:21-20:27}:"color"
demo.c: In function ‘test_2b’:
demo.c:26:18: error: ‘struct vert’ has no member named ‘colour’; did you mean ‘color’?
   26 |   /* π π π */ v->colour = 0;
      |                  ^~~~~~
      |                  color
fix-it:"demo.c":{26:18-26:24}:"color"
demo.c: In function ‘test_2c’:
demo.c:32:21: error: ‘struct vert’ has no member named ‘colour’; did you mean ‘color’?
   32 |   /* 文字化け */ v->colour = 0;
      |                     ^~~~~~
      |                     color
fix-it:"demo.c":{32:21-32:27}:"color"
demo.c: In function ‘test_3’:
demo.c:38:14: error: ‘struct vert’ has no member named ‘colour’; did you mean ‘color’?
   38 |           v->colour = 0;
      |              ^~~~~~
      |              color
fix-it:"demo.c":{38:14-38:20}:"color"
demo.c: In function ‘test_4’:
demo.c:44:10: error: ‘struct vert’ has no member named ‘colour’; did you mean ‘color’?
   44 |   if (v->colour) v->colour += 2;
      |          ^~~~~~
      |          color
fix-it:"demo.c":{44:10-44:16}:"color"
demo.c:44:21: error: ‘struct vert’ has no member named ‘colour’; did you mean ‘color’?
   44 |   if (v->colour) v->colour += 2;
      |                     ^~~~~~
      |                     color
fix-it:"demo.c":{44:21-44:27}:"color"
demo.c: In function ‘test_5’:
demo.c:51:10: error: ‘two_pi’ undeclared (first use in this function); did you mean ‘two_π’?
   51 |   return two_pi;
      |          ^~~~~~
      |          two_π
fix-it:"demo.c":{51:10-51:16}:"two_\317\200"
demo.c:51:10: note: each undeclared identifier is reported only once for each function it appears in

[-- Attachment #3: 0001-Add-GCC_EXTRA_DIAGNOSTIC_OUTPUT-environment-variable.patch --]
[-- Type: text/x-patch, Size: 15789 bytes --]

From 2e1ed6ea7ea1e524f2f5b5035327ed280fff475d Mon Sep 17 00:00:00 2001
From: David Malcolm <dmalcolm@redhat.com>
Date: Mon, 19 Oct 2020 20:19:48 -0400
Subject: [PATCH] Add GCC_EXTRA_DIAGNOSTIC_OUTPUT environment variable for
 fix-it hints

GCC has had the ability to emit fix-it hints in machine-readable form
since GCC 7 via -fdiagnostics-parseable-fixits and
-fdiagnostics-generate-patch.

The former emits additional specially-formatted lines to stderr; the
option and its format were directly taken from a pre-existing option
in clang.

Ideally this could be used by IDEs so that the user can select specific
fix-it hints and have the IDE apply them to the user's source code
(perhaps turning them into clickable elements, perhaps with an
"Apply All" option, etc).  Eclipse CDT has supported this option in
this way for a few years:
  https://bugs.eclipse.org/bugs/show_bug.cgi?id=497670

As a user of Emacs I would like Emacs to support such a feature.
https://debbugs.gnu.org/cgi/bugreport.cgi?bug=25987 tracks supporting
GCC fix-it output in Emacs.  The discussion there identifies two issues
with the existing option:

(a) columns in the output are specified as byte-offsets within the
line (for exact compatibility with the option in clang), whereas emacs
would prefer to consume them as what GCC 11 calls "display columns".
https://gcc.gnu.org/onlinedocs/gcc/Diagnostic-Message-Formatting-Options.html#index-fdiagnostics-column-unit

(b) injecting a command-line option into the build is a fiddly manual
step, varying between build systems.  It's far easier for the
user if Emacs simply sets an environment variable when compiling,
GCC uses this to enable the option if it recognizes the value, and
the emacs compilation buffer decodes the additional lines of output
and adds appropriate widgets.  In some ways it is a workaround for
not having a language server.  Doing it this way means that for the
various combinations of older and newer GCC and older and newer Emacs
that a sufficiently modern combination of both can automatically
support the rich fix-it UI, whereas other combinations will either
not provide the envvar, or silently ignore it, gracefully doing
nothing extra.

Hence this patch adds a new GCC_EXTRA_DIAGNOSTIC_OUTPUT environment
variable to GCC which enables output of machine-parseable fix-it hints.

GCC_EXTRA_DIAGNOSTIC_OUTPUT=fixits-v1 is equivalent to the existing
-fdiagnostics-parseable-fixits option.

GCC_EXTRA_DIAGNOSTIC_OUTPUT=fixits-v2 is the same, but changes the
column output mode to "display columns" rather than bytes, as
required by Emacs.

gcc/ChangeLog:
	* diagnostic.c (diagnostic_initialize): Eliminate
	parseable_fixits_p in favor of initializing extra_output_kind from
	GCC_EXTRA_DIAGNOSTIC_OUTPUT.
	(convert_column_unit): New function, split out from...
	(diagnostic_converted_column): ...this.
	(print_parseable_fixits): Add "column_unit" and "tabstop" params.
	Use them to call convert_column_unit on the column values.
	(diagnostic_report_diagnostic): Eliminate conditional on
	parseable_fixits_p in favor of a switch statement on
	extra_output_kind, passing the appropriate values to the new
	params of print_parseable_fixits.
	(selftest::test_print_parseable_fixits_none): Update for new
	params of print_parseable_fixits.
	(selftest::test_print_parseable_fixits_insert): Likewise.
	(selftest::test_print_parseable_fixits_remove): Likewise.
	(selftest::test_print_parseable_fixits_replace): Likewise.
	(selftest::test_print_parseable_fixits_bytes_vs_display_columns):
	New.
	(selftest::diagnostic_c_tests): Call it.
	* diagnostic.h (enum diagnostics_extra_output_kind): New.
	(diagnostic_context::parseable_fixits_p): Delete field in favor
	of...
	(diagnostic_context::extra_output_kind): ...this new field.
	* doc/invoke.texi (Environment Variables): Add
	GCC_EXTRA_DIAGNOSTIC_OUTPUT.
	* opts.c (common_handle_option): Update handling of
	OPT_fdiagnostics_parseable_fixits for change to diagnostic_context
	fields.
---
 gcc/diagnostic.c    | 138 ++++++++++++++++++++++++++++++++++++--------
 gcc/diagnostic.h    |  23 +++++++-
 gcc/doc/invoke.texi |  19 ++++++
 gcc/opts.c          |   4 +-
 4 files changed, 155 insertions(+), 29 deletions(-)

diff --git a/gcc/diagnostic.c b/gcc/diagnostic.c
index 1b6c9845892..7724f0e050b 100644
--- a/gcc/diagnostic.c
+++ b/gcc/diagnostic.c
@@ -219,7 +219,14 @@ diagnostic_initialize (diagnostic_context *context, int n_opts)
   context->show_line_numbers_p = false;
   context->min_margin_width = 0;
   context->show_ruler_p = false;
-  context->parseable_fixits_p = false;
+  if (const char *var = getenv ("GCC_EXTRA_DIAGNOSTIC_OUTPUT"))
+    {
+      if (!strcmp (var, "fixits-v1"))
+	context->extra_output_kind = EXTRA_DIAGNOSTIC_OUTPUT_fixits_v1;
+      else if (!strcmp (var, "fixits-v2"))
+	context->extra_output_kind = EXTRA_DIAGNOSTIC_OUTPUT_fixits_v2;
+      /* Silently ignore unrecognized values.  */
+    }
   context->column_unit = DIAGNOSTICS_COLUMN_UNIT_DISPLAY;
   context->column_origin = 1;
   context->tabstop = 8;
@@ -358,29 +365,40 @@ diagnostic_get_color_for_kind (diagnostic_t kind)
 }
 
 /* Given an expanded_location, convert the column (which is in 1-based bytes)
-   to the requested units and origin.  Return -1 if the column is
-   invalid (<= 0).  */
-int
-diagnostic_converted_column (diagnostic_context *context, expanded_location s)
+   to the requested units, without converting the origin.
+   Return -1 if the column is invalid (<= 0).  */
+
+static int
+convert_column_unit (enum diagnostics_column_unit column_unit,
+		     int tabstop,
+		     expanded_location s)
 {
   if (s.column <= 0)
     return -1;
 
-  int one_based_col;
-  switch (context->column_unit)
+  switch (column_unit)
     {
+    default:
+      gcc_unreachable ();
+
     case DIAGNOSTICS_COLUMN_UNIT_DISPLAY:
-      one_based_col = location_compute_display_column (s, context->tabstop);
-      break;
+      return location_compute_display_column (s, tabstop);
 
     case DIAGNOSTICS_COLUMN_UNIT_BYTE:
-      one_based_col = s.column;
-      break;
-
-    default:
-      gcc_unreachable ();
+      return s.column;
     }
+}
 
+/* Given an expanded_location, convert the column (which is in 1-based bytes)
+   to the requested units and origin.  Return -1 if the column is
+   invalid (<= 0).  */
+int
+diagnostic_converted_column (diagnostic_context *context, expanded_location s)
+{
+  int one_based_col
+    = convert_column_unit (context->column_unit, context->tabstop, s);
+  if (one_based_col <= 0)
+    return -1;
   return one_based_col + (context->column_origin - 1);
 }
 
@@ -920,11 +938,16 @@ print_escaped_string (pretty_printer *pp, const char *text)
   pp_character (pp, '"');
 }
 
-/* Implementation of -fdiagnostics-parseable-fixits.  Print a
-   machine-parseable version of all fixits in RICHLOC to PP.  */
+/* Implementation of -fdiagnostics-parseable-fixits and
+   GCC_EXTRA_DIAGNOSTIC_OUTPUT.
+   Print a machine-parseable version of all fixits in RICHLOC to PP,
+   using COLUMN_UNIT to express columns.
+   Use TABSTOP when handling DIAGNOSTICS_COLUMN_UNIT_DISPLAY.  */
 
 static void
-print_parseable_fixits (pretty_printer *pp, rich_location *richloc)
+print_parseable_fixits (pretty_printer *pp, rich_location *richloc,
+			enum diagnostics_column_unit column_unit,
+			int tabstop)
 {
   gcc_assert (pp);
   gcc_assert (richloc);
@@ -942,9 +965,13 @@ print_parseable_fixits (pretty_printer *pp, rich_location *richloc)
       /* For compatibility with clang, print as a half-open range.  */
       location_t next_loc = hint->get_next_loc ();
       expanded_location next_exploc = expand_location (next_loc);
+      int start_col
+	= convert_column_unit (column_unit, tabstop, start_exploc);
+      int next_col
+	= convert_column_unit (column_unit, tabstop, next_exploc);
       pp_printf (pp, ":{%i:%i-%i:%i}:",
-		 start_exploc.line, start_exploc.column,
-		 next_exploc.line, next_exploc.column);
+		 start_exploc.line, start_col,
+		 next_exploc.line, next_col);
       print_escaped_string (pp, hint->get_string ());
       pp_newline (pp);
     }
@@ -1210,10 +1237,22 @@ diagnostic_report_diagnostic (diagnostic_context *context,
   if (context->show_option_requested)
     print_option_information (context, diagnostic, orig_diag_kind);
   (*diagnostic_finalizer (context)) (context, diagnostic, orig_diag_kind);
-  if (context->parseable_fixits_p)
+  switch (context->extra_output_kind)
     {
-      print_parseable_fixits (context->printer, diagnostic->richloc);
+    default:
+      break;
+    case EXTRA_DIAGNOSTIC_OUTPUT_fixits_v1:
+      print_parseable_fixits (context->printer, diagnostic->richloc,
+			      DIAGNOSTICS_COLUMN_UNIT_BYTE,
+			      context->tabstop);
+      pp_flush (context->printer);
+      break;
+    case EXTRA_DIAGNOSTIC_OUTPUT_fixits_v2:
+      print_parseable_fixits (context->printer, diagnostic->richloc,
+			      DIAGNOSTICS_COLUMN_UNIT_DISPLAY,
+			      context->tabstop);
       pp_flush (context->printer);
+      break;
     }
   diagnostic_action_after_output (context, diagnostic->kind);
   diagnostic->x_data = NULL;
@@ -2012,7 +2051,7 @@ test_print_parseable_fixits_none ()
   pretty_printer pp;
   rich_location richloc (line_table, UNKNOWN_LOCATION);
 
-  print_parseable_fixits (&pp, &richloc);
+  print_parseable_fixits (&pp, &richloc, DIAGNOSTICS_COLUMN_UNIT_BYTE, 8);
   ASSERT_STREQ ("", pp_formatted_text (&pp));
 }
 
@@ -2031,7 +2070,7 @@ test_print_parseable_fixits_insert ()
   location_t where = linemap_position_for_column (line_table, 10);
   richloc.add_fixit_insert_before (where, "added content");
 
-  print_parseable_fixits (&pp, &richloc);
+  print_parseable_fixits (&pp, &richloc, DIAGNOSTICS_COLUMN_UNIT_BYTE, 8);
   ASSERT_STREQ ("fix-it:\"test.c\":{5:10-5:10}:\"added content\"\n",
 		pp_formatted_text (&pp));
 }
@@ -2053,7 +2092,7 @@ test_print_parseable_fixits_remove ()
   where.m_finish = linemap_position_for_column (line_table, 20);
   richloc.add_fixit_remove (where);
 
-  print_parseable_fixits (&pp, &richloc);
+  print_parseable_fixits (&pp, &richloc, DIAGNOSTICS_COLUMN_UNIT_BYTE, 8);
   ASSERT_STREQ ("fix-it:\"test.c\":{5:10-5:21}:\"\"\n",
 		pp_formatted_text (&pp));
 }
@@ -2075,11 +2114,59 @@ test_print_parseable_fixits_replace ()
   where.m_finish = linemap_position_for_column (line_table, 20);
   richloc.add_fixit_replace (where, "replacement");
 
-  print_parseable_fixits (&pp, &richloc);
+  print_parseable_fixits (&pp, &richloc, DIAGNOSTICS_COLUMN_UNIT_BYTE, 8);
   ASSERT_STREQ ("fix-it:\"test.c\":{5:10-5:21}:\"replacement\"\n",
 		pp_formatted_text (&pp));
 }
 
+/* Verify that print_parseable_fixits correctly handles
+   DIAGNOSTICS_COLUMN_UNIT_BYTE vs DIAGNOSTICS_COLUMN_UNIT_COLUMN.  */
+
+static void
+test_print_parseable_fixits_bytes_vs_display_columns ()
+{
+  line_table_test ltt;
+  rich_location richloc (line_table, UNKNOWN_LOCATION);
+
+  /* 1-based byte offsets:     12345677778888999900001234567.  */
+  const char *const content = "smile \xf0\x9f\x98\x82 colour\n";
+  /* 1-based display cols:     123456[......7-8.....]9012345.  */
+  const int tabstop = 8;
+
+  temp_source_file tmp (SELFTEST_LOCATION, ".c", content);
+  const char *const fname = tmp.get_filename ();
+
+  linemap_add (line_table, LC_ENTER, false, fname, 0);
+  linemap_line_start (line_table, 1, 100);
+  linemap_add (line_table, LC_LEAVE, false, NULL, 0);
+  source_range where;
+  where.m_start = linemap_position_for_column (line_table, 12);
+  where.m_finish = linemap_position_for_column (line_table, 17);
+  richloc.add_fixit_replace (where, "color");
+
+  const int buf_len = strlen (fname) + 100;
+  char *const expected = XNEWVEC (char, buf_len);
+
+  {
+    pretty_printer pp;
+    print_parseable_fixits (&pp, &richloc, DIAGNOSTICS_COLUMN_UNIT_BYTE,
+			    tabstop);
+    snprintf (expected, buf_len,
+	      "fix-it:\"%s\":{1:12-1:18}:\"color\"\n", fname);
+    ASSERT_STREQ (expected, pp_formatted_text (&pp));
+  }
+  {
+    pretty_printer pp;
+    print_parseable_fixits (&pp, &richloc, DIAGNOSTICS_COLUMN_UNIT_DISPLAY,
+			    tabstop);
+    snprintf (expected, buf_len,
+	      "fix-it:\"%s\":{1:10-1:16}:\"color\"\n", fname);
+    ASSERT_STREQ (expected, pp_formatted_text (&pp));
+  }
+
+  XDELETEVEC (expected);
+}
+
 /* Verify that
      diagnostic_get_location_text (..., SHOW_COLUMN)
    generates EXPECTED_LOC_TEXT, given FILENAME, LINE, COLUMN, with
@@ -2202,6 +2289,7 @@ diagnostic_c_tests ()
   test_print_parseable_fixits_insert ();
   test_print_parseable_fixits_remove ();
   test_print_parseable_fixits_replace ();
+  test_print_parseable_fixits_bytes_vs_display_columns ();
   test_diagnostic_get_location_text ();
   test_num_digits ();
 
diff --git a/gcc/diagnostic.h b/gcc/diagnostic.h
index 1d2fa119643..e88d63c2128 100644
--- a/gcc/diagnostic.h
+++ b/gcc/diagnostic.h
@@ -69,6 +69,22 @@ enum diagnostic_path_format
   DPF_HTML
 };
 
+/* An enum for capturing values of GCC_EXTRA_DIAGNOSTIC_OUTPUT,
+   and for -fdiagnostics-parseable-fixits.  */
+
+enum diagnostics_extra_output_kind
+{
+  /* No extra output, or an unrecognized value.  */
+  EXTRA_DIAGNOSTIC_OUTPUT_none,
+
+  /* Emit fix-it hints using the "fixits-v1" format, equivalent to
+     -fdiagnostics-parseable-fixits.  */
+  EXTRA_DIAGNOSTIC_OUTPUT_fixits_v1,
+
+  /* Emit fix-it hints using the "fixits-v2" format.  */
+  EXTRA_DIAGNOSTIC_OUTPUT_fixits_v2
+};
+
 /* A diagnostic is described by the MESSAGE to send, the FILE and LINE of
    its context and its KIND (ice, error, warning, note, ...)  See complete
    list in diagnostic.def.  */
@@ -296,9 +312,10 @@ struct diagnostic_context
      source output.  */
   bool show_ruler_p;
 
-  /* If true, print fixits in machine-parseable form after the
-     rest of the diagnostic.  */
-  bool parseable_fixits_p;
+  /* Used to specify additional diagnostic output to be emitted after the
+     rest of the diagnostic.  This is for implementing
+     -fdiagnostics-parseable-fixits and GCC_EXTRA_DIAGNOSTIC_OUTPUT.  */
+  enum diagnostics_extra_output_kind extra_output_kind;
 
   /* What units to use when outputting the column number.  */
   enum diagnostics_column_unit column_unit;
diff --git a/gcc/doc/invoke.texi b/gcc/doc/invoke.texi
index 55f7a9854aa..f83fe8f735c 100644
--- a/gcc/doc/invoke.texi
+++ b/gcc/doc/invoke.texi
@@ -31998,6 +31998,25 @@ Recognize EUCJP characters.
 If @env{LANG} is not defined, or if it has some other value, then the
 compiler uses @code{mblen} and @code{mbtowc} as defined by the default locale to
 recognize and translate multibyte characters.
+
+@item GCC_EXTRA_DIAGNOSTIC_OUTPUT
+If @env{GCC_EXTRA_DIAGNOSTIC_OUTPUT} is set to one of the following values,
+then additional text will be emitted to stderr when fix-it hints are
+emitted.  @option{-fdiagnostics-parseable-fixits} and
+@option{-fno-diagnostics-parseable-fixits} take precedence over this
+environment variable.
+
+@table @samp
+@item fixits-v1
+Emit parseable fix-it hints, equivalent to
+@option{-fdiagnostics-parseable-fixits}.  In particular, columns are
+expressed as a count of bytes, starting at byte 1 for the initial column.
+
+@item fixits-v2
+As @code{fixits-v1}, but columns are expressed as display columns,
+as per @option{-fdiagnostics-column-unit=display}.
+@end table
+
 @end table
 
 @noindent
diff --git a/gcc/opts.c b/gcc/opts.c
index da503c32dd0..b776f3b31e1 100644
--- a/gcc/opts.c
+++ b/gcc/opts.c
@@ -2422,7 +2422,9 @@ common_handle_option (struct gcc_options *opts,
       break;
 
     case OPT_fdiagnostics_parseable_fixits:
-      dc->parseable_fixits_p = value;
+      dc->extra_output_kind = (value
+			       ? EXTRA_DIAGNOSTIC_OUTPUT_fixits_v1
+			       : EXTRA_DIAGNOSTIC_OUTPUT_none);
       break;
 
     case OPT_fdiagnostics_column_unit_:
-- 
2.26.2