Skip to content

Fix error message for env! when env var is not valid Unicode #122663

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Merged
merged 1 commit into from
Apr 1, 2024

Conversation

beetrees
Copy link
Contributor

@beetrees beetrees commented Mar 17, 2024

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
 --> 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
 --> 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

@rustbot
Copy link
Collaborator

rustbot commented Mar 17, 2024

r? @TaKO8Ki

rustbot has assigned @TaKO8Ki.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Mar 17, 2024
@rustbot
Copy link
Collaborator

rustbot commented Mar 17, 2024

Some changes occurred in run-make tests.

cc @jieyouxu

# ignore-cross-compile

all:
(NOT_UTF8_VAR=$$(printf "\377") $(RUSTC) non_utf8_env.rs 2>&1 && exit 1 || exit 0) | diff - non_utf8_env.stderr
Copy link
Member

Choose a reason for hiding this comment

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

Is it possible to write this new run-make test in Rust instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jieyouxu Done

@beetrees beetrees force-pushed the non-unicode-env-error branch from 767bee7 to 9156164 Compare March 17, 2024 23:21
@rustbot
Copy link
Collaborator

rustbot commented Mar 17, 2024

The run-make-support library was changed

cc @jieyouxu

@bors
Copy link
Collaborator

bors commented Mar 27, 2024

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

@TaKO8Ki
Copy link
Member

TaKO8Ki commented Mar 28, 2024

@beetrees Could you resolve a conflict? It looks good to me basically.

@TaKO8Ki
Copy link
Member

TaKO8Ki commented Mar 30, 2024

@rustbot author

@rustbot rustbot 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 Mar 30, 2024
@beetrees beetrees force-pushed the non-unicode-env-error branch from 9156164 to 0bbaa25 Compare April 1, 2024 04:47
@beetrees
Copy link
Contributor Author

beetrees commented Apr 1, 2024

@TaKO8Ki Rebased

@rustbot review

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Apr 1, 2024
@TaKO8Ki
Copy link
Member

TaKO8Ki commented Apr 1, 2024

Thanks! @bors r+ rollup

@bors
Copy link
Collaborator

bors commented Apr 1, 2024

📌 Commit 0bbaa25 has been approved by TaKO8Ki

It is now in the queue for this repository.

@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-review Status: Awaiting review from the assignee but also interested parties. labels Apr 1, 2024
@bors
Copy link
Collaborator

bors commented Apr 1, 2024

⌛ Testing commit 0bbaa25 with merge 7f84ede...

@bors
Copy link
Collaborator

bors commented Apr 1, 2024

☀️ Test successful - checks-actions
Approved by: TaKO8Ki
Pushing 7f84ede to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Apr 1, 2024
@bors bors merged commit 7f84ede into rust-lang:master Apr 1, 2024
12 checks passed
@rustbot rustbot added this to the 1.79.0 milestone Apr 1, 2024
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (7f84ede): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
0.6% [0.6%, 0.6%] 1
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) 0.6% [0.6%, 0.6%] 1

Cycles

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
-1.5% [-1.5%, -1.5%] 1
All ❌✅ (primary) - - 0

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 669.457s -> 667.101s (-0.35%)
Artifact size: 315.74 MiB -> 315.69 MiB (-0.02%)

@beetrees beetrees deleted the non-unicode-env-error branch April 1, 2024 15:34
# for free to join this conversation on GitHub. Already have an account? # 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. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants