Linter: Improve no-void-element-content to detect void blocks (#1513)

This pull request updates the `actionview-no-void-element-content`
linter rule (and the parser itself) to also detect Action View tag
builder that use a block for building an HTML void element.

The following is now flagged by the rule:
```erb
<%= tag.input type: "text" do %>
  input doesn't accept content
<% end %>
```
with:
```
Void element `input` cannot have content. `tag.input` does not accept a block for content.
```

Author: marcoroth

config.yml

@@ -460,10 +460,11 @@ errors:
 
     - name: VoidElementContentError
       message:
-        template: "Void element `%s` cannot have content. `%s` does not accept a positional argument for content."
+        template: "Void element `%s` cannot have content. `%s` does not accept %s for content."
         arguments:
           - tag_name->value
           - helper_name
+          - content_type
 
       fields:
         - name: tag_name
@@ -472,6 +473,9 @@ errors:
         - name: helper_name
           type: string
 
+        - name: content_type
+          type: string
+
     - name: DotNotationCasingError
       message:
         template: "Dot-notation component tags require the first segment to start with an uppercase letter. `%s` does not start with an uppercase letter."

javascript/packages/linter/test/rules/actionview-no-void-element-content.test.ts

@@ -91,4 +91,56 @@ describe("ActionViewNoVoidElementContentRule", () => {
       <%= content_tag :br, "hello" %>
     `)
   })
+
+  test("tag.img with do...end block is not allowed", () => {
+    expectError(`Void element \`img\` cannot have content. \`tag.img\` does not accept a block for content.`)
+
+    assertOffenses(dedent`
+      <%= tag.img alt: "hello" do %>
+        a
+      <% end %>
+    `)
+  })
+
+  test("tag.br with inline block is not allowed", () => {
+    expectError(`Void element \`br\` cannot have content. \`tag.br\` does not accept a block for content.`)
+
+    assertOffenses(dedent`
+      <%= tag.br { "content" } %>
+    `)
+  })
+
+  test("tag.input with do...end block is not allowed", () => {
+    expectError(`Void element \`input\` cannot have content. \`tag.input\` does not accept a block for content.`)
+
+    assertOffenses(dedent`
+      <%= tag.input do %>
+        content
+      <% end %>
+    `)
+  })
+
+  test("content_tag :input with do...end block is not allowed", () => {
+    expectError(`Void element \`input\` cannot have content. \`content_tag :input\` does not accept a block for content.`)
+
+    assertOffenses(dedent`
+      <%= content_tag :input do %>
+        content
+      <% end %>
+    `)
+  })
+
+  test("tag.div with do...end block is allowed (non-void element)", () => {
+    expectNoOffenses(dedent`
+      <%= tag.div do %>
+        content
+      <% end %>
+    `)
+  })
+
+  test("tag.span with inline block is allowed (non-void element)", () => {
+    expectNoOffenses(dedent`
+      <%= tag.span { "content" } %>
+    `)
+  })
 })

sig/herb/errors.rbs

@@ -586,9 +586,14 @@ static AST_NODE_T* transform_tag_helper_with_attributes(
         }
       }
 
+      pm_call_node_t* void_call = parse_context->info->call_node;
+      bool content_from_block = void_call && void_call->block && void_call->block->type == PM_BLOCK_NODE;
+      hb_string_T content_type = content_from_block ? hb_string("a block") : hb_string("a positional argument");
+
       append_void_element_content_error(
         tag_name_token,
         helper_name,
+        content_type,
         content_start,
         content_end,
         allocator,
@@ -946,9 +951,42 @@ static AST_NODE_T* transform_erb_block_to_tag_helper(
   );
 
   hb_array_T* body = block_node->body ? block_node->body : hb_array_init(0, allocator);
+  hb_array_T* element_errors = hb_array_init(0, allocator);
   AST_NODE_T* close_tag = (AST_NODE_T*) block_node->end_node;
   position_T element_end = block_node->base.location.end;
 
+  bool is_void = tag_name && is_void_element(hb_string_from_c_string(tag_name)) && parse_context->matched_handler
+              && parse_context->matched_handler->name
+              && (string_equals(parse_context->matched_handler->name, "tag")
+                  || string_equals(parse_context->matched_handler->name, "content_tag")
+                  || string_equals(parse_context->matched_handler->name, "image_tag"));
+
+  if (is_void) {
+    hb_buffer_T helper_name_buffer;
+    hb_buffer_init(&helper_name_buffer, 64, allocator);
+
+    if (string_equals(parse_context->matched_handler->name, "tag")) {
+      hb_buffer_append(&helper_name_buffer, "tag.");
+      hb_buffer_append(&helper_name_buffer, tag_name);
+    } else {
+      hb_buffer_append(&helper_name_buffer, parse_context->matched_handler->name);
+      hb_buffer_append(&helper_name_buffer, " :");
+      hb_buffer_append(&helper_name_buffer, tag_name);
+    }
+
+    hb_string_T helper_name = hb_string_from_c_string(hb_buffer_value(&helper_name_buffer));
+
+    append_void_element_content_error(
+      tag_name_token,
+      helper_name,
+      hb_string("a block"),
+      block_node->base.location.start,
+      block_node->base.location.end,
+      allocator,
+      element_errors
+    );
+  }
+
   if (tag_name && parser_is_foreign_content_tag(hb_string_from_c_string(tag_name)) && context->source
       && block_node->body && hb_array_size(block_node->body) > 0) {
     size_t start_offset = block_node->tag_closing->range.to;
@@ -999,12 +1037,12 @@ static AST_NODE_T* transform_erb_block_to_tag_helper(
     (AST_NODE_T*) open_tag_node,
     tag_name_token,
     body,
-    close_tag,
-    false,
+    is_void ? NULL : close_tag,
+    is_void,
     parse_context->matched_handler->source,
     block_node->base.location.start,
     element_end,
-    hb_array_init(0, allocator),
+    element_errors,
     allocator
   );
 

src/analyze/action_view/tag_helpers.c

@@ -284,5 +284,27 @@ class ContentTagTest < Minitest::Spec
         <%= content_tag(:div, content_tag(:p, "Hello world!"), class: "strong") %>
       HTML
     end
+
+    test "content_tag :input with do...end block reports void element content error" do
+      assert_parsed_snapshot(<<~HTML, action_view_helpers: true)
+        <%= content_tag :input do %>
+          content
+        <% end %>
+      HTML
+    end
+
+    test "content_tag :img with do...end block reports void element content error" do
+      assert_parsed_snapshot(<<~HTML, action_view_helpers: true)
+        <%= content_tag :img do %>
+          content
+        <% end %>
+      HTML
+    end
+
+    test "content_tag :br with inline block reports void element content error" do
+      assert_parsed_snapshot(<<~HTML, action_view_helpers: true)
+        <%= content_tag(:br) { "content" } %>
+      HTML
+    end
   end
 end

test/analyze/action_view/tag_helper/content_tag_test.rb

@@ -573,5 +573,35 @@ class TagTest < Minitest::Spec
         <% end %>
       HTML
     end
+
+    test "tag.img with do...end block reports void element content error" do
+      assert_parsed_snapshot(<<~HTML, action_view_helpers: true)
+        <%= tag.img alt: "hello" do %>
+          a
+        <% end %>
+      HTML
+    end
+
+    test "tag.br with inline block reports void element content error" do
+      assert_parsed_snapshot(<<~HTML, action_view_helpers: true)
+        <%= tag.br { "content" } %>
+      HTML
+    end
+
+    test "tag.input with do...end block reports void element content error" do
+      assert_parsed_snapshot(<<~HTML, action_view_helpers: true)
+        <%= tag.input do %>
+          content
+        <% end %>
+      HTML
+    end
+
+    test "tag.hr with do...end block reports void element content error" do
+      assert_parsed_snapshot(<<~HTML, action_view_helpers: true)
+        <%= tag.hr class: "divider" do %>
+          content
+        <% end %>
+      HTML
+    end
   end
 end