unstable proc_macro tracked::* rename/restructure

[drahnr]

Extracted restructure of the unstable tracked::{fs,env}::*

Originally part of #84029

It's slightly clunky to to have three distinct unstable features, since one cannot use two annotations on one mod tracked {..} declaration.

The second thing I am unsure is rust-analyzer which would need some adaption for the new structure.

CC @Xanewok

[rust-highfive]

r? @petrochenkov

(rust-highfive has picked a reviewer for you, use r? to override)

[petrochenkov]

I'm going to transfer this to the libs API team.
These are APIs that allow to trigger a rebuild when a certain environment variable or a file used from a proc macro change.

I'm not sure what the current library conventions are, but seeing >2 segment paths like tracked::fs::path in code is uncommon, so the new structure will likely result in the API being used through renamed imports like

use tracked::fs::path as track_path;
use tracked::env::var as tracked_env_var;

fn my_proc_macro() {
    track_path("my_file.txt"); // returns nothing
    let var = tracked_env_var("MY_VAR"); // returns the variable's value
}

[drahnr]

Totally understandable, and I don't have a strong opinion, mostly going off of what was discussed in the referenced #84029 issue. If it is at odds with the current naming and module nesting depth, feel free to close.

At the very least though, I think it should be unified to either track_ or tracked_ prefixes for consistency.

[joshtriplett]

We discussed this in today's @rust-lang/libs-api meeting. We don't want to see an extra level of submodule nesting here; we should just have tracked::path and tracked::env_var. We didn't find the parallel with std compelling, because this will never have any significant fraction of std, and of the two methods we already have, one (path) already has no parallel in std.

Also, separate from this issue, several people in the meeting felt that we shouldn't restrict path to just str. We understand that the metadata format currently can't handle non-UTF-8, but we'd still like to see this accept a path, to avoid hardcoding that limitation in the API. We can always internally raise an error if it's not UTF-8 and the metadata can't handle it.

[drahnr]

@m-ou-se I'll implement the required changes this week, just wasn't entirely sure what your self assignment implied.

[pksunkara]

so the new structure will likely result in the API being used through renamed imports like

Shouldn't we name the functions track_path and track_env_var then?

[drahnr]

so the new structure will likely result in the API being used through renamed imports like

Shouldn't we name the functions track_path and track_env_var then?

I'd prefer the one module level variants, which is in line with other macro impls iirc:

.. we should just have tracked::path and tracked::env_var ..

[drahnr]

Also, separate from this issue, several people in the meeting felt that we shouldn't restrict path to just str. We understand that the metadata format currently can't handle non-UTF-8, but we'd still like to see this accept a path, to avoid hardcoding that limitation in the API. We can always internally raise an error if it's not UTF-8 and the metadata can't handle it.

What's meant by internally raising an error, where should this be handled? Is path.to_str().expect() good enough? Encode does not provide means to return a result type. I'd appreciate some input.

[drahnr]

Pending access to something that can be cast to a byte slice, the impl currently accepts valid UTF-8 only. I am not sure the error handling is as desired. Either way, I'd like some feedback :) Thanks!

@rustbot label -S-waiting-on-author +S-waiting-on-review

[petrochenkov]

It's not necessary to return a Result, proc macro server can simply report a compiler error if the passed Path is non-UTF8.

I'm not sure whether proc macro bridge can support passing Path though.
If it cannot, then we could actually determine non-UTF8-ness on the client side and return a Result.

[drahnr]

It's not necessary to return a Result, proc macro server can simply report a compiler error if the passed Path is non-UTF8.

I'm not sure whether proc macro bridge can support passing Path though.
If it cannot, then we could actually determine non-UTF8-ness on the client side and return a Result.

Technically this would be possible, but that would require to access the currently private fn as_u8_slice and the unsafe fn from_u8_slice to be usable from the encode/decode impl.

I added an implementation adding a Result at the outer level, avoiding the encode and decode impls for PathBuf.

[JohnCSimon]

Ping from triage:
@drahnr - can you please post your status on this PR?

[m-ou-se]

Part of the reason why this is stuck, is because there's no tracking issue with an overview of the API, making it hard to quickly see what API we're talking about or what we're actually changing.

The existing proc_macro_tracked_env unstable attributes refer to #74690, and the track_path attributes refer to #73921, but neither is a tracking issue with an api overview and history, as in our tracking issue template.

I believe this is the current state:

// proc_macro::tracked_env:
pub fn var<K: AsRef<OsStr> + AsRef<str>>(key: K) -> Result<String, VarError>;

// proc_macro::tracked_path:
pub fn path<P: AsRef<str>>(path: P);

And it seems you're proposing to change it to:

// proc_macro::tracked:
pub fn env_var<K: AsRef<OsStr> + AsRef<str>>(key: K) -> Result<String, VarError>;
pub fn path<P: AsRef<Path>>(path: P) -> Result<(), ()>;

While there's some improvements, such as AsRef<Path> instead of AsRef<str>, I'm not sure how much this is an overall improvement. path returning a Result will rarely be useful, especially if the only error it can return is Err(()) holding no information. Putting env_var and path together in the same tracked module might also get a bit confusing, since env_var actually reads the env var and gives you its contents, while path doesn't give anything useful at all, and is only used to register a path for tracking, making them rather different kind of APIs.

Considering there still needs to be more design work done before this gets anywhere near stabilization, I'm hesitant to merge this.

As previously suggested by petrochenkov, it'd be good to first merge the change that changes AsRef<str> to AsRef<Path>, as that change is uncontroversial and involves changes in the bridge implementation.

Then the discussion on the exact public interface can be had separately. Either on a PR that clearly states the newly proposed API, or as as an API Change Proposal on the libs-team repo. Once we have a new design, we should probably track both path and env_var together in a single tracking issue, so we can keep track of the full overview and history.

[m-ou-se]

I've opened #99515 to track this feature and the steps that still need to be done.

[bors]

☔ The latest upstream changes (presumably #99058) made this pull request unmergeable. Please resolve the merge conflicts.

[Dylan-DPC]

@drahnr any updates on this pr?

[drahnr]

@drahnr any updates on this pr?

@Dylan-DPC If everything goes well, I can take another look at splitting things up next week~ish according along with #87173 (comment) and drop the remaining changes. I frankly forgot about the issue.

[Dylan-DPC]

@drahnr any updates on this?

[drahnr]

Back on it.

[rustbot]

Some changes occurred in src/tools/rust-analyzer

cc @rust-lang/rust-analyzer

[rustbot]

Some changes occurred in src/tools/clippy

cc @rust-lang/clippy

[bors]

☔ The latest upstream changes (presumably #118646) made this pull request unmergeable. Please resolve the merge conflicts.