rust: fix toolchain creation with rust-overlay

[sandydoo]

In the switch from fenix to rust-overlay in #1500, the final toolchain was not correctly patched. This resulted in runtime linker errors, where tools like clippy were linked against the wrong librustc.

PR #1983 was an attempt at adjusting our APIs to better suit the toolchain/profile design of rust-overlay.

This PR retains the API compatibility at the cost of using private APIs.

[cloudflare-workers-and-pages]

Deploying devenv with    Cloudflare Pages

Latest commit:	2c65a67
Status:	✅  Deploy successful!
Preview URL:	https://5d1581ad.devenv.pages.dev
Branch Preview URL:	https://fix-rust-overlay.devenv.pages.dev

View logs

[claude]

Claude finished @sandydoo's task —— View job

---

Code Review Complete

Reviewed PR #2005 for code quality, potential bugs, performance, security, and test coverage issues.

Todo List

• Read and analyze the changed rust.nix file
• Understand the toolchain creation fix
• Check for potential bugs or issues
• Evaluate code quality and best practices
• Assess security concerns
• Review test coverage implications
• Provide comprehensive feedback

Areas for Improvement

1. Private API Usage (High Priority)

Issue: The code explicitly uses a private API from rust-overlay (mkAggregated) at line 183.

# WARNING: private API
# Import the mkAggregated function.
mkAggregated = import (rust-overlay + "/lib/mk-aggregated.nix") {

Concerns:

• Private APIs can break without notice in upstream updates
• No guarantees about API stability
• Could lead to maintenance burden

Recommendation: Consider filing an issue upstream to expose this functionality publicly, or implement a more robust fallback mechanism.

2. Error Handling for Native Target (Medium Priority)

Issue: The native target extraction at line 203 assumes pkgs.stdenv.hostPlatform.rust.rustcTargetSpec exists.

nativeTarget = pkgs.stdenv.hostPlatform.rust.rustcTargetSpec;

Concerns:

• No validation that the native target exists in the manifest
• Could fail silently if the platform doesn't have a rust target spec
• Error would be cryptic if this field is missing

Recommendation: Add validation:

nativeTarget = pkgs.stdenv.hostPlatform.rust.rustcTargetSpec or (throw "Unable to determine native Rust target for platform ${pkgs.stdenv.hostPlatform.system}");

3. Component Resolution Logic (Medium Priority)

Issue: The component resolution logic at lines 222-232 is complex and could be simplified.

resolvedComponents = lib.map
  (c:
    let
      resolvedName =
        if builtins.elem c availableComponents then c
        else if builtins.elem "${c}-preview" availableComponents then "${c}-preview"
        else throw "Component '${c}' not found. Available: ${lib.concatStringsSep ", " availableComponents}";
    in
      cfg.toolchain.${c} or cfg.toolchain.${resolvedName} or toolchainComponents.${resolvedName}
  )
  cfg.components;

Concerns:

• The preview suffix fallback might mask component naming issues
• Multiple fallback paths make debugging difficult
• Could potentially select wrong components silently

Recommendation: Make the component resolution more explicit and add better error messages when components aren't found.

4. Attribute Access Safety (Low Priority)

Issue: Several places use or patterns that could benefit from more explicit error handling:

• Line 197: availableTargets = toolchain._manifest.pkg.rust-std.target or { };
• Line 198: allComponents = toolchain._components or { };
• Line 199: availableComponents = toolchain._manifest.profiles.complete or [ ];

Recommendation: Consider whether these fallbacks are appropriate or if they should fail more explicitly when manifest structure is unexpected.

5. Code Documentation (Low Priority)

Issue: The complex logic around target handling and component resolution could benefit from more detailed comments.

Recommendation: Add comments explaining:

• Why the native target must be included
• The difference between toolchainComponents and allComponents
• When the preview suffix fallback is needed

Security Assessment

• No immediate security concerns identified
• The use of private APIs from rust-overlay introduces supply chain considerations
• File path construction appears safe (using rust-overlay + concatenation)

Test Coverage Implications

• The native target handling changes require testing across different platforms
• Component resolution logic should be tested with various component configurations
• Error handling paths need test coverage, especially for invalid targets/components

Overall Assessment

The changes successfully address the linker errors by properly handling toolchain creation, but the reliance on private APIs and increased complexity introduce maintenance risks. The native target handling is a necessary improvement but could be more robust.

---