Eliminate some buggy unreachable!()s in expand_[option_]env()#159940
Open
bal-e wants to merge 2 commits into
Open
Eliminate some buggy unreachable!()s in expand_[option_]env()#159940bal-e wants to merge 2 commits into
unreachable!()s in expand_[option_]env()#159940bal-e wants to merge 2 commits into
Conversation
I've added the expected stderr (computed using the fixed version of the code). As of *this* commit, the last line causes an ICE.
rustbot
added
A-run-make
Collaborator
|
Thanks for the pull request, and welcome! The Rust Project is excited to review your changes, and you should hear from @dingxiangfei2009 (or someone else) some time within the next two weeks. Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (
Why was this reviewer chosen?The reviewer was selected based on:
|
This comment has been minimized.
This comment has been minimized.
bal-e
force-pushed
the
env-macro-unreachables
branch
from
July 26, 2026 05:36
17024a1 to
613074a
Compare
This comment has been minimized.
This comment has been minimized.
bal-e
force-pushed
the
env-macro-unreachables
branch
from
July 26, 2026 07:47
613074a to
1a53575
Compare
Author
|
Sorry about the spam, I should have checked the remaining tests. |
The right symbol is already available as `var`, extracted post macro expansion; just use that directly. I have also restructured the `expand_env()` control flow now that there is less shared code, reducing indentation. The primary change there is to use `var` instead of `*symbol`. `var` contains the unescaped name (whereas `symbol` used to store the contents of the original string literal), so `.escape_debug()` is used to re-escape it. The escaping no longer depends on the exact string literal used in the user input, which is quite nice IMO. This required storing `String`s in the diagnostics instead of `Symbol`s. I have added a FIXME to note the accidental double macro expansion occurring in `expand_env()`. It is not a problem, per se, just something that could be cleaned up in the future.
bal-e
force-pushed
the
env-macro-unreachables
branch
from
July 26, 2026 07:49
1a53575 to
4f5f05f
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
These
unreachable!()s had misleading messages (the value they were extracting was not produced byexpr_to_string()) and one of them could be triggered, see #159939. It turns out the value being extracted wasn't necessary at all, it was already computed elsewhere, so (aside from the added test case) it's a reduction in code.Closes #159939. Adds a test case for it too.
Commits can be reviewed individually.