fix: don't error on tail comma for some macro - #23134
Conversation
Example
---
```rust
const _: &str = env!("PATH",);
```
**Before this PR**
```
/* expand error: expected string literal */
```
**After this PR**
```rust
const _: &str = "/usr/bin:/bin";
```
|
|
||
| fn parse_string(tt: &tt::TopSubtree) -> Result<(Symbol, Span), ExpandError> { | ||
| let mut tt = TtElement::Subtree(tt.top_subtree(), tt.iter()); | ||
| let expect_literal = |span| ExpandError::other(span, "expected string literal"); |
There was a problem hiding this comment.
This shouldn't be here. parse_string() is shared to many macros, not just env!().
There was a problem hiding this comment.
These macros need to support trailing commas
The only difference is that env!() also has an optional additional parameter
This implementation is very clean.
Perhaps we can add a boolean flag that only allows tokens after commas in env!()?
There was a problem hiding this comment.
Yes they do, but they do not need to support a second parameter.
I'll leave it to you whether to accept a bool in this method or to make env!() parse by itself, but do note that we'll want to use the error message in env!() which complicates a shared implementation.
There was a problem hiding this comment.
but do note that we'll want to use the error message in
env!()
Note that in the current implementation, the variable is extended to "UNRESOLVED_ENV_VAR" when it does not exist.
If you want to maintain this behavior and support error strings, the signature of parse_string needs to be modified to ExpandResult<Option<(Symbol, Span)>>
This is not convenient to handle, and r-a has some false positives on env!().
Currently, the implementation of error quiet is better
There was a problem hiding this comment.
Fair enough, but I still do not want to parse two parameters in all macros. A boolean will be fine.
|
|
||
| fn parse_string(tt: &tt::TopSubtree) -> Result<(Symbol, Span), ExpandError> { | ||
| let mut tt = TtElement::Subtree(tt.top_subtree(), tt.iter()); | ||
| fn parse_string(tt: &tt::TopSubtree, rest: bool) -> Result<(Symbol, Span), ExpandError> { |
There was a problem hiding this comment.
Maybe call it allow_second_arg? This looks more descriptive.
Fixes #23132
Example
Before this PR
After this PR