Skip to content

Conversation

@blyxyas
Copy link
Member

@blyxyas blyxyas commented May 6, 2023

Fixes #10742
changelog: Fix false negative where option_env_unwrap wouldn't warn if the env variable isn't set at compile-time.

@rustbot
Copy link
Collaborator

rustbot commented May 6, 2023

r? @flip1995

(rustbot has picked a reviewer for you, use r? to override)

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label May 6, 2023
@blyxyas
Copy link
Member Author

blyxyas commented May 6, 2023

Note that I needed to undo some "stylistic choices" (remove the if_chain!) because Rust pattern matching won't let you have 2 patterns with different types in the same if let statement.

That means that the following won't compile:

if let ExprKind::Call(caller, _) | ExprKind::Path(_, caller) = receiver.kind;

Because Call's caller and Path's caller are different types.

@blyxyas
Copy link
Member Author

blyxyas commented Jul 26, 2023

As @flip1995 is pretty busy, I'll let rustbot choose another reviewer.

r? rust-lang/clippy

@rustbot rustbot assigned matthiaskrgr and unassigned flip1995 Jul 26, 2023
fn main() {
let _ = option_env!("PATH").unwrap();
let _ = option_env!("PATH").expect("environment variable PATH isn't set");
let _ = option_env!("Y").unwrap(); // This test only works if you don't have a __Y__do_not_use env variable in your environment.
Copy link
Member

Choose a reason for hiding this comment

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

Is the comment outdated and should be

Suggested change
let _ = option_env!("Y").unwrap(); // This test only works if you don't have a __Y__do_not_use env variable in your environment.
let _ = option_env!("Y").unwrap(); // This test only works if you don't have a Y env variable in your environment.

@blyxyas blyxyas force-pushed the unset_opt_env_unwrap branch from f873e7c to 4e1db44 Compare July 26, 2023 16:58
Comment on lines 51 to 55
if let ExprKind::Call(caller, _) = &receiver.kind && is_direct_expn_of(caller.span, "option_env").is_some() {
lint(cx, expr.span);
} else if let ExprKind::Path(_, caller) = &receiver.kind && is_direct_expn_of(caller.span, "option_env").is_some() {
lint(cx, expr.span);
}
Copy link
Member

Choose a reason for hiding this comment

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

Can you add 2 short comments here, what cases are covered by Call vs Path?

@flip1995
Copy link
Member

@bors r+

Thanks!

@bors
Copy link
Contributor

bors commented Jul 27, 2023

📌 Commit 3bfccac has been approved by flip1995

It is now in the queue for this repository.

@bors
Copy link
Contributor

bors commented Jul 27, 2023

⌛ Testing commit 3bfccac with merge 295bdc0...

@bors
Copy link
Contributor

bors commented Jul 27, 2023

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: flip1995
Pushing 295bdc0 to master...

@bors bors merged commit 295bdc0 into rust-lang:master Jul 27, 2023
@blyxyas blyxyas deleted the unset_opt_env_unwrap branch October 5, 2023 09:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

S-waiting-on-review Status: Awaiting review from the assignee but also interested parties

Projects

None yet

Development

Successfully merging this pull request may close these issues.

option_env_unwrap doesn't warn if the env variable isn't set

5 participants