Support globbing in imports #2376

neunenak · 2024-09-19T08:40:15Z

Implements shell-style globs in import statements using the Rust glob crate. Addresses #1641

casey

See comments. The main one is that we have to handle non-unicode paths, so we should look at a different glob crate.

casey · 2024-09-20T02:33:28Z

src/compiler.rs

@@ -75,14 +75,39 @@ impl Compiler {
              .join(Self::expand_tilde(&relative.cooked)?)
              .lexiclean();

-            if import.is_file() {
+            let import_base_str = import.to_string_lossy();


just is expected to work with non-unicode paths, so we can't do a lossy conversation here. I'd suggest checking out the globset crate, which I believe handles non-unicode paths.

globset unfortunately doesn't have a method to iterate over a file system entry and select only paths that match the glob, like the iterator that glob_with returns with the glob crate. Looking at how glob_with is implemented in glob, it seems like this code is a bit tricky to write and I'd rather avoid it (or if I did write that code, I think it might work out to just implementing path-globbing functionality from scratch, which is also undesirable).

There's also this crate: https://crates.io/crates/wax , which is utf-8 only but claims to reasonably handle sane non-utf-8 text internally, and also seems to have the file system iterator I want. It also hasn't been updated in a bit and claims to be experimental, so I'm kind of iffy about this crate.

Maybe I'm missing something obvious that would make this easier to do.

Ahh, that's lame. A concern about wax, any syntax that wax supports becomes part of the public-facing interface of just, and so can't be added and removed without a backwards-incompatible change. wax supports some pretty exotic syntax, like repetitions <a*/:0,> and alternatives {a?c,x?z,foo}, which I doubt would be incredibly useful for use in import paths.

Actually it looks like even globset expects to be given the glob as a string: https://docs.rs/globset/latest/globset/struct.GlobBuilder.html#method.new . Also see this 2019 comment from the maintainer of globset on glob: rust-lang/glob#78 (comment) and this globset issue: BurntSushi/ripgrep#1250 .

I'm starting to think that there's no principled way to support globbing on non-utf8 filesystems without basically submitting a patch to some upstream glob project to do it, but maybe few enough people are using these tools with non-utf8 globs that no one has previously run into it?

The to_string_lossy function turns any non-utf8 sequence into the unicode replacement character �. I wonder if there is some other function that could be called to try to convert a PathBuf into a meaningful Rust utf-8 string (maybe doing something smart with guessing the original encoding). The quick solution would be to move this conversion into the has_glob block, which would mean that people using glob imports and file paths with non-utf8 names would run into issues but at least no existing just behavior would break.

It's a bit lame, but we could just throw an error if you try to glob with a path which can't be converted to unicode. Non-unicode paths are very rare and basically just crazy, so not supporting them is probably fine, especially if they're only unsupported if you use a pretty niche feature.

I'm tempted to land this feature as unstable initially, both because of the unicode path issue, but also because there's some choice about which glob syntaxes we support, and it might be good to make compare them all and make a principled choice about what to support.

casey · 2024-09-20T02:34:50Z

src/compiler.rs

+              for import in import_paths {
+                let Ok(import) = import else { continue };
+
+                if import.is_file() {


I think a glob matching a directory should be an error. Just because we can backwards-compatibly make it not an error, but not backwards-compatibly turn it into an error.

casey#1641

neunenak · 2024-10-02T08:39:41Z

Alright, I gated glob imports behind unstable, went back to using the glob crate, and explicitly error out if conversion to a utf-8 native rust string doesn't work.

casey

Did some initial review, see comments.

casey · 2024-10-02T23:28:55Z

src/analyzer.rs

-            if let Some(absolute) = absolute {
-              stack.push(asts.get(absolute).unwrap());
+          Item::Import { absolute_paths, .. } => {
+            for p in absolute_paths {


Suggested change

for p in absolute_paths {

for path in absolute_paths {

casey · 2024-10-02T23:29:43Z

src/unstable_feature.rs

@@ -3,6 +3,7 @@ use super::*;
 #[derive(Copy, Clone, Debug, PartialEq, Ord, Eq, PartialOrd)]
 pub(crate) enum UnstableFeature {
  FormatSubcommand,
+  GlobImport,


Suggested change

GlobImport,

ImportPathGlob,

casey · 2024-10-02T23:30:01Z

src/unstable_feature.rs

@@ -11,6 +12,7 @@ impl Display for UnstableFeature {
  fn fmt(&self, f: &mut Formatter) -> fmt::Result {
    match self {
      Self::FormatSubcommand => write!(f, "The `--fmt` command is currently unstable."),
+      Self::GlobImport => write!(f, "Glob imports are currently unstable"),


Suggested change

Self::GlobImport => write!(f, "Glob imports are currently unstable"),

Self::ImportPathGlob => write!(f, "Globs in import paths are currently unstable."),

casey · 2024-10-02T23:30:33Z

src/parser.rs

@@ -365,7 +365,7 @@ impl<'run, 'src> Parser<'run, 'src> {
            let optional = self.accepted(QuestionMark)?;
            let (path, relative) = self.parse_string_literal_token()?;
            items.push(Item::Import {
-              absolute: None,
+              absolute_paths: vec![],


Suggested change

absolute_paths: vec![],

absolute_paths: Vec::new(),

(No real reason, I just like avoiding macros.)

casey · 2024-10-02T23:31:18Z

src/item.rs

@@ -7,7 +7,7 @@ pub(crate) enum Item<'src> {
  Assignment(Assignment<'src>),
  Comment(&'src str),
  Import {
-    absolute: Option<PathBuf>,
+    absolute_paths: Vec<PathBuf>,


I would actually just keep this called absolute. It's a vec of paths, so we know that they're paths and there's potentially more than one of them.

My argument for changing it is that "absolute" is a pretty general word out of context, so it's hard to guess what it might mean when you see it as a random property access somewhere in code. I don't feel super strongly about it however.

casey · 2024-10-02T23:37:05Z

src/compiler.rs

@@ -75,14 +86,65 @@ impl Compiler {
              .join(Self::expand_tilde(&relative.cooked)?)
              .lexiclean();

-            if import.is_file() {
+            let has_glob = import.as_os_str().as_encoded_bytes().contains(&b'*');


This is tricky because glob supports more meta-characters than *. It supports ?, *, [, !, and ]. I suppose we should check for all of them.

Can we assume that none of those characters will show up in a legitimate justfile? I think that's safe to assume for * but maybe not for characters like [ and ].

What if there was a slightly different version of the import keyword that signaled that shell globs might be used, or an argument to it? import glob "my/path/*.just" or something like that?

casey · 2024-10-02T23:39:48Z

src/compiler.rs

+                  })?;
+              let glob_options = glob::MatchOptions {
+                case_sensitive: true,
+                require_literal_separator: false,


I believe this should be true. If false, then / can be matched by *, which is the bash default and zsh default. ** can be used to match an arbitrary directory path, including *.

casey · 2024-10-02T23:41:27Z

src/compiler.rs

+              let import_base_str =
+                import
+                  .as_os_str()
+                  .to_str()


We can do to_str on the path without first converting to an OsStr.

casey · 2024-10-02T23:42:43Z

src/compiler.rs

+                  }
+                })?;
+              for import in import_paths {
+                let Ok(import) = import else { continue };


This swallows I/O errors. If we encounter an I/O error we should fail.

casey · 2024-10-02T23:43:20Z

src/compiler.rs

+                  });
+                }
+              }
+            } else if import.is_file() {


Is this circular import test duplicated with the one above?

It is, but I think that trying to de-duplicate those lines of code would add more complexity than it's worth, since the logic here is just constructing the CircularImport error if necessary and then manipulating some local variables.

casey · 2024-12-22T03:15:44Z

I keep coming back to this. I have these worries:

I worry about delegating glob parsing to a dependency we don't control, and thus may not understand the syntax.
I worry about the backwards compatibility impact on existing justfiles. Since although it is crazy to use */?/** in a path name, it does happen, and maybe especially on windows or GUI systems. This might also be extra confusing because the globbing character wouldn't appear in the path literal in the justfile.
I worry about the restriction that it doesn't work with non-unicode paths.

I wonder if maybe it would be better if there was some syntax to enable the glob at the import, and it was that syntax which was experimental, instead of a setting which turns it on for all imports

Something like:

glob import '*/mod.just'

neunenak force-pushed the import-glob branch 2 times, most recently from 55a246a to 94fd369 Compare September 19, 2024 09:02

neunenak marked this pull request as ready for review September 19, 2024 09:02

neunenak force-pushed the import-glob branch from cd7c5a5 to dedf056 Compare September 19, 2024 09:42

casey requested changes Sep 20, 2024

View reviewed changes

neunenak force-pushed the import-glob branch from bce3e52 to ee26676 Compare September 25, 2024 08:31

neunenak added 9 commits October 2, 2024 00:17

WIP globbing

80e65a4

casey#1641

glob crate

79f1e53

Work

715ddd1

remove unwrap

9197d45

globset

7c06b60

WIP globset work

1d346c9

globwalk crate

a7ce5b1

try globwalk

0531bb5

Work

0c03054

neunenak force-pushed the import-glob branch from 3e88a69 to 0c03054 Compare October 2, 2024 07:18

neunenak added 2 commits October 2, 2024 01:21

More work

329c7fc

UTF-8

c0b4023

neunenak requested a review from casey October 2, 2024 08:39

directory error case

440ee92

casey requested changes Oct 2, 2024

View reviewed changes

neunenak added 4 commits October 2, 2024 19:00

some PR feedback

12a06c8

Addl work

fdbc562

Glob err

c971456

More err

227586e

neunenak requested a review from casey October 4, 2024 21:39

neunenak added 3 commits October 6, 2024 00:56

Merge branch 'master' into import-glob

de881a4

Merge branch 'master' into import-glob

e0e2bee

Merge branch 'master' into import-glob

ec7c1d2

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support globbing in imports #2376

Support globbing in imports #2376

neunenak commented Sep 19, 2024 •

edited

Loading

casey left a comment

casey Sep 20, 2024

neunenak Sep 25, 2024

casey Sep 25, 2024

neunenak Sep 27, 2024

casey Sep 30, 2024

casey Sep 20, 2024

neunenak commented Oct 2, 2024

casey left a comment

casey Oct 2, 2024

casey Oct 2, 2024

casey Oct 2, 2024

casey Oct 2, 2024

casey Oct 2, 2024

neunenak Oct 3, 2024

casey Oct 2, 2024

neunenak Oct 3, 2024

neunenak Oct 3, 2024

casey Oct 2, 2024

casey Oct 2, 2024

casey Oct 2, 2024

casey Oct 2, 2024

neunenak Oct 4, 2024

casey commented Dec 22, 2024

	Self::GlobImport => write!(f, "Glob imports are currently unstable"),
	Self::ImportPathGlob => write!(f, "Globs in import paths are currently unstable."),

Support globbing in imports #2376

Are you sure you want to change the base?

Support globbing in imports #2376

Conversation

neunenak commented Sep 19, 2024 • edited Loading

casey left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

neunenak commented Oct 2, 2024

casey left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

casey commented Dec 22, 2024

neunenak commented Sep 19, 2024 •

edited

Loading