Another try at actually using an nth index cache

This re-uses the cache only between the match operations for a single select
iteration which ensures that the selector is not dropped (and its allocation
address re-used) while the cache is alive thereby avoiding the ABA problem fixed
by 7fdac0a.

This does not require any change in user code to benefit from the increased
caching even though some Debug and Clone implementations need to be done
manually now.

Author: adamreichold

src/element_ref/mod.rs

@@ -1,11 +1,13 @@
 //! Element references.
 
+use std::fmt;
 use std::iter::FusedIterator;
 use std::ops::Deref;
 
 use ego_tree::iter::{Edge, Traverse};
 use ego_tree::NodeRef;
 use html5ever::serialize::{serialize, SerializeOpts, TraversalScope};
+use selectors::NthIndexCache;
 
 use crate::node::Element;
 use crate::{Node, Selector};
@@ -47,6 +49,7 @@ impl<'a> ElementRef<'a> {
             scope: *self,
             inner,
             selector,
+            nth_index_cache: NthIndexCache::default(),
         }
     }
 
@@ -122,11 +125,33 @@ impl<'a> Deref for ElementRef<'a> {
 }
 
 /// Iterator over descendent elements matching a selector.
-#[derive(Debug, Clone)]
 pub struct Select<'a, 'b> {
     scope: ElementRef<'a>,
     inner: Traverse<'a, Node>,
     selector: &'b Selector,
+    nth_index_cache: NthIndexCache,
+}
+
+impl fmt::Debug for Select<'_, '_> {
+    fn fmt(&self, fmt: &mut fmt::Formatter<'_>) -> fmt::Result {
+        fmt.debug_struct("Select")
+            .field("scope", &self.scope)
+            .field("inner", &self.inner)
+            .field("selector", &self.selector)
+            .field("nth_index_cache", &"..")
+            .finish()
+    }
+}
+
+impl Clone for Select<'_, '_> {
+    fn clone(&self) -> Self {
+        Self {
+            scope: self.scope,
+            inner: self.inner.clone(),
+            selector: self.selector,
+            nth_index_cache: NthIndexCache::default(),
+        }
+    }
 }
 
 impl<'a, 'b> Iterator for Select<'a, 'b> {
@@ -136,7 +161,11 @@ impl<'a, 'b> Iterator for Select<'a, 'b> {
         for edge in &mut self.inner {
             if let Edge::Open(node) = edge {
                 if let Some(element) = ElementRef::wrap(node) {
-                    if self.selector.matches_with_scope(&element, Some(self.scope)) {
+                    if self.selector.matches_with_scope_and_cache(
+                        &element,
+                        Some(self.scope),
+                        &mut self.nth_index_cache,
+                    ) {
                         return Some(element);
                     }
                 }
@@ -169,6 +198,8 @@ impl<'a> Iterator for Text<'a> {
     }
 }
 
+impl FusedIterator for Text<'_> {}
+
 mod element;
 mod serializable;
 

src/html/mod.rs

@@ -2,14 +2,15 @@
 
 #[cfg(feature = "errors")]
 use std::borrow::Cow;
+use std::fmt;
 use std::iter::FusedIterator;
 
 use ego_tree::iter::Nodes;
 use ego_tree::Tree;
 use html5ever::serialize::SerializeOpts;
 use html5ever::tree_builder::QuirksMode;
-use html5ever::QualName;
-use html5ever::{driver, serialize};
+use html5ever::{driver, serialize, QualName};
+use selectors::NthIndexCache;
 use tendril::TendrilSink;
 
 use crate::selector::Selector;
@@ -94,6 +95,7 @@ impl Html {
         Select {
             inner: self.tree.nodes(),
             selector,
+            nth_index_cache: NthIndexCache::default(),
         }
     }
 
@@ -122,10 +124,30 @@ impl Html {
 }
 
 /// Iterator over elements matching a selector.
-#[derive(Debug)]
 pub struct Select<'a, 'b> {
     inner: Nodes<'a, Node>,
     selector: &'b Selector,
+    nth_index_cache: NthIndexCache,
+}
+
+impl fmt::Debug for Select<'_, '_> {
+    fn fmt(&self, fmt: &mut fmt::Formatter<'_>) -> fmt::Result {
+        fmt.debug_struct("Select")
+            .field("inner", &self.inner)
+            .field("selector", &self.selector)
+            .field("nth_index_cache", &"..")
+            .finish()
+    }
+}
+
+impl Clone for Select<'_, '_> {
+    fn clone(&self) -> Self {
+        Self {
+            inner: self.inner.clone(),
+            selector: self.selector,
+            nth_index_cache: NthIndexCache::default(),
+        }
+    }
 }
 
 impl<'a, 'b> Iterator for Select<'a, 'b> {
@@ -134,7 +156,13 @@ impl<'a, 'b> Iterator for Select<'a, 'b> {
     fn next(&mut self) -> Option<ElementRef<'a>> {
         for node in self.inner.by_ref() {
             if let Some(element) = ElementRef::wrap(node) {
-                if element.parent().is_some() && self.selector.matches(&element) {
+                if element.parent().is_some()
+                    && self.selector.matches_with_scope_and_cache(
+                        &element,
+                        None,
+                        &mut self.nth_index_cache,
+                    )
+                {
                     return Some(element);
                 }
             }
@@ -153,7 +181,13 @@ impl<'a, 'b> DoubleEndedIterator for Select<'a, 'b> {
     fn next_back(&mut self) -> Option<Self::Item> {
         for node in self.inner.by_ref().rev() {
             if let Some(element) = ElementRef::wrap(node) {
-                if element.parent().is_some() && self.selector.matches(&element) {
+                if element.parent().is_some()
+                    && self.selector.matches_with_scope_and_cache(
+                        &element,
+                        None,
+                        &mut self.nth_index_cache,
+                    )
+                {
                     return Some(element);
                 }
             }

src/selector.rs

@@ -8,6 +8,7 @@ use html5ever::{LocalName, Namespace};
 use selectors::{
     matching,
     parser::{self, ParseRelative, SelectorList, SelectorParseErrorKind},
+    NthIndexCache,
 };
 
 use crate::error::SelectorErrorKind;
@@ -42,11 +43,22 @@ impl Selector {
     /// The optional `scope` argument is used to specify which element has `:scope` pseudo-class.
     /// When it is `None`, `:scope` will match the root element.
     pub fn matches_with_scope(&self, element: &ElementRef, scope: Option<ElementRef>) -> bool {
-        let mut nth_index_cache = Default::default();
+        self.matches_with_scope_and_cache(element, scope, &mut NthIndexCache::default())
+    }
+
+    // The `nth_index_cache` must not be used after `self` is dropped
+    // to avoid incorrect results (even though no undefined behaviour is possible)
+    // due to the usage of selector memory addresses as cache keys.
+    pub(crate) fn matches_with_scope_and_cache(
+        &self,
+        element: &ElementRef,
+        scope: Option<ElementRef>,
+        nth_index_cache: &mut NthIndexCache,
+    ) -> bool {
         let mut context = matching::MatchingContext::new(
             matching::MatchingMode::Normal,
             None,
-            &mut nth_index_cache,
+            nth_index_cache,
             matching::QuirksMode::NoQuirks,
             matching::NeedsSelectorFlags::No,
             matching::IgnoreNthChildForInvalidation::No,