From 7ac919c9ae25ce19e8c06ecfe66f0215d617dd78 Mon Sep 17 00:00:00 2001
From: David Malcolm <dmalcolm@redhat.com>
Date: Wed, 8 Jun 2016 14:13:17 -0400
Subject: [PATCH 05/25] FIXME: add fixit validation and consolidation
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Before, 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;
               ^~~~~

There's no guarantee that this can be applied, so after:

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;
               ^~~~~

FIXME: would it be better to display it, but do the filtering
elsewhere?
---
 gcc/diagnostic-show-locus.c                | 155 +++++++++++++++++++++++++++++
 gcc/gcc-rich-location.h                    |   5 -
 gcc/testsuite/gcc.dg/spellcheck-fields-2.c |  23 ++++-
 libcpp/include/line-map.h                  |  29 +++++-
 libcpp/line-map.c                          | 132 +++++++++++++++++++++++-
 5 files changed, 333 insertions(+), 11 deletions(-)

diff --git a/gcc/diagnostic-show-locus.c b/gcc/diagnostic-show-locus.c
index 32b1078..94b3de5 100644
--- a/gcc/diagnostic-show-locus.c
+++ b/gcc/diagnostic-show-locus.c
@@ -1618,6 +1618,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
@@ -1632,6 +1786,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/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
+}
diff --git a/libcpp/include/line-map.h b/libcpp/include/line-map.h
index f65931c..0fc4848 100644
--- a/libcpp/include/line-map.h
+++ b/libcpp/include/line-map.h
@@ -296,6 +296,16 @@ struct GTY(()) source_range
     return result;
   }
 
+  /* Make a source_range from a pair of source_location.  */
+  static source_range from_locations (source_location start,
+				      source_location finish)
+  {
+    source_range result;
+    result.m_start = start;
+    result.m_finish = finish;
+    return result;
+  }
+
   /* Is there any part of this range on the given line?  */
   bool intersects_line_p (const char *file, int line) const;
 };
@@ -1357,9 +1367,6 @@ class rich_location
   /* Constructing from a location.  */
   rich_location (line_maps *set, source_location loc);
 
-  /* Constructing from a source_range.  */
-  rich_location (source_range src_range);
-
   /* Destructor.  */
   ~rich_location ();
 
@@ -1401,12 +1408,17 @@ class rich_location
 
   unsigned int get_num_fixit_hints () const { return m_num_fixit_hints; }
   fixit_hint *get_fixit_hint (int idx) const { return m_fixit_hints[idx]; }
+  fixit_hint *get_last_fixit_hint () const;
+
+private:
+  bool reject_impossible_fixit (source_location where);
 
 public:
   static const int MAX_RANGES = 3;
   static const int MAX_FIXIT_HINTS = 2;
 
 protected:
+  line_maps *m_line_table;
   unsigned int m_num_ranges;
   location_range m_ranges[MAX_RANGES];
 
@@ -1417,6 +1429,7 @@ protected:
 
   unsigned int m_num_fixit_hints;
   fixit_hint *m_fixit_hints[MAX_FIXIT_HINTS];
+  bool m_seen_impossible_fixit;
 };
 
 class fixit_hint
@@ -1430,6 +1443,10 @@ public:
   virtual bool affects_line_p (const char *file, int line) = 0;
   virtual source_location get_start_loc () const = 0;
   virtual bool maybe_get_end_loc (source_location *out) const = 0;
+  /* Vfunc for consolidating successor fixits.  */
+  virtual bool maybe_append_replace (line_maps *set,
+				     source_range src_range,
+				     const char *new_content) = 0;
 };
 
 class fixit_insert : public fixit_hint
@@ -1442,6 +1459,9 @@ class fixit_insert : public fixit_hint
   bool affects_line_p (const char *file, int line);
   source_location get_start_loc () const { return m_where; }
   bool maybe_get_end_loc (source_location *) const { return false; }
+  bool maybe_append_replace (line_maps *set,
+			     source_range src_range,
+			     const char *new_content);
 
   source_location get_location () const { return m_where; }
   const char *get_string () const { return m_bytes; }
@@ -1468,6 +1488,9 @@ class fixit_replace : public fixit_hint
     *out = m_src_range.m_finish;
     return true;
   }
+  bool maybe_append_replace (line_maps *set,
+			     source_range src_range,
+			     const char *new_content);
 
   source_range get_range () const { return m_src_range; }
   const char *get_string () const { return m_bytes; }
diff --git a/libcpp/line-map.c b/libcpp/line-map.c
index 3890eff..d601b71 100644
--- a/libcpp/line-map.c
+++ b/libcpp/line-map.c
@@ -1959,11 +1959,13 @@ source_range::intersects_line_p (const char *file, int line) const
 
 /* Construct a rich_location with location LOC as its initial range.  */
 
-rich_location::rich_location (line_maps */*set*/, source_location loc) :
+rich_location::rich_location (line_maps *set, source_location loc) :
+  m_line_table (set),
   m_num_ranges (0),
   m_column_override (0),
   m_have_expanded_location (false),
-  m_num_fixit_hints (0)
+  m_num_fixit_hints (0),
+  m_seen_impossible_fixit (false)
 {
   add_range (loc, true);
 }
@@ -2075,6 +2077,9 @@ void
 rich_location::add_fixit_insert (source_location where,
 				 const char *new_content)
 {
+  if (reject_impossible_fixit (where))
+    return;
+
   linemap_assert (m_num_fixit_hints < MAX_FIXIT_HINTS);
   m_fixit_hints[m_num_fixit_hints++]
     = new fixit_insert (where, new_content);
@@ -2089,6 +2094,44 @@ rich_location::add_fixit_remove (source_range src_range)
   add_fixit_replace (src_range, "");
 }
 
+/* Return true iff A is in the column directly before B, on the
+   same line of the same source file.  */
+
+static bool
+column_before_p (line_maps *set, source_location a, source_location b)
+{
+  if (IS_ADHOC_LOC (a))
+    a = get_location_from_adhoc_loc (set, a);
+  if (IS_ADHOC_LOC (b))
+    b = get_location_from_adhoc_loc (set, b);
+
+  /* They must both be in ordinary maps.  */
+  const struct line_map *linemap_a = linemap_lookup (set, a);
+  if (linemap_macro_expansion_map_p (linemap_a))
+    return false;
+  const struct line_map *linemap_b = linemap_lookup (set, b);
+  if (linemap_macro_expansion_map_p (linemap_b))
+    return false;
+
+  /* To be on the same line, they must be in the same ordinary map.  */
+  if (linemap_a != linemap_b)
+    return false;
+
+  linenum_type line_a
+    = SOURCE_LINE (linemap_check_ordinary (linemap_a), a);
+  linenum_type line_b
+    = SOURCE_LINE (linemap_check_ordinary (linemap_b), b);
+  if (line_a != line_b)
+    return false;
+
+  linenum_type column_a
+    = SOURCE_COLUMN (linemap_check_ordinary (linemap_a), a);
+  linenum_type column_b
+    = SOURCE_COLUMN (linemap_check_ordinary (linemap_b), b);
+
+  return column_b == column_a + 1;
+}
+
 /* Add a fixit-hint, suggesting replacement of the content at
    SRC_RANGE with NEW_CONTENT.  */
 
@@ -2097,10 +2140,67 @@ rich_location::add_fixit_replace (source_range src_range,
 				  const char *new_content)
 {
   linemap_assert (m_num_fixit_hints < MAX_FIXIT_HINTS);
+
+  if (reject_impossible_fixit (src_range.m_start))
+    return;
+  if (reject_impossible_fixit (src_range.m_finish))
+    return;
+
+  /* Consolidate neighboring fixits.  */
+  fixit_hint *prev = get_last_fixit_hint ();
+  if (m_num_fixit_hints > 0)
+    {
+      if (prev->maybe_append_replace (m_line_table, src_range, new_content))
+	return;
+    }
+
   m_fixit_hints[m_num_fixit_hints++]
     = new fixit_replace (src_range, new_content);
 }
 
+/* FIXME.  */
+
+fixit_hint *
+rich_location::get_last_fixit_hint () const
+{
+  if (m_num_fixit_hints > 0)
+    return m_fixit_hints[m_num_fixit_hints - 1];
+  else
+    return NULL;
+}
+
+/* If WHERE is an "awkward" location, then mark this rich_location as not
+   supporting fixits, purging any thay were already added, and return true.
+
+   Otherwise (the common case), return false.  */
+
+bool
+rich_location::reject_impossible_fixit (source_location where)
+{
+  /* Fix-its within a rich_location should either all be suggested, or
+     none of them should be suggested.
+     Once we've rejected a fixit, we reject any more, even those
+     with reasonable locations.  */
+  if (m_seen_impossible_fixit)
+    return true;
+
+  if (where <= LINE_MAP_MAX_LOCATION_WITH_COLS)
+    /* WHERE is a reasonable location for a fix-it; don't reject it.  */
+    return false;
+
+  /* Otherwise we have an attempt to add a fix-it with an "awkward"
+     location: either one that we can't obtain column information
+     for (within an ordinary map), or one within a macro expansion.  */
+  m_seen_impossible_fixit = true;
+
+  /* Purge the rich_location of any fix-its that were already added. */
+  for (unsigned int i = 0; i < m_num_fixit_hints; i++)
+    delete m_fixit_hints[i];
+  m_num_fixit_hints = 0;
+
+  return true;
+}
+
 /* class fixit_insert.  */
 
 fixit_insert::fixit_insert (source_location where,
@@ -2129,6 +2229,14 @@ fixit_insert::affects_line_p (const char *file, int line)
   return false;
 }
 
+/* FIXME.  */
+
+bool
+fixit_insert::maybe_append_replace (line_maps *, source_range, const char *)
+{
+  return false;
+}
+
 /* class fixit_replace.  */
 
 fixit_replace::fixit_replace (source_range src_range,
@@ -2151,3 +2259,23 @@ fixit_replace::affects_line_p (const char *file, int line)
 {
   return m_src_range.intersects_line_p (file, line);
 }
+
+/* FIXME.  */
+
+bool
+fixit_replace::maybe_append_replace (line_maps *set,
+				     source_range src_range,
+				     const char *new_content)
+{
+  if (!column_before_p (set, m_src_range.m_finish, src_range.m_start))
+    return false;
+
+  /* We have neighboring replacements; merge them.  */
+  m_src_range.m_finish = src_range.m_finish;
+  size_t extra_len = strlen (new_content);
+  m_bytes = (char *)xrealloc (m_bytes, m_len + extra_len + 1);
+  memcpy (m_bytes + m_len, new_content, extra_len);
+  m_len += extra_len;
+  m_bytes[m_len] = '\0';
+  return true;
+}
-- 
1.8.5.3

