RULEAPI-788 Validate use of diff-id and diff-type (#2790)

The validation is not yet enabled on CI checks because there exist too many errors in existing rule descriptions at the moment.

.cirrus.yml

@@ -87,7 +87,7 @@ validate_metadata_task:
     - ./ci/validate_metadata.sh
 
 validate_ci_tests_task:
-  skip: "!changesInclude('ci_tests/*', 'ci/*')"
+  skip: "!changesInclude('ci_tests/**', 'ci/**')"
   eks_container:
     <<: *CONTAINER_DEFINITION
     dockerfile: ci/Dockerfile

ci/asciidoc_validation/custom-asciidoctor

@@ -1,18 +1,24 @@
-#!/usr/bin/ruby
+#!/usr/bin/env ruby
 # Based on asciidoctor main ruby script.
 # This is only meant to introspect and log which asciidoc files were used.
 
 require 'asciidoctor'
 require 'asciidoctor/cli'
 
+MAIN_FILE_REGEX = /^.*\/rules\/(?<id>S\d+)\/(?:(?<lang>\w+)\/)?rule.adoc$/
+
 class MainFileLogger < Asciidoctor::Extensions::Preprocessor
   include Asciidoctor::Logging
 
   def process document, reader
+    # Enable sourcemap to track source location.
+    # This is useful to report more accurate errors in other loggers.
+    document.sourcemap = true
+
     main_file = document.normalize_system_path(reader.file, document.reader.dir)
 
     # This assumes unix-style path separator.
-    if nil == main_file.match(/^.*\/rules\/S\d+\/(\w+\/)?rule.adoc$/)
+    if nil == main_file.match(MAIN_FILE_REGEX)
       abort("Main file does not follow expected pattern: #{main_file}")
     end
 
@@ -73,9 +79,87 @@ class IncludeLogger < Asciidoctor::Extensions::IncludeProcessor
   # Intentionnaly no process function here.
 end
 
+class SourceLogger < Asciidoctor::Extensions::TreeProcessor
+  include Asciidoctor::Logging
+
+  def get_source_location block
+    loc = block.source_location # Asciidoctor::Reader::Cursor.
+    return "#{loc.file}:#{loc.lineno}"
+  end
+
+  def get_rule document
+    main_file = document.normalize_system_path(document.reader.file, document.reader.dir)
+    res = main_file.match(MAIN_FILE_REGEX)
+    lang = res[:lang] || 'default'
+    "#{res[:id]}/#{lang}"
+  end
+
+  def process document
+    rule = get_rule(document)
+    source_blocks = document.find_by(context: :listing, style: 'source')
+
+    # Collect individually valid blocks per diff-id.
+    blocks_per_id = Hash.new { |hash, key| hash[key] = Array.new }
+
+    # Find blocks with only diff-id but no diff-type, or vice-versa, or invalid diff-type.
+    source_blocks.each do |block|
+      id = block.attr('diff-id', nil)
+      type = block.attr('diff-type', nil)
+
+      if !id and !type
+        next # Nothing to validate
+      end
+
+      loc = get_source_location(block)
+
+      if !id
+        logger.info("ASCIIDOC LOGGER DIFF:[#{rule}] diff-id is missing in #{loc}")
+        next
+      end
+
+      if !type
+        logger.info("ASCIIDOC LOGGER DIFF:[#{rule}] diff-type is missing in #{loc}")
+        next
+      elsif !['compliant', 'noncompliant'].include?(type)
+        logger.info("ASCIIDOC LOGGER DIFF:[#{rule}] diff-type '#{type}' is not valid in #{loc}")
+        next
+      end
+
+      # The block is valid on its own.
+      blocks_per_id[id].push(block)
+    end
+
+    # Each diff-id should have exactly 1 compliant and 1 noncompliant block.
+    # Find blocks that break this rule.
+    blocks_per_id.each do |id, blocks|
+      # Sort to ensure deterministic output.
+      blocks.sort_by! { |block| get_source_location(block) }
+
+      case blocks.length
+      when 1
+        loc = get_source_location(blocks[0])
+        logger.info("ASCIIDOC LOGGER DIFF:[#{rule}] Incomplete example for diff-id=#{id}, missing counterpart for #{loc}")
+      when 2
+        type1 = blocks[0].attr('diff-type')
+        type2 = blocks[1].attr('diff-type')
+        if type1 == type2
+          loc1 = get_source_location(blocks[0])
+          loc2 = get_source_location(blocks[1])
+          logger.info("ASCIIDOC LOGGER DIFF:[#{rule}] Two #{type1} examples for diff-id=#{id}: #{loc1} and #{loc2}")
+        end
+      else
+        locs = blocks.map { |block| get_source_location(block) }
+        locs = locs.join(', ')
+        logger.info("ASCIIDOC LOGGER DIFF:[#{rule}] #{blocks.length} examples for diff-id=#{id}: #{locs}")
+      end
+    end
+  end
+end
+
 Asciidoctor::Extensions.register do
   preprocessor MainFileLogger
   include_processor IncludeLogger.new @document
+  treeprocessor SourceLogger
 end
 
 invoker = Asciidoctor::Cli::Invoker.new ARGV

ci/asciidoc_validation/validate.sh

@@ -22,6 +22,7 @@
 #     asciidoctor: INFO: ASCIIDOC LOGGER MAIN FILE: $PATH
 #     asciidoctor: INFO: ASCIIDOC LOGGER INCLUDED: $PATH
 #     asciidoctor: INFO: ASCIIDOC LOGGER CROSSREFERENCE: $RULEID cross-references $PATH
+#     asciidoctor: INFO: ASCIIDOC LOGGER DIFF: $VALIDATION_FAILURE_MESSAGE
 
 set -euo pipefail
 
@@ -39,21 +40,19 @@ grep_nofail() {
   grep "$@" || [ "$?" == "1" ]
 }
 
+extract_messages_from_log() {
+  # The first 3 columns of the log are not relevant.
+  # The 4th (and any that follows if ':' is used in the message)
+  # provides the relevant validation error message.
+  cut -d ':' -f 4- | sort -u
+}
+
 find "${RULES_DIR}" -name 'rule.adoc' \
   | xargs "${SCRIPT_DIR}/custom-asciidoctor" -a rspecator-view --verbose -R "${RULES_DIR}" -D "${TMPOUT_DIR}" 2>&1 \
   | grep_nofail -e 'ASCIIDOC LOGGER' \
   > "${TMPOUT_DIR}/asciidoc_introspection"
 
-grep -ve 'CROSSREFERENCE' "${TMPOUT_DIR}/asciidoc_introspection" \
+cross_references=$(grep_nofail -e 'CROSSREFERENCE' "${TMPOUT_DIR}/asciidoc_introspection" | extract_messages_from_log)
-  | cut -d ':' -f 4 \
-  | sort -u \
-  > "${TMPOUT_DIR}/used_asciidoc_files"
-
-git ls-files --cached -- "${RULES_DIR}/**.adoc" "${SHARED_CONTENT_DIR}/**.adoc" \
-  | xargs realpath \
-  > "${TMPOUT_DIR}/all_asciidoc_files"
-
-cross_references=$(grep_nofail -e 'CROSSREFERENCE' "${TMPOUT_DIR}/asciidoc_introspection" | cut -d ':' -f 4 | sort -u)
 if [[ -n "$cross_references" ]]; then
   echo >&2 'ERROR: Some rules try to include content from unallowed directories.'
   echo >&2 'To share content between rules, you should use the "shared_content" folder at the root of the repository.'
@@ -62,10 +61,29 @@ if [[ -n "$cross_references" ]]; then
   exit_code=1
 fi
 
+grep_nofail -Pe '(INCLUDE|MAIN FILE)' "${TMPOUT_DIR}/asciidoc_introspection" \
+  | extract_messages_from_log \
+  > "${TMPOUT_DIR}/used_asciidoc_files"
+
+git ls-files --cached -- "${RULES_DIR}/**.adoc" "${SHARED_CONTENT_DIR}/**.adoc" \
+  | xargs realpath \
+  > "${TMPOUT_DIR}/all_asciidoc_files"
+
 orphans=$(comm -1 -3 <(sort -u "${TMPOUT_DIR}/used_asciidoc_files") <(sort -u "${TMPOUT_DIR}/all_asciidoc_files"))
-if [[ -n "$orphans" ]]; then
+if [[ -n "$orphans" ]]
+then
   printf >&2 'ERROR: These adoc files are not included anywhere:\n-----\n%s\n-----\n' "$orphans"
   exit_code=1
 fi
 
+bad_diffs=$(grep_nofail -e 'DIFF' "${TMPOUT_DIR}/asciidoc_introspection" | extract_messages_from_log)
+if [[ -n "$bad_diffs" ]]
+then
+  printf >&2 'ERROR: Diff highlighting is used incorrectly:\n-----\n%s\n-----\n' "$bad_diffs"
+  if [[ -n "${RUNNING_CI_INTEGRATION_TEST:-}" ]]
+  then
+    exit_code=1 # FIXME: there are currently validation errors in the repo. Unconditionally enable this line when they are all fixed.
+  fi
+fi
+
 exit $exit_code

ci_tests/asciidoc_validation/run_tests.sh

@@ -4,6 +4,8 @@
 
 set -uo pipefail
 
+export RUNNING_CI_INTEGRATION_TEST=1 # FIXME remove this once it's no longer needed in validate.sh
+
 # We could write complex checks to ensure only specific commands fail and emit
 # a specific error message. Instead, we rely on `set -xe` to consistently and
 # reliably exit with non-zero if any command fails and pinpoint which command
@@ -77,4 +79,22 @@ run_test "test_bad_cross_ref" \
   "S100 cross-references .*rules/S1000/bad.adoc" \
   "S1000 cross-references .*rules/S100/java/bad.adoc"
 
+run_test "test_diff_source" \
+  "ERROR: Diff highlighting is used incorrectly:" \
+  "\[S100/cfamily] diff-type is missing in .*/rules/S100/cfamily/rule.adoc:3" \
+  "\[S100/cfamily] diff-id is missing in .*/rules/S100/cfamily/rule.adoc:8" \
+  "\[S100/cfamily] diff-type 'bad' is not valid in .*/rules/S100/cfamily/rule.adoc:13" \
+  "\[S100/cfamily] diff-type is missing in .*/rules/S100/cfamily/local.adoc:3" \
+  "\[S100/cfamily] diff-id is missing in .*/rules/S100/cfamily/local.adoc:8" \
+  "\[S100/cfamily] diff-type 'local' is not valid in .*/rules/S100/cfamily/local.adoc:13" \
+  "\[S100/cfamily] diff-type is missing in .*/shared_content/cfamily/shared.adoc:3" \
+  "\[S100/cfamily] diff-id is missing in .*/shared_content/cfamily/shared.adoc:8" \
+  "\[S100/cfamily] diff-type 'shared' is not valid in .*/shared_content/cfamily/shared.adoc:13" \
+  "\[S100/java] Incomplete example for diff-id=1, missing counterpart for .*/rules/S100/java/rule.adoc:3" \
+  "\[S100/java] Incomplete example for diff-id=2, missing counterpart for .*/rules/S100/java/rule.adoc:8" \
+  "\[S100/java] Incomplete example for diff-id=3, missing counterpart for .*/shared_content/java/example.adoc:3" \
+  "\[S100/java] 3 examples for diff-id=4: .*/rules/S100/java/rule.adoc:15, .*/shared_content/java/example.adoc:13, .*/shared_content/java/example.adoc:8" \
+  "\[S200/default] 3 examples for diff-id=1: .*/rules/S200/rule.adoc:12, .*/rules/S200/rule.adoc:2, .*/rules/S200/rule.adoc:7" \
+  "\[S200/default] Two noncompliant examples for diff-id=2: .*/rules/S200/rule.adoc:17 and .*/rules/S200/rule.adoc:22"
+
 echo "All tests passed"

ci_tests/asciidoc_validation/test_diff_source/rules/S100/cfamily/local.adoc

@@ -0,0 +1,15 @@
+
+[source,cpp,diff-id=1]
+----
+Missing diff-type
+----
+
+[source,cpp,diff-type=compliant]
+----
+Missing diff-id
+----
+
+[source,c,diff-id=1,diff-type=local]
+----
+Bad diff-type
+----

ci_tests/asciidoc_validation/test_diff_source/rules/S100/cfamily/rule.adoc

@@ -0,0 +1,19 @@
+
+[source,cpp,diff-id=1]
+----
+Missing diff-type
+----
+
+[source,cpp,diff-type=compliant]
+----
+Missing diff-id
+----
+
+[source,c,diff-id=1,diff-type=bad]
+----
+Bad diff-type
+----
+
+include::./local.adoc[]
+
+include::../../../shared_content/cfamily/shared.adoc[]

ci_tests/asciidoc_validation/test_diff_source/rules/S100/java/rule.adoc

@@ -0,0 +1,17 @@
+
+[source,java,diff-id=1,diff-type=compliant]
+----
+1. compliant but missing noncompliant
+----
+
+[source,java,diff-id=2,diff-type=noncompliant]
+----
+2. noncompliant but missing compliant
+----
+
+include::../../../shared_content/java/example.adoc[]
+
+[source,java,diff-id=4,diff-type=noncompliant]
+----
+4. noncompliant B
+----

ci_tests/asciidoc_validation/test_diff_source/rules/S200/rule.adoc

@@ -0,0 +1,24 @@
+[source,diff-id=1,diff-type=compliant]
+----
+1. compliant
+----
+
+[source,diff-id=1,diff-type=noncompliant]
+----
+1. noncompliant A
+----
+
+[source,diff-id=1,diff-type=noncompliant]
+----
+1. noncompliant B
+----
+
+[source,diff-id=2,diff-type=noncompliant]
+----
+2. noncompliant A
+----
+
+[source,diff-id=2,diff-type=noncompliant]
+----
+2. noncompliant B
+----

ci_tests/asciidoc_validation/test_diff_source/shared_content/cfamily/shared.adoc

@@ -0,0 +1,15 @@
+
+[source,cpp,diff-id=1]
+----
+Missing diff-type
+----
+
+[source,cpp,diff-type=compliant]
+----
+Missing diff-id
+----
+
+[source,c,diff-id=1,diff-type=shared]
+----
+Bad diff-type
+----

ci_tests/asciidoc_validation/test_diff_source/shared_content/java/example.adoc

@@ -0,0 +1,15 @@
+
+[source,java,diff-id=3,diff-type=compliant]
+----
+3. compliant but missing noncompliant
+----
+
+[source,java,diff-id=4,diff-type=compliant]
+----
+4. compliant but too many noncompliant
+----
+
+[source,java,diff-id=4,diff-type=noncompliant]
+----
+4. noncompliant A
+----

ci_tests/asciidoc_validation/test_valid/rules/S100/cfamily/rule.adoc

@@ -3,3 +3,13 @@ S100 cfamily
 include::./another.adoc[]
 
 include::../../../shared_content/anything.adoc[]
+
+[source,cpp,diff-id=1,diff-type=noncompliant]
+----
+noncompliant
+----
+
+[source,cpp,diff-id=1,diff-type=compliant]
+----
+compliant
+----