Return None from value_source_location() for attributes written without a value

CHANGELOG.md

@@ -1,5 +1,14 @@
 # Changelog
 
+## Unreleased
+
+- Fixed `Attribute::value_source_location()` returning a bogus span for attributes written
+  without a value (e.g. `<div hidden>`): it now returns `None` for them. Previously it
+  returned a zero-length span pointing at the start of the parser's current buffer — or
+  `None`, depending on where the tag fell relative to a `write()` boundary — and in the
+  `None` case `name_source_location()` was also absent. Attribute source locations no
+  longer depend on how the input was chunked.
+
 ## v3.0.1
 
 - Improved performance of selector matching on deeply nested elements and on stray end tags.

src/parser/lexer/actions.rs

@@ -321,12 +321,14 @@ impl<S: LexemeSink> StateMachineActions for Lexer<S> {
             ..
         }) = self.current_attr
         {
-            *value = get_token_part_range!(self);
+            let value_range = get_token_part_range!(self);
+
+            *value = Some(value_range);
 
             // NOTE: include closing quote into the raw value if it's present
             raw_range.end = match input.get(self.next_pos - 1).copied() {
-                Some(ch) if ch == self.closing_quote => value.end + 1,
-                _ => value.end,
+                Some(ch) if ch == self.closing_quote => value_range.end + 1,
+                _ => value_range.end,
             };
         }
     }

src/parser/lexer/lexeme/token_outline.rs

@@ -5,7 +5,8 @@ use crate::parser::AttributeBuffer;
 #[derive(Debug, Default, Copy, Clone)]
 pub(crate) struct AttributeOutline {
     pub name: Range,
-    pub value: Range,
+    /// `None` for attributes written without a value (e.g. `<div hidden>`).
+    pub value: Option<Range>,
     pub raw_range: Range,
 }
 

src/parser/tree_builder_simulator/mod.rs

@@ -291,7 +291,9 @@ impl TreeBuilderSimulator {
                     if !self_closing && eq_case_insensitive(&name, b"annotation-xml") {
                         for attr in attributes {
                             let name = lexeme.part(attr.name);
-                            let value = lexeme.part(attr.value);
+                            let Some(value) = lexeme.opt_part(attr.value) else {
+                                continue;
+                            };
 
                             if eq_case_insensitive(&name, b"encoding")
                                 && (eq_case_insensitive(&value, b"text/html")

src/rewritable_units/tokens/attributes.rs

@@ -6,7 +6,6 @@ use crate::rewritable_units::Serialize;
 use encoding_rs::Encoding;
 use std::cell::OnceCell;
 use std::fmt::{self, Debug};
-use std::num::NonZero;
 use thiserror::Error;
 
 /// An error that occurs when invalid value is provided for the attribute name.
@@ -42,8 +41,9 @@ pub struct Attribute<'i> {
     value: BytesCow<'i>,
     raw: Option<Bytes<'i>>,
     encoding: &'static Encoding,
-    /// absolute document position of attribute name and attribute value
-    name_value_start: Option<(usize, NonZero<usize>)>,
+    /// Absolute document position of the attribute name and, if the attribute
+    /// was written with a value, of the attribute value.
+    name_value_start: Option<(usize, Option<usize>)>,
 }
 
 impl<'i> Attribute<'i> {
@@ -54,7 +54,7 @@ impl<'i> Attribute<'i> {
         value: BytesCow<'i>,
         raw: Bytes<'i>,
         encoding: &'static Encoding,
-        name_value_start: Option<(usize, NonZero<usize>)>,
+        name_value_start: Option<(usize, Option<usize>)>,
     ) -> Self {
         Attribute {
             name,
@@ -123,12 +123,14 @@ impl<'i> Attribute<'i> {
     ///
     /// The range covers only the value itself, excluding any quotes or the `=` sign.
     ///
-    /// Returns `None` for attributes that were added or modified.
+    /// Returns `None` for attributes that were added or modified, and for attributes
+    /// written without a value (e.g. `<div hidden>`).
     #[inline]
     #[must_use]
     pub fn value_source_location(&self) -> Option<SourceLocation> {
         self.name_value_start
-            .map(|(_, value)| SourceLocation::from_start_len(value.get(), self.value.len()))
+            .and_then(|(_, value)| value)
+            .map(|value| SourceLocation::from_start_len(value, self.value.len()))
     }
 
     #[inline]
@@ -279,15 +281,15 @@ impl<'i> Attributes<'i> {
                     .opt_slice(Some(a.name))
                     .unwrap_or_else(cant_fail)
                     .into(),
-                self.input
-                    .opt_slice(Some(a.value))
-                    .unwrap_or_else(cant_fail)
+                a.value
+                    .map(|value| self.input.opt_slice(Some(value)).unwrap_or_else(cant_fail))
+                    .unwrap_or_default()
                     .into(),
                 self.input
                     .opt_slice(Some(a.raw_range))
                     .unwrap_or_else(cant_fail),
                 self.encoding,
-                NonZero::new(base + a.value.start).map(|val| (base + a.name.start, val)),
+                Some((base + a.name.start, a.value.map(|value| base + value.start))),
             )
         })
     }

src/rewriter/mod.rs

@@ -1926,6 +1926,90 @@ mod tests {
             assert_eq!(&html[locs[2].3.clone().unwrap()], "baz");
         }
 
+        /// Collects `(name span, value span)` for every attribute of every `<div>`,
+        /// feeding the input to the rewriter in `chunk_size`-byte writes.
+        #[allow(clippy::type_complexity)]
+        fn div_attribute_locations(
+            html: &str,
+            chunk_size: usize,
+        ) -> Vec<(
+            Option<std::ops::Range<usize>>,
+            Option<std::ops::Range<usize>>,
+        )> {
+            let locations = Arc::new(Mutex::new(Vec::new()));
+            let locations_clone = Arc::clone(&locations);
+
+            let mut rewriter = HtmlRewriter::new(
+                Settings::new().append_element_content_handler(element!("div", move |el| {
+                    locations_clone
+                        .lock()
+                        .unwrap()
+                        .extend(el.attributes().iter().map(|attr| {
+                            (
+                                attr.name_source_location().map(|l| l.bytes()),
+                                attr.value_source_location().map(|l| l.bytes()),
+                            )
+                        }));
+                    Ok(())
+                })),
+                |_: &[u8]| {},
+            );
+
+            for chunk in html.as_bytes().chunks(chunk_size) {
+                rewriter.write(chunk).unwrap();
+            }
+
+            rewriter.end().unwrap();
+
+            let locations = locations.lock().unwrap();
+
+            locations.clone()
+        }
+
+        // See https://github.com/cloudflare/lol-html/issues/333.
+        #[test]
+        fn attribute_source_locations_for_valueless_attributes() {
+            // NOTE: the comment pushes the tag off byte 0 of the parser's buffer,
+            // which used to turn the bogus value span for `hidden` non-zero.
+            for html in ["<div hidden></div>", "<!--pad--><div hidden></div>"] {
+                for chunk_size in [1, 4, 8, 16, html.len()] {
+                    let locations = div_attribute_locations(html, chunk_size);
+
+                    assert_eq!(locations.len(), 1, "{html:?} at chunk size {chunk_size}");
+
+                    let (name, value) = locations[0].clone();
+
+                    assert_eq!(
+                        &html[name.expect("name should have a source location")],
+                        "hidden",
+                        "{html:?} at chunk size {chunk_size}"
+                    );
+                    assert_eq!(
+                        value, None,
+                        "valueless attribute should have no value source location \
+                         ({html:?} at chunk size {chunk_size})"
+                    );
+                }
+            }
+        }
+
+        // See https://github.com/cloudflare/lol-html/issues/333.
+        #[test]
+        fn attribute_source_locations_do_not_depend_on_chunk_size() {
+            let html = r#"<!--pad--><div class="foo" hidden id='bar' data-x=baz></div>"#;
+            let whole_input = div_attribute_locations(html, html.len());
+
+            assert_eq!(whole_input.len(), 4);
+
+            for chunk_size in [1, 2, 3, 5, 8, 16] {
+                assert_eq!(
+                    div_attribute_locations(html, chunk_size),
+                    whole_input,
+                    "spans changed at chunk size {chunk_size}"
+                );
+            }
+        }
+
         #[test]
         fn attribute_source_locations_none_for_programmatic_attributes() {
             rewrite_str::<LocalHandlerTypes>(

src/selectors_vm/attribute_matcher.rs

@@ -78,8 +78,12 @@ impl<'i> AttributeMatcher<'i> {
 
     #[inline]
     fn get_value(&self, lowercased_name: &[u8]) -> Option<&'i [u8]> {
-        self.find(lowercased_name)
-            .map(|a| self.input.slice(a.value).as_slice())
+        // NOTE: an attribute written without a value has an empty value
+        // as far as selector matching is concerned.
+        self.find(lowercased_name).map(|a| {
+            a.value
+                .map_or(&[][..], |value| self.input.slice(value).as_slice())
+        })
     }
 
     #[inline]