Merge #217

217: Fixed toolchain version drift, improved error handling, config loading and refactoring. r=xFrednet a=nitkach

### This PR solves several problems

## Problem 1

After resolving this issue #188, this problem was revealed.
I didn't create an issue, I just fixed it in this PR.

When `cargo` of the new version tries to call `rustc` of the old version and pass it arguments that it does not support, the program breaks.
This happens when executing a command in `cargo_metadata_command()`.

Description of the situation in the diagram:

![toolchain-fiasco](https://github.com/nitkach/marker/assets/108859698/65be44f1-19f7-4881-8c1c-d8294db74c39)

Inside `MetadataCommand::new()` creates `cargo metadata` which calls `rustc`.
It, in turn, takes the toolchain from `PATH="~/.rustup/toolchains/1.65.0-*"`

## Problem 2
Also in repository with 1.65.0 in rust-toolchain file the following error occurs:
```
$ cargo marker

Compiling Lints:
error: the `-Z` flag is only accepted on the nightly channel of Cargo, but this is the `stable` channel
See https://doc.rust-lang.org/book/appendix-07-nightly-rust.html for more information about Rust release channels.
Error: LintCrateBuildFail
```
<!-- Reproduces if there is no `cargo_marker` in the current `Cargo.toml` dependencies.
The `PATH` additionally specifies the path to `cargo-marker`. -->

Diagram of command calls and environment variables:

![toolchain-unstable](https://github.com/nitkach/marker/assets/108859698/aeec07b2-c6a9-44c9-a0b6-46c8f0260f82)

The command `cargo build ... -Z unstable-options ...` uses a stable toolchain, despite the specified `RUSTUP_TOOLCHAIN=nightly-2023-07-03`.

Then `rustc` is called, the stable toolchain is taken from `PATH` and a command execution error occurs.
Cannot use the arguments available on the nightly version of `cargo` in the stable channel.

---

To fix this, I changed the `cargo_command()` of `Toolchain`.
Now the
```
rustup run {toolchain} cargo
```
is being created.
This makes `rustup` a proxy for `cargo`, guaranteed to call `cargo` with the specified `toolchain`.

---

### Cargo workspace manifest location

Now the location of the workspace manifest file is obtained by executing the `cargo_locate_project` method on the `Cargo` type.
The `rustup run {toolchain} cargo locate-project --workspace` command is executed.

---

#### Error handling
I have plan to refactor error handling in future PR.
I have removed the error codes from `ExitStatus`, as they do not work as I assume. In the terminal, it always shows that `(exit code: 1)` is returned.

https://github.com/nitkach/marker/blob/b95435d77065210bbe38e6995f35bd165f280612/cargo-marker/src/exit.rs#L81-L83
Adding the `Fatal` variant with a description and the source of the error makes creating errors easier.

---

#### Parsing TOML as strongly typed data structures
For parsing `toml`, I added the use of rust types, which deserializes the document into a structure with fields.

---

#### `Display` error for toml file while parsing

I also noticed that it is possible to output errors in more readable way

`Debug`:
```
Can't parse config: Error { inner: Error { inner: TomlError { message: "data did not match any variant of untagged enum LintDependency", original: Some("[workspace]\nmembers = [\n  \"cargo-marker\",\n  \"marker_adapter\",\n  \"marker_api\",\n  \"marker_rustc_driver\",\n  \"marker_utils\",\n  \"marker_lints\",\n  \"marker_uitest\",\n  \"marker_uilints\",\n]\nresolver = \"2\"\n\n[workspace.metadata.marker.lints]\nmarker_lints = { foo = \"./marker_lints\" }\n"), keys: ["workspace", "metadata", "marker", "lints", "marker_lints"], span: Some(245..271) } } }
```
`Display`:
```
Can't parse config: TOML parse error at line 15, column 16
   |
15 | marker_lints = { foo = "./marker_lints" }
   |                ^^^^^^^^^^^^^^^^^^^^^^^^^^
data did not match any variant of untagged enum LintDependency
```

---

#### Some code changes

I'm learning how to use iterators. Where the `mut` variable and the `for` loop were used, I replaced that with iterators.
In some places, an immutable `Vec<T>` was created, which I changed to an `[T]`.


Co-authored-by: Nikita <[email protected]>

cargo-marker/Cargo.toml

@@ -10,15 +10,17 @@ license     = "MIT OR Apache-2.0"
 repository  = "https://github.com/rust-marker/marker"
 
 # Crate names in Rust are fun. I reserved `cargo_marker` as a crate name. However,
-# Cargo requires it's subcommands to use a dash like `cargo-marker`. Unable to rename
+# Cargo requires it's subcommands to use a dash like `cargo-marker`. Unable to
 # rename the create on crates.io we now have this hack... At least it works
 [[bin]]
 name = "cargo-marker"
 path = "src/main.rs"
 
 [dependencies]
+camino         = { version = "1.1", features = ["serde1"] }
 cargo_metadata = "0.15.4"
 clap           = { version = "4.0", features = ["string", "derive"] }
 once_cell      = "1.17.0"
 serde          = { version = "1.0", features = ["derive"] }
-toml           = { version = "0.7" }
+serde_json     = "1.0"
+toml           = "0.7"

cargo-marker/src/backend.rs

@@ -14,6 +14,7 @@ use crate::{config::LintDependencyEntry, ExitStatus};
 
 use self::{lints::LintCrate, toolchain::Toolchain};
 
+pub mod cargo;
 pub mod driver;
 pub mod lints;
 pub mod toolchain;
@@ -37,8 +38,6 @@ pub struct Config {
     pub build_rustc_flags: String,
     /// Indicates if this is a release or debug build.
     pub debug_build: bool,
-    /// Indicates if this is a development build.
-    pub dev_build: bool,
     pub toolchain: Toolchain,
 }
 
@@ -49,7 +48,6 @@ impl Config {
             lints: HashMap::default(),
             build_rustc_flags: String::new(),
             debug_build: false,
-            dev_build: cfg!(feature = "dev-build"),
             toolchain,
         })
     }
@@ -78,7 +76,7 @@ pub fn prepare_check(config: &Config) -> Result<CheckInfo, ExitStatus> {
         ("RUSTC_WORKSPACE_WRAPPER", config.toolchain.driver_path.as_os_str().to_os_string()),
         ("MARKER_LINT_CRATES", to_marker_lint_crates_env(&lints)),
     ];
-    if let Some(toolchain) = &config.toolchain.toolchain {
+    if let Some(toolchain) = &config.toolchain.cargo.toolchain {
         env.push(("RUSTUP_TOOLCHAIN", toolchain.into()));
     }
 

cargo-marker/src/backend/cargo.rs

@@ -0,0 +1,80 @@
+use std::process::Command;
+
+use camino::Utf8PathBuf;
+use cargo_metadata::MetadataCommand;
+use serde::Deserialize;
+
+use crate::ExitStatus;
+
+#[derive(Debug, Default)]
+pub struct Cargo {
+    /// The rustc toolchain this driver belongs to. This can be `None` during
+    /// execution commands such as `cargo locate-project`
+    pub(crate) toolchain: Option<String>,
+}
+
+#[derive(Deserialize, Debug)]
+struct ProjectLocation {
+    root: Utf8PathBuf,
+}
+
+impl Cargo {
+    pub fn with_toolchain(toolchain: impl Into<String>) -> Self {
+        Self {
+            toolchain: Some(toolchain.into()),
+        }
+    }
+
+    /// This returns a command calling rustup, which acts as a proxy for the
+    /// Cargo of the selected toolchain.
+    /// It may add additional flags for verbose output.
+    ///
+    /// See also [`super::toolchain::Toolchain::cargo_build_command`] if the
+    /// commands is intended to build a crate.
+    pub fn command(&self) -> Command {
+        // Marker requires rustc's shared libraries to be available. These are
+        // added by rustup, when it acts like a proxy for cargo/rustc/etc.
+        // This also means that cargo needs to be invoked via rustup, to have
+        // the libraries available when Marker is invoked. This is such a mess...
+        // All of this would be so, so much simpler if marker was part of rustup :/
+        if let Some(toolchain) = &self.toolchain {
+            let mut cmd = Command::new("rustup");
+            cmd.args(["run", toolchain, "cargo"]);
+            cmd
+        } else {
+            // for cargo locate-project - this command can be run without
+            // specified toolchain
+            Command::new("cargo")
+        }
+    }
+
+    pub fn cargo_locate_project(&self) -> Result<Utf8PathBuf, ExitStatus> {
+        let mut cmd = self.command();
+
+        let output = cmd
+            .arg("locate-project")
+            .arg("--workspace")
+            .output()
+            .map_err(|err| ExitStatus::fatal(err, "error locating workspace manifest Cargo.toml"))?;
+
+        if !output.status.success() {
+            let stderr = String::from_utf8_lossy(&output.stderr);
+            return Err(ExitStatus::fatal(
+                stderr.trim(),
+                format!("cargo locate-project failed with {}", output.status),
+            ));
+        }
+
+        let manifest_location: ProjectLocation = serde_json::from_slice(&output.stdout)
+            .map_err(|err| ExitStatus::fatal(err, "failed to deserialize cargo locate-project output"))?;
+
+        Ok(manifest_location.root)
+    }
+
+    // Keep self for future changes. It's implemented in such way that clippy
+    // doesn't ask to write it as an associative function.
+    #[allow(clippy::unused_self)]
+    pub fn metadata(&self) -> MetadataCommand {
+        MetadataCommand::new()
+    }
+}

cargo-marker/src/backend/driver.rs

@@ -2,7 +2,7 @@ use std::{path::Path, process::Command, str::from_utf8};
 
 use once_cell::sync::Lazy;
 
-use crate::ExitStatus;
+use crate::{utils::is_local_driver, ExitStatus};
 
 use super::toolchain::{get_toolchain_folder, rustup_which, Toolchain};
 
@@ -125,7 +125,7 @@ fn install_toolchain(toolchain: &str) -> Result<(), ExitStatus> {
 
 /// This tries to compile the driver.
 fn build_driver(toolchain: &str, version: &str, additional_rustc_flags: &str) -> Result<(), ExitStatus> {
-    if cfg!(debug_assertions) {
+    if is_local_driver() {
         println!("Compiling rustc driver");
     } else {
         println!("Compiling rustc driver v{version} with {toolchain}");
@@ -135,7 +135,7 @@ fn build_driver(toolchain: &str, version: &str, additional_rustc_flags: &str) ->
 
     // Build driver
     let mut cmd = Command::new("cargo");
-    if cfg!(debug_assertions) {
+    if is_local_driver() {
         cmd.args(["build", "--bin", "marker_rustc_driver"]);
     } else {
         cmd.env("RUSTUP_TOOLCHAIN", toolchain);

cargo-marker/src/backend/lints/fetch.rs

@@ -114,7 +114,7 @@ const DUMMY_MAIN_CONTENT: &str = r#"
 "#;
 
 fn call_cargo_fetch(manifest: &Path, config: &Config) -> Result<(), ExitStatus> {
-    let mut cmd = config.toolchain.cargo_command();
+    let mut cmd = config.toolchain.cargo.command();
     cmd.arg("fetch");
     cmd.arg("--manifest-path");
     cmd.arg(manifest.as_os_str());
@@ -141,7 +141,7 @@ fn call_cargo_fetch(manifest: &Path, config: &Config) -> Result<(), ExitStatus>
 }
 
 fn call_cargo_metadata(manifest: &PathBuf, config: &Config) -> Result<Metadata, ExitStatus> {
-    let res = config.toolchain.cargo_metadata_command().manifest_path(manifest).exec();
+    let res = config.toolchain.cargo.metadata().manifest_path(manifest).exec();
 
     // FIXME(xFrednet): Handle errors properly.
     res.map_err(|_| ExitStatus::LintCrateFetchFailed)