Move paths to lib

[azdavis]

These aren't really rust (analyzer) specific, and might be useful in other language servers.

If this seems ok in principle, we might want to

• factor out some rust-specific bits
• ensure they get published to crates.io
• tweak the names to allow for that
• update comments and docs

[bjorn3]

I think you will need to change the versions from 0.0.0 to 0.1.0.

[azdavis]

Ok, I did that and added descriptions in the Cargo.toml for each

[Veykril]

The crate name vfs is already taken so we would need to come up with a more unique name here :) paths surprisingly isn't, though I am inclined to pick something else here as well maybe

[azdavis]

What about prefix each with la_ like la_arena from lib? (Not sure if that makes sense since I don't know what la stands for)

Or ra_ as the prefix since they were from rust-analyzer.

Or we could try to be "clever".

[bors]

☔ The latest upstream changes (presumably #12502) made this pull request unmergeable. Please resolve the merge conflicts.

[Veykril]

the la_ prefix is a pun I believe. A ra_ prefix sounds plausible, though in that case we already publish these crates as ra_ap_vfs etc as we do with all other crates.

@matklad you might wanna have a say in this matter

[matklad]

Stuff in libs doesn’t and should have rust-analyzer specific naming. This can even be a litmus test for whether a thing should be in lib.

However, stuff in libs has to have meaningful semver API. By moving stuff to lib, we are committing to not releasing too many major versions, and to having footgun-less API.

vfs stuff doesn’t feel crates-io worthy to me. It would be great to have proper vfs implemented, but it has to be far more involved (see watchman). The hard part would be providing sound APIs which guarantee that no notification is missed, and provides anomaly-free view of a sequence of modifications.

paths potentially is crates.io worthy, but it’s API should be carefully audited for any oddities. And the name should be longer for crates.io.

“arena library” is indeed named after “el libro de arena” by Borges

[matklad]

Yeah, paths look fine, but we need:

• introduce error types for try-forms
• remove assert method: now that try-from is in prelude, and with custom error types, try-from().unwrap() would be a nice to use API
• remove deprecated exists method
• replace impl trait with named generics
• double check that we aren’t exposing any way to break invariants via normalize or something
• add top-level io ing function fn to_abs_path(path: &Path) -> io::Result<AbsPathBuf> which works via std::env::current_dir

And… maybe paths name is OK?

[matklad]

Important philosophical observation: the fact that something isn’t on crates.io does not preclude using it! The code is MIT licensed, so anyone is allowed and explicitly encouraged to vendor, fork, or publish in their name. This gives us freedom to make sure that, whatever we publish to crates.io, is actually good (makes a mental effort to ignore smol_str)

[azdavis]

introduce error types for try-forms

Would this be just a type whose only field is the non-absolute (or non-relative) path or pathbuf? Is it enough to just return the thing itself instead of having error types?

Adding error types would I think add 4 new types: Not{Relative, Absolute}Path{, Buf}.

Is the benefit that we could make them impl std::error::Error?

remove assert method: now that try-from is in prelude, and with custom error types, try-from().unwrap() would be a nice to use API

Ok, removed that

remove deprecated exists method

Ok, removed

replace impl trait with named generics

I didn't see any input (or output) impl Trait

double check that we aren’t exposing any way to break invariants via normalize or something

I think not. Some functions (like normalize) re-assert the invariants. Some just use unsafe. (Would it actually be memory unsafe to be wrong?)

add top-level io ing function fn to_abs_path(path: &Path) -> io::Result<AbsPathBuf> which works via std::env::current_dir

Ok, added

And… maybe paths name is OK?

Fine with me (:

A thought: would it be nice to have a wrapper function that returns Either<AbsPath(Buf), RelPath(Buf)>? Or a custom enum if we don't want to depend on either.

[azdavis]

Also, I walked back the changes to vfs{, -notify}

[matklad]

Is the benefit that we could make them impl std::error::Error?

Yeah, I do think we need to add 4 error types (or maybe two, NotAbsolute<&Path>). The benefits are:

• good error message on .unwrap() (which obviates the need for assert function. By itself assert is a good API, but holistically it isn't, as it violates the expectation that .try_into() is how you'd do any trivially failable conversion)
• future-proofness. I don't envision us changing anything about errors in the future, but for libraries it's just a good rule of thumb to make everything opaque
• impl std::error::Error, yeah

I agree that adding a bunch of new types feels like a needless chore, two thoughts here:

• I think that's just how Rust works. For example, Rust stdlib has a boat-load of similar small error types:

  • https://doc.rust-lang.org/stable/std/str/struct.Utf8Error.html
  • https://doc.rust-lang.org/stable/std/string/struct.FromUtf8Error.html

  they create extra burden for the author of the library, but I think they are pretty much invisible for the users, until they need them. So, to me this seems like an acceptable situation.

• This is exactly the kind of thing I am thinking about when considering ./lib. That's the tradeoff: if you need somtheing, you just put the simplest possible thing in ./crates. If someone else needs it, they can copy-paste and adjust as they see fit. I routinely copy-paste Cargo's path normalization and read2 code :D

  If you want the stuff to be generally reusable, that's work! You don't just dump the simplest thing to crates.io, you have an (voluntarily) obligation to your users to ensure:

  • correctness (there should not be odd behaviors in the API)
  • churn resilience (the API should be 1.0)

[matklad]

rust-analyzer/crates/paths/src/lib.rs

Line 133 in 7db7387

pub fn join(&self, path: impl AsRef<Path>) -> AbsPathBuf {

^^ that's the impl Trait

[matklad]

A thought: would it be nice to have a wrapper function that returns Either<AbsPath(Buf), RelPath(Buf)>? Or a custom enum if we don't want to depend on either.

Yeah, been considering this as well! I'd say this is not necessary, as in, API is fine without, and it can be added in a minor release. If there's a clear good solution, I'd do that. But I don't think there is -- either is a dependency, and the abs or rel itself should have Path APIs I suppose?

I am wondering if we could/should cannibalize Result here? Eg, for try-into I think we do want to have proper errors, but maybe something like this would make sense as well?

// top-level
fn classify_path(path: &Path) -> Result<&RelPath, &AbsPath>

[bors]

☔ The latest upstream changes (presumably #12681) made this pull request unmergeable. Please resolve the merge conflicts.

[bors]

☔ The latest upstream changes (presumably #12808) made this pull request unmergeable. Please resolve the merge conflicts.

[azdavis]

This hasn't seen much movement, and I've adapted the parts I need for my own purposes. Going to close due to lack of interest.