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

file_absolute! macro #478

Open
epage opened this issue Nov 8, 2024 · 8 comments
Open

file_absolute! macro #478

epage opened this issue Nov 8, 2024 · 8 comments
Labels
api-change-proposal A proposal to add or alter unstable APIs in the standard libraries I-libs-api-nominated Indicates that an issue has been nominated for discussion during a team meeting. T-libs-api

Comments

@epage
Copy link

epage commented Nov 8, 2024

Proposal

Problem statement

Being able to bless a test failure as expected and have the expected value updated (ie snapshot testing) is a big productivity boon. For example, the Cargo team just put in a lot of work to switch thousands of tests to snapshot testing (rust-lang/cargo#14039). Having that expected value inside the source code makes it easier to manage (rather than dealing with thousands of snapshots that may or may not be used anymore) and allows you to more easily inspect what condition is being tested. You can look at the reverse dependencies of snapbox, expect-test, and insta. The first two primarily work through inline snapshots. I can't say how often people use inline snapshots with insta.

Similarly, it is can be helpful to be able to look up resources relative to the source file, rather than relative to the manifest root (via CARGO_MANIFEST_DIR). These can include

Today this done by using file! (which is unspecified) and assuming it is relative to the workspace root (which isn't always true), and somehow discovering the workspace root. The "somehow" is usually a mixture of

The Cargo team considered adding a CARGO_RUSTC_CURRENT_DIR (rust-lang/cargo#12996) that would solve this problem but there were concerns over this existing purely for file! and file! being unspecified, causing us to push people towards relying on unspecified behavior.

Motivating examples or use cases

Normally, this will be abstracted away in a test like:

    #[test]
    fn test_no_infostring() {
        snapbox::assert_data_eq!(
            si!(r#"---
[dependencies]
time="0.1.25"
---
fn main() {}
"#),
            str![[r#"
[[bin]]
name = "test-"
path = [..]

[dependencies]
time = "0.1.25"

[package]
autobenches = false
autobins = false
autoexamples = false
autolib = false
autotests = false
build = false
edition = "2021"
name = "test-"

[profile.release]
strip = true

[workspace]

"#]]
        );
    }

This relies on macros like

#[macro_export]
macro_rules! str {
    [$data:literal] => { $crate::str![[$data]] };
    [[$data:literal]] => {{
        let position = $crate::data::Position {
            file: $crate::utils::current_rs!(),
            line: line!(),
            column: column!(),
        };
        let inline = $crate::data::Inline {
            position,
            data: $data,
        };
        inline
    }};
    [] => { $crate::str![[""]] };
    [[]] => { $crate::str![[""]] };
}

or

#[macro_export]
macro_rules! file {
    [_] => {{
        let path = $crate::data::generate_snapshot_path($crate::fn_path!(), None);
        $crate::Data::read_from(&path, None)
    }};
    [_ : $type:ident] => {{
        let format = $crate::data::DataFormat:: $type;
        let path = $crate::data::generate_snapshot_path($crate::fn_path!(), Some(format));
        $crate::Data::read_from(&path, Some($crate::data::DataFormat:: $type))
    }};
    [$path:literal] => {{
        let mut path = $crate::utils::current_dir!();
        path.push($path);
        $crate::Data::read_from(&path, None)
    }};
    [$path:literal : $type:ident] => {{
        let mut path = $crate::utils::current_dir!();
        path.push($path);
        $crate::Data::read_from(&path, Some($crate::data::DataFormat:: $type))
    }};
}

Solution sketch

Like file! provide a file_absolute! that fills the role of the above $crate::utils::current_rs!() (for which a $crate::utils::current_dir!() can also be made).

Likewise, we should probably make the expectations for file! more explicit

Open questions

  • How will this interact with trim-paths? We were thinking warn or error
  • How will this interact with the depinfo file? Unclear. If someone moves their directory, they will want it rebuilt. However, for CI caching where snapshots won't be used, they won't want it tracked.
  • Should file! be "unspecified, for display purposes only" or be expected to be relative to a certain location (barring trim paths)

Alternatives

Links and related work

What happens now?

This issue contains an API change proposal (or ACP) and is part of the libs-api team feature lifecycle. Once this issue is filed, the libs-api team will review open proposals as capability becomes available. Current response times do not have a clear estimate, but may be up to several months.

Possible responses

The libs team may respond in various different ways. First, the team will consider the problem (this doesn't require any concrete solution or alternatives to have been proposed):

  • We think this problem seems worth solving, and the standard library might be the right place to solve it.
  • We think that this probably doesn't belong in the standard library.

Second, if there's a concrete solution:

  • We think this specific solution looks roughly right, approved, you or someone else should implement this. (Further review will still happen on the subsequent implementation PR.)
  • We're not sure this is the right solution, and the alternatives or other materials don't give us enough information to be sure about that. Here are some questions we have that aren't answered, or rough ideas about alternatives we'd want to see discussed.
@epage epage added api-change-proposal A proposal to add or alter unstable APIs in the standard libraries T-libs-api labels Nov 8, 2024
@m-ou-se
Copy link
Member

m-ou-se commented Nov 18, 2024

We discussed this in last week's libs-api meeting. We're very hesitant to add absolute paths to the source files as a feature in the standard libarry.

As you mention above, it's an open question how this will work with the "trim paths" feature. It's quite possible that trim-paths will become a often used or even default behaviour in the future, so adding a macro that is incompatible with that (e.g. errors/warns), seems like a step in the wrong direction.

There seem to be other possible solutions that can solve your problem without relying on absolute paths being hardcoded in binaries, so we'd like to see more exporation in those directions. (E.g. getting the relevant information from cargo, as you mention above.)

@epage
Copy link
Author

epage commented Nov 18, 2024

There seem to be other possible solutions that can solve your problem without relying on absolute paths being hardcoded in binaries, so we'd like to see more exporation in those directions. (E.g. getting the relevant information from cargo, as you mention above.)

We previously explored these options already before I came here, including having rust-lang/cargo#13644 for stabilizing it. A core problem is that file! is unspecified and changing of it for trim paths could also cause problems, see rust-lang/cargo#13644 (comment)

If we could specify file!, including a way for callers to tell when its trimmed, then we might be able to bounce this back to Cargo. Is that something t-libs-api is open to?

CC @ehuss

@RalfJung
Copy link
Member

RalfJung commented Dec 1, 2024

We've had to add awkward work-arounds for this in rust-lang/rust#133533, which also turned out to be problematic so rust-lang/rust#132390 has to use a different work-around.

The current situation is that a libs-api member requested to not add a new cargo env var for this, and suggested file_absolute!. But then when file_absolute! is proposed, libs-api says they'd rather see cargo provide this information (I guess via an env var). Would be good to get some clarity from t-libs-api on how this should be resolved, since it is a real issue that's causing problems even for rustc development itself. :)

@RalfJung
Copy link
Member

RalfJung commented Dec 1, 2024

@rustbot label +I-libs-api-nominated

@rustbot rustbot added the I-libs-api-nominated Indicates that an issue has been nominated for discussion during a team meeting. label Dec 1, 2024
@joshtriplett
Copy link
Member

(Speaking for myself, not the Cargo or libs-api team.)

@RalfJung Sympathy for the net outcome of mixed messaging here.

The previously proposed approach on the Cargo side seems like it would have involved Cargo attempting to expose the current directory that rustc used as a base for file!(), while keeping the current behavior of that base changing depending on how the compiler was invoked. I don't think that would have been the right approach.

I don't think using absolute paths is the right approach either.

I would propose that we should have a "base" directory passed into rustc, so that cargo and other build tools can always consistently pass in a standard base (e.g. the crate root, even when in a workspace). And cases like Rust-for-Linux could (for instance) pass in the root of the kernel tree. We could then have a file_relative! or (naming is hard), as well as an include_str_relative!.

@programmerjake
Copy link
Member

well, if the path should be based on the passed-in base directory, how about file_based!()?

@dtolnay
Copy link
Member

dtolnay commented Dec 3, 2024

The previously proposed approach on the Cargo side seems like it would have involved Cargo attempting to expose the current directory that rustc used as a base for file!(), while keeping the current behavior of that base changing depending on how the compiler was invoked. I don't think that would have been the right approach.

"Cargo attempting to expose the current directory that rustc used as a base for file!()" is opposite to how I think about that proposal, for two reasons:

  • Rustc doesn't think in terms of choosing a base for file!(). It uses what paths Cargo tells it, already. If rustc is run as rustc path/to/file.rs then file!() is "path/to/file.rs". If rustc is run as rustc /absolute/file.rs then file!() is "/absolute/file.rs". Furthermore, if file contains include!($path2) or #[path = $path2] then inside that other file, file!() is formed from well-specifiably joining first file's path with $path2 — there continues to be no notion of a base path having to be chosen for the joined result to be relative to. If $path2 is absolute, it makes sense that file!() would end up being absolute.

  • When using mod in straightforward ways, Cargo is the thing that is most in charge of what paths look like, through its choice of current directory for rustc (whether that's workspace root or manifest root, whether it varies between local dependencies and crates.io registry dependencies, whether it is absolute). After factoring in weird include! or #[path] usage, the source code being compiled is the thing most in charge of what paths look like. Rustc is the thing least in charge — it is behaving as told by Cargo and by the code being compiled, in a well-specifiable (not necessarily currently well specified) way.

Judging based on rust-lang/cargo#13644 (comment), that stabilization stalled out on the fact that file!()'s behavior is not well specified. If we can fix that, I think the Cargo change is still my preferred approach than having the standard library/rustc provide a dedicated absolute path macro. (The other concern was trim-paths, which would just as much need to be resolved regarding file_absolute!, right?)

It makes sense to me that it would be up to Cargo to supply context about Cargo's choice of rustc execution directory because the expectation that we can bake a source's absolute path into compiled code (the snapbox::file! use case) isn't a thing that generalize well from Cargo to other users of rustc:

  • Bazel: rustc is always invoked inside a sandbox containing copied or symlinked sources, which is blown away as soon as rustc is done. So if you were to bake absolute paths to the sandboxed paths into an executable, then run it, it would observe that those paths never exist.

  • Buck: similar, but the paths would commonly be on a different machine, or even different OS, between the compilation and test execution.

  • Sccache: I am less familiar with this one but part of its point is sharing identical compilations across different projects, so putting absolute paths from one of those projects into the shared compiled result does not fly.

file_absolute!() would be effectively nonsense in each of these systems (it might as well expand to "/asdf/jkl"). Whereas env!("CARGO_RUSTC_CURRENT_DIR") generalizes better. For example Buck could arrange to set that in a meaningful way for third-party code like it does for $OUT_DIR and $CARGO_MANIFEST_DIR and other idioms used in crates.io.

@epage
Copy link
Author

epage commented Dec 4, 2024

As file! is a built-in, would specifying its behavior be an ACP, MCP, or both? I'd like to post something about that to have side-by-side with this as we work out what path to go down.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api-change-proposal A proposal to add or alter unstable APIs in the standard libraries I-libs-api-nominated Indicates that an issue has been nominated for discussion during a team meeting. T-libs-api
Projects
None yet
Development

No branches or pull requests

7 participants