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
This commit is contained in:
David Malcolm 2016-08-26 21:25:41 +00:00 committed by David Malcolm
parent d41e76cf75
commit ee90851679
8 changed files with 365 additions and 11 deletions

View file

@ -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);
}
/* Get the last fix-it hint within this rich_location, or NULL if none. */
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,15 @@ fixit_insert::affects_line_p (const char *file, int line)
return false;
}
/* Implementation of maybe_append_replace for fixit_insert. Reject
the attempt to consolidate fix-its. */
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 +2260,26 @@ fixit_replace::affects_line_p (const char *file, int line)
{
return m_src_range.intersects_line_p (file, line);
}
/* Implementation of maybe_append_replace for fixit_replace. If
possible, merge the new replacement into this one and return true.
Otherwise return false. */
bool
fixit_replace::maybe_append_replace (line_maps *set,
source_range src_range,
const char *new_content)
{
/* Does SRC_RANGE start immediately after this one finishes? */
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;
}