Skip to content

fix: Avoid .unwrap() when running the discover command #20126

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Wilfred
Copy link
Contributor

@Wilfred Wilfred commented Jun 30, 2025

Previously, the following configuration in settings.json:

"rust-analyzer.workspace.discoverConfig": {
    "command": [
        "oops",
        "develop-json",
        "{arg}"
    ],
    "progressLabel": "rust-analyzer",
    "filesToWatch": [
        "BUCK",
        "TARGETS"
    ]
},

Would previously cause a crash in rust-analyzer:

thread 'LspServer' panicked at crates/rust-analyzer/src/main_loop.rs:776:84:
called `Result::unwrap()` on an `Err` value: Os { code: 2, kind: NotFound, message: "No such file or directory" }

Instead, log an error and proceed. This gives the user a useful hint as to what's broken, and ensures we have a basic rust-analyzer running.

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jun 30, 2025
@Wilfred Wilfred changed the title fix: Avoid .unwrap() when running the discover command. fix: Avoid .unwrap() when running the discover command Jun 30, 2025
Previously, the following configuration in settings.json:

    "rust-analyzer.workspace.discoverConfig": {
        "command": [
            "oops",
            "develop-json",
            "{arg}"
        ],
        "progressLabel": "rust-analyzer",
        "filesToWatch": [
            "BUCK",
            "TARGETS"
        ]
    },

Would previously cause a crash in rust-analyzer:

    thread 'LspServer' panicked at crates/rust-analyzer/src/main_loop.rs:776:84:
    called `Result::unwrap()` on an `Err` value: Os { code: 2, kind: NotFound, message: "No such file or directory" }

Instead, log an error and proceed. This gives the user a useful hint
as to what's broken, and ensures we have a basic rust-analyzer
running.
@Wilfred Wilfred force-pushed the no_unwrap_in_discover_projects branch from b034b22 to f20eec0 Compare June 30, 2025 14:00
@ChayimFriedman2
Copy link
Contributor

I feel like crashing is acceptable here? If we can't run the command, we can't discover the project, and r-a doesn't have a lot it can do.

The choice of the temp dir also feels arbitrary and may cause weird misbehaviors.

@flodiebold
Copy link
Member

We don't crash when e.g. cargo metadata fails, right?

@Wilfred
Copy link
Contributor Author

Wilfred commented Jun 30, 2025

Logging a specific error and r-a continuing in detached file mode does feel more friendly than just "rust-analyzer crashed five times, it will not restart" which is the current behaviour.

I noticed this when I'd configured the discover command with a local build, forgot about it, eventually cleaned up my build directory, and got very confused when my r-a setup broke.

@Wilfred
Copy link
Contributor Author

Wilfred commented Jun 30, 2025

Regarding the fallback directory, I considered / but temp_dir() has the advantage that it works on Windows too. I do think the spawn() failing is much more likely, but I figured I'd remove both .unwrap() calls for cleanliness.

@davidbarsky
Copy link
Contributor

My weak opinion here is that we should probably crash five times if the provided argument doesn't exist/buck2 did some some garbage collection on buck-out. Maybe panic with a more descriptive error message?

Don't get me wrong: I don't think the current state is good, but we could probably make this suck less with an .expect("Unable to run project discovery command") and get us 80% of the way there...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants