Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Error on package.path shorthand #56

Open
Kyuuhachi opened this issue Mar 2, 2023 · 9 comments
Open

Error on package.path shorthand #56

Kyuuhachi opened this issue Mar 2, 2023 · 9 comments
Assignees
Labels
bug Something isn't working

Comments

@Kyuuhachi
Copy link

Current behavior

If I write package = { path = "../package" }, everything works just fine. But if I instead write package.path = "../package", it gives an error Error fetching crate. I think those two are supposed to be equivalent in TOML — cargo accepts it without complaining, at least.

Does the same if I write for example extend.version = "1.1.0", though that's less important since there's already a convenient shorthand for that.

Expected behavior

Treat the above two snippets as equivalent and don't error.

Neovim version

nvim --version
NVIM v0.8.2
Build type: Release
LuaJIT 2.1.0-beta3
Compiled by builduser

Features: +acl +iconv +tui
See ":help feature-compile"

   system vimrc file: "$VIM/sysinit.vim"
  fall-back for $VIM: "/usr/share/nvim"

Run :checkhealth for more info
@Kyuuhachi Kyuuhachi added the bug Something isn't working label Mar 2, 2023
@saecki
Copy link
Owner

saecki commented Mar 6, 2023

I'm a bit torn on this one. The way toml files are currently parsed is just a bunch of regexes. This works quite well actually, because we can make a lot of assumptions and be quite accommodating for invalid syntax like unclosed quotes or similar. The downside to it is that we can't realistically handle every syntactically correct way of specifying dependencies.
We could implement this additional case, but that would add complexity in many places. Maybe it is a common enough case though, that it would be worth supporting.
The "right" solution for this issue would be a lenient toml parser, that can handle any valid toml file you throw at it, and would combine the best of both worlds. A while ago I've dabbled a bit with writing one in rust, but that isn't far along, writing one in lua/teal, would be a bit of a pain and probably too slow to be a nice user experience.

Anyway, I'm currently quite busy and wouldn't have time to implement this myself.

@utkarshgupta137
Copy link

@saecki doesn't Rust already have a TOML parser? I mean if cargo can parse them, then you could just borrow the code?

@saecki
Copy link
Owner

saecki commented May 4, 2023

Yes indeed, there are a lot of TOML parsers. Unfortunately most of them wont be able to recover from errors (they are not lenient). As a result we wouldn't be able to recover any information as soon as one syntax error occurs.

@utkarshgupta137
Copy link

Yes, but most of the time we are dealing with valid TOMLs? Would it be possible to use a parser & fall back to your current logic if the parser fails? Even when inserting versions or features, the state would be like this:

serde = ""
# or
serde = { version = "1.0", features = [] }

@saecki
Copy link
Owner

saecki commented May 4, 2023

I would argue the opposite, most of the time we are dealing with invalid TOMLs. When editing a file, most of the time the state is incomplete, and only stop when we archived the wanted result, which is a syntactically valid file. Especially when editing running two parsers at a time would probably mean double the latency, which is undesirable.

@utkarshgupta137
Copy link

Idk, in my experience, the TOML itself is usually valid. Operations such as removing crates/features or adding features or changing versions shouldn't break the TOML. It would probably only break if you start from an empty line. I usually copy an existing line & then modify it, so for me, the TOML is rarely broken.
Anyway, would it make sense to use parser + regex fallback if not in insert mode & using only regex if in insert mode? I think it would be a pretty nice tradeoff.
The biggest issue for me right now is that crates.nvim continues showing an error in NeoTree, even if I close the file. Many times, I open it just to check if crates are included or required features are enabled or not. There is no way to get rid of it without restarting nvim, which takes 10-15 seconds because I have a workspace with 50K+ lines & 800+ deps.

@utkarshgupta137
Copy link

Here is the minimum reproduction of the issue I'm facing btw:

==> Cargo.toml <==
[workspace]
members = ["crates1", "crates2"]

[dependencies]
crates1 = { version="0.1", path = "crates1" }
crates2 = { version="0.1", path = "crates2" }

==> crate1 <==

==> crate1/Cargo.toml <==
[package]
name = "crate1"
version = "0.1.0"
edition = "2021"

# See more keys and their definitions at https://doc.rust-lang.org/cargo/reference/manifest.html

[dependencies]

==> crate1/src <==

==> crate1/src/lib.rs <==
pub fn add(left: usize, right: usize) -> usize {
    left + right
}

#[cfg(test)]
mod tests {
    use super::*;

    #[test]
    fn it_works() {
        let result = add(2, 2);
        assert_eq!(result, 4);
    }
}

==> crate2 <==

==> crate2/Cargo.toml <==
[package]
name = "crate2"
version = "0.1.0"
edition = "2021"

# See more keys and their definitions at https://doc.rust-lang.org/cargo/reference/manifest.html

[dependencies]
crate1 = { workspace=true, features = [] }

==> crate2/src <==

==> crate2/src/lib.rs <==
pub fn add(left: usize, right: usize) -> usize {
    left + right
}

#[cfg(test)]
mod tests {
    use super::*;

    #[test]
    fn it_works() {
        let result = add(2, 2);
        assert_eq!(result, 4);
    }
}

In crate2/Cargo.toml, I'm getting Error fetching crate for crate1. I'm assuming the issue is related to regex parsing too? If not, i can open a separate issue.

@saecki
Copy link
Owner

saecki commented May 5, 2023

I don't think this is related, could you move this to a separate issue?

@saecki
Copy link
Owner

saecki commented Dec 17, 2023

For anyone interested, I've been working on said parser lately.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

3 participants