document followup work

src/python/pants/engine/fs.py

@@ -38,6 +38,9 @@ class PathGlobs(datatype([
   """A wrapper around sets of filespecs to include and exclude.
 
   The syntax supported is roughly git's glob syntax.
+
+  NB: this object is interpreted from within Snapshot::lift_path_globs() -- that method will need to
+  be aware of any changes to this object's definition.
   """
 
   def __new__(cls, include, exclude, glob_match_error_behavior=None):

src/python/pants/engine/legacy/graph.py

@@ -381,6 +381,8 @@ def _eager_fileset_with_spec(spec_path, filespec, snapshot, include_dirs=False):
 def hydrate_sources(sources_field, glob_match_error_behavior):
   """Given a SourcesField, request a Snapshot for its path_globs and create an EagerFilesetWithSpec.
   """
+  # TODO(cosmicexplorer): merge the target's selection of --glob-expansion-failure (which doesn't
+  # exist yet) with the global default! See #5864.
   path_globs = sources_field.path_globs.with_match_error_behavior(glob_match_error_behavior)
   snapshot = yield Get(Snapshot, PathGlobs, path_globs)
   fileset_with_spec = _eager_fileset_with_spec(
@@ -393,8 +395,6 @@ def hydrate_sources(sources_field, glob_match_error_behavior):
 @rule(HydratedField, [Select(BundlesField), Select(GlobMatchErrorBehavior)])
 def hydrate_bundles(bundles_field, glob_match_error_behavior):
   """Given a BundlesField, request Snapshots for each of its filesets and create BundleAdaptors."""
-  # TODO(cosmicexplorer): merge the target's selection of --glob-expansion-failure (which doesn't
-  # exist yet) with the global default!
   path_globs_with_match_errors = [
     pg.with_match_error_behavior(glob_match_error_behavior)
     for pg in bundles_field.path_globs_list

src/python/pants/engine/legacy/structs.py

@@ -180,7 +180,7 @@ def _construct_bundles_field(self):
       base_globs = BaseGlobs.from_sources_field(bundle.fileset, rel_root)
       # TODO: we want to have this field set from the global option --glob-expansion-failure, or
       # something set on the target. Should we move --glob-expansion-failure to be a bootstrap
-      # option?
+      # option? See #5864.
       path_globs = base_globs.to_path_globs(rel_root)
 
       filespecs_list.append(base_globs.filespecs)

src/python/pants/option/global_options.py

@@ -21,6 +21,11 @@
 
 
 class GlobMatchErrorBehavior(datatype(['failure_behavior'])):
+  """Describe the action to perform when matching globs in BUILD files to source files.
+
+  NB: this object is interpreted from within Snapshot::lift_path_globs() -- that method will need to
+  be aware of any changes to this object's definition.
+  """
 
   IGNORE = 'ignore'
   WARN = 'warn'
@@ -37,23 +42,15 @@ class GlobMatchErrorBehavior(datatype(['failure_behavior'])):
   @classmethod
   @memoized_method
   def _singletons(cls):
-    ret = {}
-    for idx, behavior in enumerate(cls.ranked_values):
-      ret[behavior] = {
-        'singleton_object': cls(behavior),
-        # FIXME(cosmicexplorer): use this 'priority' field to implement a method of per-target
-        # selection of this behavior as well as the command-line.
-        'priority': idx,
-      }
-    return ret
+    return { behavior: cls(behavior) for behavior in cls.ranked_values }
 
   @classmethod
   def create(cls, value=None):
     if isinstance(value, cls):
       return value
     if not value:
       value = cls.default_value
-    return cls._singletons()[value]['singleton_object']
+    return cls._singletons()[value]
 
 
 class GlobalOptionsRegistrar(SubsystemClientMixin, Optionable):
@@ -278,8 +275,7 @@ def register_options(cls, register):
              help='Use a global lock to exclude other versions of pants from running during '
                   'critical operations.')
     # TODO(cosmicexplorer): Make a custom type abstract class to automate the production of an
-    # option with specific allowed values from a datatype. Also consider whether "ranking" as in
-    # GlobMatchErrorBehavior is something that should be generalized.
+    # option with specific allowed values from a datatype.
     register('--glob-expansion-failure', type=str,
              choices=GlobMatchErrorBehavior.ranked_values,
              default=GlobMatchErrorBehavior.default_option_value,

src/rust/engine/fs/src/lib.rs

@@ -244,16 +244,18 @@ impl PathGlob {
   }
 
   pub fn create(filespecs: &[String]) -> Result<Vec<PathGlob>, String> {
+    // Getting a Vec<PathGlob> per filespec is needed to create a `PathGlobs`, but we don't need
+    // that here.
     let filespecs_globs = Self::spread_filespecs(filespecs)?;
     let all_globs = Self::flatten_entries(filespecs_globs);
     Ok(all_globs)
   }
 
-  pub fn flatten_entries(entries: Vec<PathGlobIncludeEntry>) -> Vec<PathGlob> {
+  fn flatten_entries(entries: Vec<PathGlobIncludeEntry>) -> Vec<PathGlob> {
     entries.into_iter().flat_map(|entry| entry.globs).collect()
   }
 
-  pub fn spread_filespecs(filespecs: &[String]) -> Result<Vec<PathGlobIncludeEntry>, String> {
+  fn spread_filespecs(filespecs: &[String]) -> Result<Vec<PathGlobIncludeEntry>, String> {
     let mut spec_globs_map = Vec::new();
     for filespec in filespecs {
       let canonical_dir = Dir(PathBuf::new());
@@ -493,7 +495,6 @@ pub enum GlobMatch {
 
 #[derive(Clone, Debug)]
 struct GlobExpansionCacheEntry {
-  // We could add `PathStat`s here if we need them for checking matches more deeply.
   globs: Vec<PathGlob>,
   matched: GlobMatch,
   sources: Vec<GlobSource>,
@@ -510,7 +511,6 @@ pub struct SingleExpansionResult {
 struct PathGlobsExpansion<T: Sized> {
   context: T,
   // Globs that have yet to be expanded, in order.
-  // TODO: profile/trace to see if this affects perf over a Vec.
   todo: Vec<GlobWithSource>,
   // Paths to exclude.
   exclude: Arc<GitignoreStyleExcludes>,
@@ -930,7 +930,7 @@ pub trait VFS<E: Send + Sync + 'static>: Clone + Send + Sync + 'static {
             .push(source);
 
           // TODO(cosmicexplorer): is cloning these so many times as `GlobSource`s something we can
-          // bypass by using `Arc`s?
+          // bypass by using `Arc`s? Profile first.
           let source_for_children = GlobSource::ParentGlob(path_glob);
           for child_glob in globs {
             if expansion.completed.contains_key(&child_glob) {
@@ -1009,7 +1009,8 @@ pub trait VFS<E: Send + Sync + 'static>: Clone + Send + Sync + 'static {
           .collect();
 
         if !non_matching_inputs.is_empty() {
-          // TODO: explain what global option to set to modify this behavior!
+          // TODO(cosmicexplorer): explain what global and/or target-specific option to set to
+          // modify this behavior! See #5864.
           let msg = format!(
             "Globs did not match. Excludes were: {:?}. Unmatched globs were: {:?}.",
             exclude.exclude_patterns(),
@@ -1043,11 +1044,6 @@ pub trait VFS<E: Send + Sync + 'static>: Clone + Send + Sync + 'static {
     sourced_glob: GlobWithSource,
     exclude: &Arc<GitignoreStyleExcludes>,
   ) -> BoxFuture<SingleExpansionResult, E> {
-    // If we were to match on &path_glob (which I just tried), it seems like it's impossible to move
-    // something while borrowing it, even if the move happens at the end of the borrowed scope. That
-    // seems like it doesn't need to happen? I think there is no way to avoid the clone here,
-    // however, because directory_listing() returns a future and therefore can use only static
-    // references.
     match sourced_glob.path_glob.clone() {
       PathGlob::Wildcard { canonical_dir, symbolic_path, wildcard } =>
         // Filter directory listing to return PathStats, with no continuation.

src/rust/engine/src/core.rs

@@ -77,6 +77,7 @@ pub struct Key {
   type_id: TypeId,
 }
 
+// This is consumed in user-facing error messages, such as for --glob-expansion-failure=error.
 impl fmt::Debug for Key {
   fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
     write!(f, "Key(val={:?})", externs::key_to_str(self))
@@ -109,10 +110,6 @@ impl Key {
   pub fn type_id(&self) -> &TypeId {
     &self.type_id
   }
-
-  pub fn to_string(&self) -> String {
-    externs::key_to_str(&self)
-  }
 }
 
 ///