Add validation and consolidation of fix-it hints

The first aspect of this patch is to add some checking of fix-it hints.
The idea is to put this checking within the rich_location machinery,
rather than requiring every diagnostic to implement it for itself.

The fixits within a rich_location are "atomic": all must be valid for
any to be applicable.

We reject any fixits involving locations above
LINE_MAP_MAX_LOCATION_WITH_COLS.

There's no guarantee that it's sane to modify a macro, so we reject
any fix-its that touch them.

For example, note the attempt to provide a fix-it for the definition
of the macro FIELD:

spellcheck-fields-2.c: In function ‘test_macro’:
spellcheck-fields-2.c:26:15: error: ‘union u’ has no member named ‘colour’; did you mean ‘color’?
 #define FIELD colour
               ^
               color
spellcheck-fields-2.c:27:15: note: in expansion of macro ‘FIELD’
   return ptr->FIELD;
               ^~~~~

After this patch, the fixit is not displayed:

spellcheck-fields-2.c: In function ‘test_macro’:
spellcheck-fields-2.c:26:15: error: ‘union u’ has no member named ‘colour’; did you mean ‘color’?
 #define FIELD colour
               ^
spellcheck-fields-2.c:27:15: note: in expansion of macro ‘FIELD’
   return ptr->FIELD;
               ^~~~~

We might want some way for a diagnostic to opt-in to fix-its that
affect macros, but for now it's simplest to reject them.

The other aspect of this patch is fix-it consolidation: in some cases
neighboring fix-its can be merged.  For example, in a diagnostic to
modernize old-style struct initializers from:

 struct s example = {
- foo: 1,
+ .foo = 1,
 };

one approach would be to replace the "foo" with ".foo" and the ":"
with " =".  This would give two "replace" fix-its:

  foo: 1,
  --- FIXIT 1
  .foo
     - FIXIT 2
     =

This patch allows them to be consolidated into a single "replace" fix-it:

  foo: 1,
  ----
  .foo =

gcc/ChangeLog:
	* diagnostic-show-locus.c
	(selftest::test_fixit_consolidation): New function.
	(selftest::diagnostic_show_locus_c_tests): Call it.
	* gcc-rich-location.h (gcc_rich_location): Eliminate unused
	constructor based on source_range.

gcc/testsuite/ChangeLog:
	* gcc.dg/spellcheck-fields-2.c (test): Move
	dg-begin/end-multiline-output within function body.
	(test_macro): New function.

libcpp/ChangeLog:
	* include/line-map.h (rich_location): Eliminate unimplemented
	constructor based on source_range.
	(rich_location::get_last_fixit_hint): New method.
	(rich_location::reject_impossible_fixit): New method.
	(rich_location): Add fields m_line_table and
	m_seen_impossible_fixit.
	(fixit_hint::maybe_append_replace): New pure virtual function.
	(fixit_insert::maybe_append_replace): New function.
	(fixit_replace::maybe_append_replace): New function.
	* line-map.c (rich_location::rich_location): Initialize
	m_line_table and m_seen_impossible_fixit.
	(rich_location::add_fixit_insert): Call
	reject_impossible_fixit and bail out if true.
	(column_before_p): New function.
	(rich_location::add_fixit_replace): Call reject_impossible_fixit
	and bail out if true.  Attempt to consolidate with neighboring
	fixits.
	(rich_location::get_last_fixit_hint): New method.
	(rich_location::reject_impossible_fixit): New method.
	(fixit_insert::maybe_append_replace): New method.
	(fixit_replace::maybe_append_replace): New method.

From-SVN: r239789

5 files changed, 191 insertions, 6 deletions

diff --git a/gcc/ChangeLog b/gcc/ChangeLog
index e719a87..cfa9c3d 100644
--- a/gcc/ChangeLog
+++ b/gcc/ChangeLog
@@ -1,5 +1,13 @@
 2016-08-26  David Malcolm  <dmalcolm@redhat.com>
 
+	* diagnostic-show-locus.c
+	(selftest::test_fixit_consolidation): New function.
+	(selftest::diagnostic_show_locus_c_tests): Call it.
+	* gcc-rich-location.h (gcc_rich_location): Eliminate unused
+	constructor based on source_range.
+
+2016-08-26  David Malcolm  <dmalcolm@redhat.com>
+
 	* diagnostic-color.c (color_dict): Add "fixit-insert" and
 	"fixit-delete".
 	(parse_gcc_colors): Update description of default GCC_COLORS.
diff --git a/gcc/diagnostic-show-locus.c b/gcc/diagnostic-show-locus.c
index 94b7349..f3f661e 100644
--- a/gcc/diagnostic-show-locus.c
+++ b/gcc/diagnostic-show-locus.c
@@ -1628,6 +1628,160 @@ test_diagnostic_show_locus_one_liner (const line_table_case &case_)
   test_one_liner_fixit_replace_equal_secondary_range ();
 }
 
+/* Verify that fix-it hints are appropriately consolidated.
+
+   If any fix-it hints in a rich_location involve locations beyond
+   LINE_MAP_MAX_LOCATION_WITH_COLS, then we can't reliably apply
+   the fix-it as a whole, so there should be none.
+
+   Otherwise, verify that consecutive "replace" and "remove" fix-its
+   are merged, and that other fix-its remain separate.   */
+
+static void
+test_fixit_consolidation (const line_table_case &case_)
+{
+  line_table_test ltt (case_);
+
+  linemap_add (line_table, LC_ENTER, false, "test.c", 1);
+
+  const location_t c10 = linemap_position_for_column (line_table, 10);
+  const location_t c15 = linemap_position_for_column (line_table, 15);
+  const location_t c16 = linemap_position_for_column (line_table, 16);
+  const location_t c17 = linemap_position_for_column (line_table, 17);
+  const location_t c20 = linemap_position_for_column (line_table, 20);
+  const location_t caret = c10;
+
+  /* Insert + insert. */
+  {
+    rich_location richloc (line_table, caret);
+    richloc.add_fixit_insert (c10, "foo");
+    richloc.add_fixit_insert (c15, "bar");
+
+    if (c15 > LINE_MAP_MAX_LOCATION_WITH_COLS)
+      /* Bogus column info for 2nd fixit, so no fixits.  */
+      ASSERT_EQ (0, richloc.get_num_fixit_hints ());
+    else
+      /* They should not have been merged.  */
+      ASSERT_EQ (2, richloc.get_num_fixit_hints ());
+  }
+
+  /* Insert + replace. */
+  {
+    rich_location richloc (line_table, caret);
+    richloc.add_fixit_insert (c10, "foo");
+    richloc.add_fixit_replace (source_range::from_locations (c15, c17),
+			       "bar");
+
+    if (c17 > LINE_MAP_MAX_LOCATION_WITH_COLS)
+      /* Bogus column info for 2nd fixit, so no fixits.  */
+      ASSERT_EQ (0, richloc.get_num_fixit_hints ());
+    else
+      /* They should not have been merged.  */
+      ASSERT_EQ (2, richloc.get_num_fixit_hints ());
+  }
+
+  /* Replace + non-consecutive insert. */
+  {
+    rich_location richloc (line_table, caret);
+    richloc.add_fixit_replace (source_range::from_locations (c10, c15),
+			       "bar");
+    richloc.add_fixit_insert (c17, "foo");
+
+    if (c17 > LINE_MAP_MAX_LOCATION_WITH_COLS)
+      /* Bogus column info for 2nd fixit, so no fixits.  */
+      ASSERT_EQ (0, richloc.get_num_fixit_hints ());
+    else
+      /* They should not have been merged.  */
+      ASSERT_EQ (2, richloc.get_num_fixit_hints ());
+  }
+
+  /* Replace + non-consecutive replace. */
+  {
+    rich_location richloc (line_table, caret);
+    richloc.add_fixit_replace (source_range::from_locations (c10, c15),
+			       "foo");
+    richloc.add_fixit_replace (source_range::from_locations (c17, c20),
+			       "bar");
+
+    if (c20 > LINE_MAP_MAX_LOCATION_WITH_COLS)
+      /* Bogus column info for 2nd fixit, so no fixits.  */
+      ASSERT_EQ (0, richloc.get_num_fixit_hints ());
+    else
+      /* They should not have been merged.  */
+      ASSERT_EQ (2, richloc.get_num_fixit_hints ());
+  }
+
+  /* Replace + consecutive replace. */
+  {
+    rich_location richloc (line_table, caret);
+    richloc.add_fixit_replace (source_range::from_locations (c10, c15),
+			       "foo");
+    richloc.add_fixit_replace (source_range::from_locations (c16, c20),
+			       "bar");
+
+    if (c20 > LINE_MAP_MAX_LOCATION_WITH_COLS)
+      /* Bogus column info for 2nd fixit, so no fixits.  */
+      ASSERT_EQ (0, richloc.get_num_fixit_hints ());
+    else
+      {
+	/* They should have been merged into a single "replace".  */
+	ASSERT_EQ (1, richloc.get_num_fixit_hints ());
+	const fixit_hint *hint = richloc.get_fixit_hint (0);
+	ASSERT_EQ (fixit_hint::REPLACE, hint->get_kind ());
+	const fixit_replace *replace = (const fixit_replace *)hint;
+	ASSERT_STREQ ("foobar", replace->get_string ());
+	ASSERT_EQ (c10, replace->get_range ().m_start);
+	ASSERT_EQ (c20, replace->get_range ().m_finish);
+      }
+  }
+
+  /* Replace + consecutive removal. */
+  {
+    rich_location richloc (line_table, caret);
+    richloc.add_fixit_replace (source_range::from_locations (c10, c15),
+			       "foo");
+    richloc.add_fixit_remove (source_range::from_locations (c16, c20));
+
+    if (c20 > LINE_MAP_MAX_LOCATION_WITH_COLS)
+      /* Bogus column info for 2nd fixit, so no fixits.  */
+      ASSERT_EQ (0, richloc.get_num_fixit_hints ());
+    else
+      {
+	/* They should have been merged into a single replace, with the
+	   range extended to cover that of the removal.  */
+	ASSERT_EQ (1, richloc.get_num_fixit_hints ());
+	const fixit_hint *hint = richloc.get_fixit_hint (0);
+	ASSERT_EQ (fixit_hint::REPLACE, hint->get_kind ());
+	const fixit_replace *replace = (const fixit_replace *)hint;
+	ASSERT_STREQ ("foo", replace->get_string ());
+	ASSERT_EQ (c10, replace->get_range ().m_start);
+	ASSERT_EQ (c20, replace->get_range ().m_finish);
+      }
+  }
+
+  /* Consecutive removals. */
+  {
+    rich_location richloc (line_table, caret);
+    richloc.add_fixit_remove (source_range::from_locations (c10, c15));
+    richloc.add_fixit_remove (source_range::from_locations (c16, c20));
+
+    if (c20 > LINE_MAP_MAX_LOCATION_WITH_COLS)
+      /* Bogus column info for 2nd fixit, so no fixits.  */
+      ASSERT_EQ (0, richloc.get_num_fixit_hints ());
+    else
+      {
+	/* They should have been merged into a single "replace-with-empty".  */
+	ASSERT_EQ (1, richloc.get_num_fixit_hints ());
+	const fixit_hint *hint = richloc.get_fixit_hint (0);
+	ASSERT_EQ (fixit_hint::REPLACE, hint->get_kind ());
+	const fixit_replace *replace = (const fixit_replace *)hint;
+	ASSERT_STREQ ("", replace->get_string ());
+	ASSERT_EQ (c10, replace->get_range ().m_start);
+	ASSERT_EQ (c20, replace->get_range ().m_finish);
+      }
+  }
+}
+
 /* Run all of the selftests within this file.  */
 
 void
@@ -1642,6 +1796,7 @@ diagnostic_show_locus_c_tests ()
   test_diagnostic_show_locus_unknown_location ();
 
   for_each_line_table_case (test_diagnostic_show_locus_one_liner);
+  for_each_line_table_case (test_fixit_consolidation);
 }
 
 } // namespace selftest
diff --git a/gcc/gcc-rich-location.h b/gcc/gcc-rich-location.h
index aa69b2e..cc5987f 100644
--- a/gcc/gcc-rich-location.h
+++ b/gcc/gcc-rich-location.h
@@ -31,11 +31,6 @@ class gcc_rich_location : public rich_location
   gcc_rich_location (source_location loc) :
     rich_location (line_table, loc) {}
 
-  /* Constructing from a source_range.  */
-  gcc_rich_location (source_range src_range) :
-    rich_location (src_range) {}
-
-
   /* Methods for adding ranges via gcc entities.  */
   void
   add_expr (tree expr);
diff --git a/gcc/testsuite/ChangeLog b/gcc/testsuite/ChangeLog
index 352769a..952380f 100644
--- a/gcc/testsuite/ChangeLog
+++ b/gcc/testsuite/ChangeLog
@@ -1,5 +1,11 @@
 2016-08-26  David Malcolm  <dmalcolm@redhat.com>
 
+	* gcc.dg/spellcheck-fields-2.c (test): Move
+	dg-begin/end-multiline-output within function body.
+	(test_macro): New function.
+
+2016-08-26  David Malcolm  <dmalcolm@redhat.com>
+
 	* gcc.dg/plugin/diagnostic-test-show-locus-color.c
 	(test_fixit_insert): Update expected output.
 	(test_fixit_remove): Likewise.
diff --git a/gcc/testsuite/gcc.dg/spellcheck-fields-2.c b/gcc/testsuite/gcc.dg/spellcheck-fields-2.c
index d6ebff1..7c54214 100644
--- a/gcc/testsuite/gcc.dg/spellcheck-fields-2.c
+++ b/gcc/testsuite/gcc.dg/spellcheck-fields-2.c
@@ -9,7 +9,6 @@ union u
 int test (union u *ptr)
 {
   return ptr->colour; /* { dg-error "did you mean .color.?" } */
-}
 
 /* Verify that we get an underline and a fixit hint.  */
 /* { dg-begin-multiline-output "" }
@@ -17,3 +16,25 @@ int test (union u *ptr)
                ^~~~~~
                color
    { dg-end-multiline-output "" } */
+}
+
+
+/* Verify that we don't offer a fixit hint in the presence of
+   a macro.  */
+int test_macro (union u *ptr)
+{
+#define FIELD colour /* { dg-error "did you mean .color.?" } */
+  return ptr->FIELD;
+
+/* { dg-begin-multiline-output "" }
+ #define FIELD colour
+               ^
+   { dg-end-multiline-output "" } */
+
+/* { dg-begin-multiline-output "" }
+   return ptr->FIELD;
+               ^~~~~
+   { dg-end-multiline-output "" } */
+
+#undef FIELD
+}