Auto merge of #122663 - beetrees:non-unicode-env-error, r=TaKO8Ki

bors · bors · commit 7f84ede33d60 · 2024-04-01T05:18:51.000Z
Fix error message for `env!` when env var is not valid Unicode

Currently (without this PR) the `env!` macro emits an ```environment variable `name` not defined at compile time``` error when the environment variable is defined, but not a valid Unicode string. This PR introduces a separate more accurate error message, and a test to verify this behaviour.

For reference, before this PR, the new test would have outputted:
```
error: environment variable `NON_UNICODE_VAR` not defined at compile time
 --&gt; non_unicode_env.rs:2:13
  |
2 |     let _ = env!("NON_UNICODE_VAR");
  |             ^^^^^^^^^^^^^^^^^^^^^^^
  |
  = help: use `std::env::var("NON_UNICODE_VAR")` to read the variable at run time
  = note: this error originates in the macro `env` (in Nightly builds, run with -Z macro-backtrace for more info)

error: aborting due to 1 previous error
```

whereas with this PR, the test ouputs:
```
error: environment variable `NON_UNICODE_VAR` is not a valid Unicode string
 --&gt; non_unicode_env.rs:2:13
  |
2 |     let _ = env!("NON_UNICODE_VAR");
  |             ^^^^^^^^^^^^^^^^^^^^^^^
  |
  = note: this error originates in the macro `env` (in Nightly builds, run with -Z macro-backtrace for more info)

error: aborting due to 1 previous error
```
diff --git a/compiler/rustc_builtin_macros/messages.ftl b/compiler/rustc_builtin_macros/messages.ftl
@@ -114,6 +114,8 @@ builtin_macros_env_not_defined = environment variable `{$var}` not defined at co
     .cargo = Cargo sets build script variables at run time. Use `std::env::var({$var_expr})` instead
     .custom = use `std::env::var({$var_expr})` to read the variable at run time
 
+builtin_macros_env_not_unicode = environment variable `{$var}` is not a valid Unicode string
+
 builtin_macros_env_takes_args = `env!()` takes 1 or 2 arguments
 
 builtin_macros_expected_one_cfg_pattern = expected 1 cfg-pattern
diff --git a/compiler/rustc_builtin_macros/src/env.rs b/compiler/rustc_builtin_macros/src/env.rs
@@ -11,18 +11,19 @@ use rustc_expand::base::{DummyResult, ExpandResult, ExtCtxt, MacEager, MacroExpa
 use rustc_span::symbol::{kw, sym, Ident, Symbol};
 use rustc_span::Span;
 use std::env;
+use std::env::VarError;
 use thin_vec::thin_vec;
 
 use crate::errors;
 
-fn lookup_env<'cx>(cx: &'cx ExtCtxt<'_>, var: Symbol) -> Option<Symbol> {
+fn lookup_env<'cx>(cx: &'cx ExtCtxt<'_>, var: Symbol) -> Result<Symbol, VarError> {
     let var = var.as_str();
     if let Some(value) = cx.sess.opts.logical_env.get(var) {
-        return Some(Symbol::intern(value));
+        return Ok(Symbol::intern(value));
     }
     // If the environment variable was not defined with the `--env-set` option, we try to retrieve it
     // from rustc's environment.
-    env::var(var).ok().as_deref().map(Symbol::intern)
+    Ok(Symbol::intern(&env::var(var)?))
 }
 
 pub fn expand_option_env<'cx>(
@@ -39,7 +40,7 @@ pub fn expand_option_env<'cx>(
     };
 
     let sp = cx.with_def_site_ctxt(sp);
-    let value = lookup_env(cx, var);
+    let value = lookup_env(cx, var).ok();
     cx.sess.psess.env_depinfo.borrow_mut().insert((var, value));
     let e = match value {
         None => {
@@ -108,35 +109,43 @@ pub fn expand_env<'cx>(
 
     let span = cx.with_def_site_ctxt(sp);
     let value = lookup_env(cx, var);
-    cx.sess.psess.env_depinfo.borrow_mut().insert((var, value));
+    cx.sess.psess.env_depinfo.borrow_mut().insert((var, value.as_ref().ok().copied()));
     let e = match value {
-        None => {
+        Err(err) => {
             let ExprKind::Lit(token::Lit {
                 kind: LitKind::Str | LitKind::StrRaw(..), symbol, ..
             }) = &var_expr.kind
             else {
                 unreachable!("`expr_to_string` ensures this is a string lit")
             };
 
-            let guar = if let Some(msg_from_user) = custom_msg {
-                cx.dcx().emit_err(errors::EnvNotDefinedWithUserMessage { span, msg_from_user })
-            } else if is_cargo_env_var(var.as_str()) {
-                cx.dcx().emit_err(errors::EnvNotDefined::CargoEnvVar {
-                    span,
-                    var: *symbol,
-                    var_expr: var_expr.ast_deref(),
-                })
-            } else {
-                cx.dcx().emit_err(errors::EnvNotDefined::CustomEnvVar {
-                    span,
-                    var: *symbol,
-                    var_expr: var_expr.ast_deref(),
-                })
+            let guar = match err {
+                VarError::NotPresent => {
+                    if let Some(msg_from_user) = custom_msg {
+                        cx.dcx()
+                            .emit_err(errors::EnvNotDefinedWithUserMessage { span, msg_from_user })
+                    } else if is_cargo_env_var(var.as_str()) {
+                        cx.dcx().emit_err(errors::EnvNotDefined::CargoEnvVar {
+                            span,
+                            var: *symbol,
+                            var_expr: var_expr.ast_deref(),
+                        })
+                    } else {
+                        cx.dcx().emit_err(errors::EnvNotDefined::CustomEnvVar {
+                            span,
+                            var: *symbol,
+                            var_expr: var_expr.ast_deref(),
+                        })
+                    }
+                }
+                VarError::NotUnicode(_) => {
+                    cx.dcx().emit_err(errors::EnvNotUnicode { span, var: *symbol })
+                }
             };
 
             return ExpandResult::Ready(DummyResult::any(sp, guar));
         }
-        Some(value) => cx.expr_str(span, value),
+        Ok(value) => cx.expr_str(span, value),
     };
     ExpandResult::Ready(MacEager::expr(e))
 }
diff --git a/compiler/rustc_builtin_macros/src/errors.rs b/compiler/rustc_builtin_macros/src/errors.rs
@@ -458,6 +458,14 @@ pub(crate) enum EnvNotDefined<'a> {
     },
 }
 
+#[derive(Diagnostic)]
+#[diag(builtin_macros_env_not_unicode)]
+pub(crate) struct EnvNotUnicode {
+    #[primary_span]
+    pub(crate) span: Span,
+    pub(crate) var: Symbol,
+}
+
 #[derive(Diagnostic)]
 #[diag(builtin_macros_format_requires_string)]
 pub(crate) struct FormatRequiresString {
diff --git a/library/core/src/macros/mod.rs b/library/core/src/macros/mod.rs
@@ -1053,7 +1053,8 @@ pub(crate) mod builtin {
     ///
     /// If the environment variable is not defined, then a compilation error
     /// will be emitted. To not emit a compile error, use the [`option_env!`]
-    /// macro instead.
+    /// macro instead. A compilation error will also be emitted if the
+    /// environment variable is not a vaild Unicode string.
     ///
     /// # Examples
     ///
diff --git a/src/tools/run-make-support/src/lib.rs b/src/tools/run-make-support/src/lib.rs
@@ -19,7 +19,11 @@ pub fn tmp_dir() -> PathBuf {
 }
 
 fn handle_failed_output(cmd: &str, output: Output, caller_line_number: u32) -> ! {
-    eprintln!("command failed at line {caller_line_number}");
+    if output.status.success() {
+        eprintln!("command incorrectly succeeded at line {caller_line_number}");
+    } else {
+        eprintln!("command failed at line {caller_line_number}");
+    }
     eprintln!("{cmd}");
     eprintln!("output status: `{}`", output.status);
     eprintln!("=== STDOUT ===\n{}\n\n", String::from_utf8(output.stdout).unwrap());
diff --git a/src/tools/run-make-support/src/rustc.rs b/src/tools/run-make-support/src/rustc.rs
@@ -1,4 +1,5 @@
 use std::env;
+use std::ffi::OsStr;
 use std::path::Path;
 use std::process::{Command, Output};
 
@@ -133,6 +134,11 @@ impl Rustc {
         self
     }
 
+    pub fn env(&mut self, name: impl AsRef<OsStr>, value: impl AsRef<OsStr>) -> &mut Self {
+        self.cmd.env(name, value);
+        self
+    }
+
     // Command inspection, output and running helper methods
 
     /// Get the [`Output`][std::process::Output] of the finished `rustc` process.
@@ -153,6 +159,18 @@ impl Rustc {
         output
     }
 
+    #[track_caller]
+    pub fn run_fail(&mut self) -> Output {
+        let caller_location = std::panic::Location::caller();
+        let caller_line_number = caller_location.line();
+
+        let output = self.cmd.output().unwrap();
+        if output.status.success() {
+            handle_failed_output(&format!("{:#?}", self.cmd), output, caller_line_number);
+        }
+        output
+    }
+
     /// Inspect what the underlying [`Command`] is up to the current construction.
     pub fn inspect(&mut self, f: impl FnOnce(&Command)) -> &mut Self {
         f(&self.cmd);
diff --git a/tests/run-make/non-unicode-env/non_unicode_env.rs b/tests/run-make/non-unicode-env/non_unicode_env.rs
@@ -0,0 +1,3 @@
+fn main() {
+    let _ = env!("NON_UNICODE_VAR");
+}
diff --git a/tests/run-make/non-unicode-env/non_unicode_env.stderr b/tests/run-make/non-unicode-env/non_unicode_env.stderr
@@ -0,0 +1,10 @@
+error: environment variable `NON_UNICODE_VAR` is not a valid Unicode string
+ --> non_unicode_env.rs:2:13
+  |
+2 |     let _ = env!("NON_UNICODE_VAR");
+  |             ^^^^^^^^^^^^^^^^^^^^^^^
+  |
+  = note: this error originates in the macro `env` (in Nightly builds, run with -Z macro-backtrace for more info)
+
+error: aborting due to 1 previous error
+
diff --git a/tests/run-make/non-unicode-env/rmake.rs b/tests/run-make/non-unicode-env/rmake.rs
@@ -0,0 +1,14 @@
+extern crate run_make_support;
+
+use run_make_support::rustc;
+
+fn main() {
+    #[cfg(unix)]
+    let non_unicode: &std::ffi::OsStr = std::os::unix::ffi::OsStrExt::from_bytes(&[0xFF]);
+    #[cfg(windows)]
+    let non_unicode: std::ffi::OsString = std::os::windows::ffi::OsStringExt::from_wide(&[0xD800]);
+    let output = rustc().input("non_unicode_env.rs").env("NON_UNICODE_VAR", non_unicode).run_fail();
+    let actual = std::str::from_utf8(&output.stderr).unwrap();
+    let expected = std::fs::read_to_string("non_unicode_env.stderr").unwrap();
+    assert_eq!(actual, expected);
+}

Original file line number	Diff line number	Diff line change
`@@ -0,0 +1,3 @@`
	`1`	`+fn main() {`
	`2`	`+ let _ = env!("NON_UNICODE_VAR");`
	`3`	`+}`