sarif output: add descriptions to fix-it hints (§3.55.2) [PR121986]

SARIF "fix" objects SHOULD have a "description" property (§3.55.2) that
describes the proposed fix, but currently GCC's SARIF output doesn't
support this, and we don't capture this anywhere internally as we build
fix-it hints in the compiler.

Currently we can have zero or more instances of fixit_hint associated
with a diagnostic, each representing an edit of a range of the source
code. Ideally we would have an internal API that allowed for associating
multiple fixes with a diagnostic, each with a description worded in terms
of the source language (e.g. "Fix 'colour' mispelling of field 'color'"),
and each consisting of multiple edited ranges.

For now, this patch extends the sarif output sink so that it
autogenerates descriptions of fix-it hints for simple cases of
insertion, deletion, and replacement of a single range
(e.g. "Replace 'colour' with 'color'").

gcc/ChangeLog:
	PR diagnostics/121986
	* diagnostics/sarif-sink.cc: Include "intl.h".
	(sarif_builder::make_message_describing_fix_it_hint): New.
	(sarif_builder::make_fix_object): Attempt to auto-generate a
	description for fix-it hints.

gcc/testsuite/ChangeLog:
	PR diagnostics/121986
	* gcc.dg/sarif-output/extra-semicolon.c: New test.
	* gcc.dg/sarif-output/extra-semicolon.py: New test.
	* gcc.dg/sarif-output/missing-semicolon.py: Verify the description
	of the insertion fix-it hint.
	* libgdiagnostics.dg/test-fix-it-hint-c.py: Verify the description
	of the replacement fix-it hint.

libcpp/ChangeLog:
	PR diagnostics/121986
	* include/rich-location.h (fixit_hint::deletion_p): New accessor.
	(fixit_hint::replacement_p): New accessor.

Signed-off-by: David Malcolm <dmalcolm@redhat.com>

diff --git a/gcc/diagnostics/sarif-sink.cc b/gcc/diagnostics/sarif-sink.cc
index 144040e9dfb10f3d3fd0f22e9e09760ad582c0ef..8bd026901c6b4033e26da766564691a57786ea02 100644 (file)
--- a/gcc/diagnostics/sarif-sink.cc
+++ b/gcc/diagnostics/sarif-sink.cc
@@ -54,6 +54,7 @@ along with GCC; see the file COPYING3.  If not see
 #include "demangle.h"
 #include "backtrace.h"
 #include "xml.h"
+#include "intl.h"
 
 namespace diagnostics {
 
@@ -948,6 +949,8 @@ private:
                                      int start_line,
                                      int end_line,
                                      const content_renderer *r) const;
+  std::unique_ptr<sarif_message>
+  make_message_describing_fix_it_hint (const fixit_hint &hint) const;
   std::unique_ptr<sarif_fix>
   make_fix_object (const rich_location &rich_loc);
   std::unique_ptr<sarif_artifact_change>
@@ -3747,6 +3750,65 @@ maybe_make_artifact_content_object (const char *filename,
   return artifact_content_obj;
 }
 
+/* Attempt to generate a "message" object describing a fix-it hint,
+   or null if there was a problem.  */
+
+std::unique_ptr<sarif_message>
+sarif_builder::
+make_message_describing_fix_it_hint (const fixit_hint &hint) const
+{
+  pretty_printer pp;
+  if (hint.insertion_p ())
+    pp_printf (&pp, G_("Insert %qs"), hint.get_string ());
+  else
+    {
+      /* Try to get prior content.  */
+      expanded_location start = expand_location (hint.get_start_loc ());
+      expanded_location next_loc = expand_location (hint.get_next_loc ());
+      if (start.file != next_loc.file)
+       return nullptr;
+      if (start.line != next_loc.line)
+       return nullptr;
+      if (start.column == 0)
+       return nullptr;
+      if (next_loc.column == 0)
+       return nullptr;
+
+      const int start_offset = start.column - 1;
+      const int next_offset = next_loc.column - 1;
+      if (next_offset <= start_offset)
+       return nullptr;
+
+      size_t victim_len = next_offset - start_offset;
+
+      char_span existing_line = get_context ()
+       .get_file_cache ()
+       .get_source_line (start.file, start.line);
+      if (!existing_line)
+       return nullptr;
+
+      label_text existing_text
+       = label_text::take (existing_line.subspan (start_offset,
+                                                  victim_len).xstrdup ());
+
+      if (hint.deletion_p ())
+       {
+         // Removal
+         pp_printf (&pp, G_("Delete %qs"),
+                    existing_text.get ());
+       }
+      else
+       {
+         // Replacement
+         gcc_assert (hint.replacement_p ());
+         pp_printf (&pp, G_("Replace %qs with %qs"),
+                    existing_text.get (),
+                    hint.get_string ());
+       }
+    }
+  return make_message_object (pp_formatted_text (&pp));
+}
+
 /* Make a "fix" object (SARIF v2.1.0 section 3.55) for RICHLOC.  */
 
 std::unique_ptr<sarif_fix>
@@ -3762,6 +3824,18 @@ sarif_builder::make_fix_object (const rich_location &richloc)
   fix_obj->set<json::array> ("artifactChanges",
                             std::move (artifact_change_arr));
 
+  // 3.55.2 "description" property
+  /* Attempt to generate a description.  We can only do this
+     if there was a single hint.  */
+  if (richloc.get_num_fixit_hints () == 1)
+    {
+      const fixit_hint *hint = richloc.get_fixit_hint (0);
+      gcc_assert (hint);
+      if (auto desc_msg = make_message_describing_fix_it_hint (*hint))
+       fix_obj->set<sarif_message> ("description",
+                                    std::move (desc_msg));
+    }
+
   return fix_obj;
 }
 

diff --git a/gcc/testsuite/gcc.dg/sarif-output/extra-semicolon.c b/gcc/testsuite/gcc.dg/sarif-output/extra-semicolon.c
new file mode 100644 (file)
index 0000000..bcbb267
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/sarif-output/extra-semicolon.c
@@ -0,0 +1,16 @@
+/* { dg-do compile } */
+/* { dg-options "-fdiagnostics-format=sarif-file -Wpedantic" } */
+
+/* Verify SARIF output for a deletion fix-it hint.  */
+
+struct foo
+{
+  int color;;  
+};
+
+/* Verify that some JSON was written to a file with the expected name:
+   { dg-final { verify-sarif-file } } */
+
+/* Use a Python script to verify various properties about the generated
+   .sarif file:
+   { dg-final { run-sarif-pytest extra-semicolon.c "extra-semicolon.py" } } */

diff --git a/gcc/testsuite/gcc.dg/sarif-output/extra-semicolon.py b/gcc/testsuite/gcc.dg/sarif-output/extra-semicolon.py
new file mode 100644 (file)
index 0000000..d7f54d2
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/sarif-output/extra-semicolon.py
@@ -0,0 +1,37 @@
+from sarif import *
+
+import pytest
+
+@pytest.fixture(scope='function', autouse=True)
+def sarif():
+    return sarif_from_env()
+
+def test_deletion_fixit(sarif):
+    runs = sarif['runs']
+    run = runs[0]
+    results = run['results']
+
+    # We expect a single error with a secondary location and a fix-it hint.
+    #
+    # The textual form of the diagnostic would look like this:
+    #  . PATH/extra-semicolon.c:8:13: warning: extra semicolon in struct or union specified [-Wpedantic]
+    #  .     8 |   int color;;
+    #  .       |             ^
+    #  .       |             -
+    assert len(results) == 1
+    
+    result = results[0]
+    assert result['level'] == 'warning'
+    assert result['message']['text'] == "extra semicolon in struct or union specified"
+
+    # We expect one fix-it hint representing a deletion of ';'
+    assert len(result['fixes']) == 1
+    fix = result['fixes'][0]
+    assert fix['description']['text'] == "Delete ';'"
+    assert len(fix['artifactChanges']) == 1
+    change = fix['artifactChanges'][0]
+    replacement = change['replacements'][0]
+    assert replacement['deletedRegion']['startLine'] == 8
+    assert replacement['deletedRegion']['startColumn'] == 13
+    assert replacement['deletedRegion']['endColumn'] == 14
+    assert replacement['insertedContent']['text'] == ''

diff --git a/gcc/testsuite/gcc.dg/sarif-output/missing-semicolon.py b/gcc/testsuite/gcc.dg/sarif-output/missing-semicolon.py
index a0f848387a22e830125135ab398d9ad36864b279..374d2ec0cfaf0d127c49032a663a6650a95e6a21 100644 (file)
--- a/gcc/testsuite/gcc.dg/sarif-output/missing-semicolon.py
+++ b/gcc/testsuite/gcc.dg/sarif-output/missing-semicolon.py
@@ -79,8 +79,10 @@ def test_location_relationships(sarif):
 
     # We expect one fix-it hint representing an insertion of ';'
     assert len(result['fixes']) == 1
-    assert len(result['fixes'][0]['artifactChanges']) == 1
-    change = result['fixes'][0]['artifactChanges'][0]
+    fix = result['fixes'][0]
+    assert fix['description']['text'] == "Insert ';'"
+    assert len(fix['artifactChanges']) == 1
+    change = fix['artifactChanges'][0]
     assert change['artifactLocation']['uri'].endswith('missing-semicolon.c')
     assert len(change['replacements']) == 1
     replacement = change['replacements'][0]

diff --git a/gcc/testsuite/libgdiagnostics.dg/test-fix-it-hint-c.py b/gcc/testsuite/libgdiagnostics.dg/test-fix-it-hint-c.py
index f3dc71c27b562b37865f72dd9866b66f58511157..cf1466e6983efadca1e3549ca99868c685669d41 100644 (file)
--- a/gcc/testsuite/libgdiagnostics.dg/test-fix-it-hint-c.py
+++ b/gcc/testsuite/libgdiagnostics.dg/test-fix-it-hint-c.py
@@ -37,6 +37,7 @@ def test_sarif_output_with_fixes(sarif):
 
     assert len(results[0]['fixes']) == 1
     fix = results[0]['fixes'][0]
+    assert fix['description']['text'] == "Replace 'colour' with 'color'"
     assert len(fix['artifactChanges']) == 1
     change = fix['artifactChanges'][0]
     assert change['artifactLocation']['uri'].endswith('test-fix-it-hint.c')

diff --git a/libcpp/include/rich-location.h b/libcpp/include/rich-location.h
index c74e80e9f61eb63fc1b9b38a77ba8a4f1022489e..cc9547d3ab7a9e1d081d6c85a2da3b07d854b1c0 100644 (file)
--- a/libcpp/include/rich-location.h
+++ b/libcpp/include/rich-location.h
@@ -642,6 +642,8 @@ class fixit_hint
   size_t get_length () const { return m_len; }
 
   bool insertion_p () const { return m_start == m_next_loc; }
+  bool deletion_p () const { return m_len == 0; }
+  bool replacement_p () const { return m_len > 0 && m_start != m_next_loc; }
 
   bool ends_with_newline_p () const;