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

proc_macro: Add API for tracked access to environment variables #74653

Merged
merged 1 commit into from Jul 27, 2020

Conversation

petrochenkov
Copy link
Contributor

@petrochenkov petrochenkov commented Jul 22, 2020

Continuation of #71858.

proc_macro::tracked_env::var is similar to regular env::var called from a proc macro, except that it also adds the accessed variable to depinfo.

@rust-highfive
Copy link
Collaborator

@petrochenkov: no appropriate reviewer found, use r? to override

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 22, 2020
@petrochenkov
Copy link
Contributor Author

r? @eddyb @Mark-Simulacrum

@Mark-Simulacrum
Copy link
Member

❤️ Thank you for following through on this. cc @dtolnay to get libs sign-off on the unstable API.

Copy link
Member

@dtolnay dtolnay left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems fine. Please file a tracking issue.

Some notes to address here or otherwise add to the tracking issue to address before stabilizing:

  • Ideally the signature would match the function in std::env but I see that putting OsStr in depinfo is possibly hard. Don't filepaths go into depinfo though? Those are OS strings so what do we do there, and would it be possible to do that for here as well?

  • If there is a possibility we want to expose some other things from std::env, maybe proc_macro::tracked_env::var would be more appropriate to keep those together.

@petrochenkov
Copy link
Contributor Author

@dtolnay

Don't filepaths go into depinfo though?

They do, and they use lossy Path::display, so paths incompatible with UTF-8 are not treated correctly.

There's some discussion about non-UTF-8 cases in environment variables in #71858, looks like the general stance is that they don't matter.
FWIW, they can be added later as a new depinfo directive (e.g. # env-dep-bytes as opposed to existing # env-dep) with bytes printed in escaped form (like byte string literals).

Ideally the signature would match the function in std::env

I thought about the signature a bit, assuming we are keeping UTF-8, the alternatives to var: &str (which I've chosen as the simplest variant) could be:

pub fn tracked_env_var(var: impl AsRef<OsStr> + AsRef<str> + Copy) -> Result<String, VarError> {
    let value = env::var(var); // Need `Copy` to avoid consuming `var` here
    bridge::client::FreeFunctions::track_env_var(var, value.as_deref().ok());
    value
}

or

// Without `Copy`
pub fn tracked_env_var(var: impl AsRef<OsStr> + AsRef<str>) -> Result<String, VarError> {
    let var = &var.to_string(); // Have to convert into an owned string
    let value = env::var(var);
    bridge::client::FreeFunctions::track_env_var(var, value.as_deref().ok());
    value
}

The original env::var uses just impl AsRef<OsStr>.

@petrochenkov
Copy link
Contributor Author

If there is a possibility we want to expose some other things from std::env, maybe proc_macro::tracked_env::var would be more appropriate to keep those together.

Yeah, sounds reasonable.

Copy link
Member

@dtolnay dtolnay left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would feel more comfortable with var: impl AsRef<OsStr> + AsRef<str>. That leaves the possibility to remove AsRef<str> bound backward compatibly to match the std::env::var signature.

@petrochenkov petrochenkov added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 25, 2020
@petrochenkov
Copy link
Contributor Author

Updated.

  • The function is moved into a module (tracked_env_var -> tracked_env::var).
  • Tracking issue number is updated.
  • The signature is changed to impl AsRef<OsStr> + AsRef<str>.
    r? @dtolnay

@rust-highfive rust-highfive assigned dtolnay and unassigned eddyb Jul 26, 2020
Copy link
Member

@dtolnay dtolnay left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good!

@dtolnay
Copy link
Member

dtolnay commented Jul 26, 2020

@bors r+

@bors
Copy link
Contributor

bors commented Jul 26, 2020

📌 Commit 62c9fa9 has been approved by dtolnay

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jul 26, 2020
@bors
Copy link
Contributor

bors commented Jul 27, 2020

⌛ Testing commit 62c9fa9 with merge 1841fb9...

@bors
Copy link
Contributor

bors commented Jul 27, 2020

☀️ Test successful - checks-actions, checks-azure
Approved by: dtolnay
Pushing 1841fb9 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Jul 27, 2020
@bors bors merged commit 1841fb9 into rust-lang:master Jul 27, 2020
@rust-highfive
Copy link
Collaborator

📣 Toolstate changed by #74653!

Tested on commit 1841fb9.
Direct link to PR: #74653

💔 rls on windows: test-pass → build-fail (cc @Xanewok).
💔 rls on linux: test-pass → build-fail (cc @Xanewok).
💔 rustfmt on windows: test-pass → build-fail (cc @topecongiro @calebcartwright).
💔 rustfmt on linux: test-pass → build-fail (cc @topecongiro @calebcartwright).

rust-highfive added a commit to rust-lang-nursery/rust-toolstate that referenced this pull request Jul 27, 2020
Tested on commit rust-lang/rust@1841fb9.
Direct link to PR: <rust-lang/rust#74653>

💔 rls on windows: test-pass → build-fail (cc @Xanewok).
💔 rls on linux: test-pass → build-fail (cc @Xanewok).
💔 rustfmt on windows: test-pass → build-fail (cc @topecongiro @calebcartwright).
💔 rustfmt on linux: test-pass → build-fail (cc @topecongiro @calebcartwright).
@bors bors mentioned this pull request Jul 27, 2020
bors bot added a commit to rust-lang/rust-analyzer that referenced this pull request Oct 8, 2020
5651: Add track_env_var to the proc macro server r=kjeremy a=lnicola

See rust-lang/rust#74653.

Fixes #6054.
Fixes #5640, maybe.

Should be merged when 1.47 is released.

Proc macros still don't work for me, but it no longer crashes.



Co-authored-by: Laurențiu Nicola <lnicola@dend.ro>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants