make panictry! private to libsyntax
#56897
Merged
Conversation
This file contains 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
|
(rust_highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
|
The only hand-expanding is in rustdoc, where I think we do still want fatal errors. Uses in libsyntax_ext are replaced by |
|
☔ The latest upstream changes (presumably #56977) made this pull request unmergeable. Please resolve the merge conflicts. |
nikomatsakis
requested changes
Dec 19, 2018
There was a problem hiding this comment.
Very nice PR. I left some comments and suggested changes. Thoughts?
I might want to shift review off to @estebank or @petrochenkov though as they work more often in the parser.
| None => return DummyResult::expr(sp), | ||
| }; | ||
|
|
||
| if p2.token == token::Eof { |
There was a problem hiding this comment.
why are we handling EOF specially here compared to how we did it before? Is this to preserve the error message?
(if so, it seems a bit unfortunate that we have to duplicate the error here)
There was a problem hiding this comment.
There isn't a span associated with the EOF error, so the diagnostics are very poor unless we specifically check for it. See #55547 for an example..
There was a problem hiding this comment.
OK, seems like a problem, but an orthogonal one.
| // so ensure that it is volatile | ||
| if outputs.is_empty() { | ||
| volatile = true; | ||
| } |
There was a problem hiding this comment.
where did this logic (about volatile = true) go to?
There was a problem hiding this comment.
It's in the main expansion function now.
| @@ -23,33 +25,18 @@ pub fn expand_assert<'cx>( | |||
| sp: Span, | |||
| tts: &[TokenTree], | |||
| ) -> Box<dyn MacResult + 'cx> { | |||
| let mut parser = cx.new_parser_from_tts(tts); | |||
|
|
|||
| if parser.token == token::Eof { | |||
There was a problem hiding this comment.
I guess we have some similar code duplication here already, though.
| @@ -25,16 +27,39 @@ pub fn expand_cfg<'cx>(cx: &mut ExtCtxt, | |||
| tts: &[tokenstream::TokenTree]) | |||
| -> Box<dyn base::MacResult + 'static> { | |||
| let sp = sp.apply_mark(cx.current_expansion.mark); | |||
|
|
|||
| match parse_cfg(cx, sp, tts) { | |||
There was a problem hiding this comment.
Perhaps we should use try here instead of creating a new function?
This could be
let result = try { /* old code */ };
| Some(ts) | ||
| } else { | ||
| None | ||
| let Assert { cond_expr, custom_message } = match parse_assert(cx, sp, tts) { |
There was a problem hiding this comment.
Perhaps we should use try here instead of creating a new function?
This could be
let result = try { /* old code */ };
There was a problem hiding this comment.
We could, but doesn't this require migrating to the new edition?
Also, I personally prefer the separation of the parsing logic and the expansion logic.
There was a problem hiding this comment.
Seems fine. We can leave it like this. I agree separating the parsing/expansion logic makes sense.
| @@ -42,24 +44,47 @@ pub fn expand_global_asm<'cx>(cx: &'cx mut ExtCtxt, | |||
| return DummyResult::any(sp); | |||
| } | |||
|
|
|||
| match parse_global_asm(cx, sp, tts) { | |||
There was a problem hiding this comment.
similarly try may be useful here
nikomatsakis
added
S-waiting-on-author
nikomatsakis
approved these changes
Dec 26, 2018
|
@estebank Rebased. |
This comment has been minimized.
This comment has been minimized.
kennytm
added
S-waiting-on-review
estebank
approved these changes
Dec 27, 2018
| } | ||
|
|
||
| let fmtstr = panictry!(p.parse_expr()); | ||
| let fmtstr = p.parse_expr()?; | ||
| let mut named = false; | ||
|
|
||
| while p.token != token::Eof { | ||
| if !p.eat(&token::Comma) { |
There was a problem hiding this comment.
Shouldn't this be p.expect(&token::Comma)??
There was a problem hiding this comment.
Hm, looks like that regresses diagnostics:
diff --git a/src/test/ui/codemap_tests/bad-format-args.stderr b/src/test/ui/codemap_tests/bad-format-args.stderr
index 38320416b4..46deb9d4b0 100644
--- a/src/test/ui/codemap_tests/bad-format-args.stderr
+++ b/src/test/ui/codemap_tests/bad-format-args.stderr
@@ -6,17 +6,17 @@ LL | format!(); //~ ERROR requires at least a format string argument
|
= note: this error originates in a macro outside of the current crate (in Nightly builds, run with -Z external-macro-backtrace for more info)
-error: expected token: `,`
+error: expected one of `,`, `.`, `?`, or an operator, found `1`
--> $DIR/bad-format-args.rs:3:16
|
LL | format!("" 1); //~ ERROR expected token: `,`
- | ^
+ | ^ expected one of `,`, `.`, `?`, or an operator here
-error: expected token: `,`
+error: expected one of `,`, `.`, `?`, or an operator, found `1`
--> $DIR/bad-format-args.rs:4:19
|
LL | format!("", 1 1); //~ ERROR expected token: `,`
- | ^
+ | ^ expected one of `,`, `.`, `?`, or an operator here
error: aborting due to 3 previous errors
diff --git a/src/test/ui/macros/missing-comma.stderr b/src/test/ui/macros/missing-comma.stderr
index 5881e0b7b6..024c0bfada 100644
--- a/src/test/ui/macros/missing-comma.stderr
+++ b/src/test/ui/macros/missing-comma.stderr
@@ -1,8 +1,8 @@
-error: expected token: `,`
+error: expected one of `,`, `.`, `?`, or an operator, found `a`
--> $DIR/missing-comma.rs:10:19
|
LL | println!("{}" a);
- | ^
+ | ^ expected one of `,`, `.`, `?`, or an operator here
error: no rules expected the token `b`
--> $DIR/missing-comma.rs:12:12
There was a problem hiding this comment.
Doesn't seem clear to me that these are regressions. Changes, sure, more verbose, true, but not necessarily a regression. Arguably that is the right output: it's not only a comma that is expected there (although it is missing the closing brace).
Lets merge this PR as is and we can get back to this in the future.
There was a problem hiding this comment.
But out of those tokens, only the comma is valid at that position in the syntax extension, right? So isn't including the other expected tokens misleading?
There was a problem hiding this comment.
println!("{} {}", a.foo, b?); is valid code, so for the second case above the change is valid, while the other two are not necessarily an improvement... Let's leave as is for now.
There was a problem hiding this comment.
Oh, I see. I was only looking at the case after the string literal.
|
@bors r+ |
|
📌 Commit 39404c1d18bd309de7023c9fc11e1ca0ed342a8a has been approved by |
bors
added
S-waiting-on-bors
|
⌛ Testing commit 39404c1d18bd309de7023c9fc11e1ca0ed342a8a with merge 4d30868eb2341aee8d561ddfcdc54822d7be0066... |
|
💔 Test failed - status-travis |
bors
removed
the
S-waiting-on-bors
bors
added
the
S-waiting-on-review
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
kennytm
added
S-waiting-on-author
This commit completely removes usage of the `panictry!` macro from outside libsyntax. The macro causes parse errors to be fatal, so using it in libsyntax_ext caused parse failures *within* a syntax extension to be fatal, which is probably not intended. Furthermore, this commit adds spans to diagnostics emitted by empty extensions if they were missing, à la rust-lang#56491.
estebank
reviewed
Jan 2, 2019
| fn main() { | ||
| let a : u32 = 0; | ||
| let a : usize = 0; |
There was a problem hiding this comment.
Why was this type changed? Isn't the fix only to enable type_ascription?
There was a problem hiding this comment.
No, without this change the ascription causes type mismatch errors.
|
@bors r+ |
|
📌 Commit 0a6fb84 has been approved by |
bors
added
S-waiting-on-bors
Mark-Simulacrum
added a commit
to Mark-Simulacrum/rust
that referenced
this pull request
Jan 3, 2019
make `panictry!` private to libsyntax This commit completely removes usage of the `panictry!` macro from outside libsyntax. The macro causes parse errors to be fatal, so using it in libsyntax_ext caused parse failures *within* a syntax extension to be fatal, which is probably not intended. Furthermore, this commit adds spans to diagnostics emitted by empty extensions if they were missing, à la rust-lang#56491.
bors
added a commit
that referenced
this pull request
Jan 4, 2019
make `panictry!` private to libsyntax This commit completely removes usage of the `panictry!` macro from outside libsyntax. The macro causes parse errors to be fatal, so using it in libsyntax_ext caused parse failures *within* a syntax extension to be fatal, which is probably not intended. Furthermore, this commit adds spans to diagnostics emitted by empty extensions if they were missing, à la #56491.
|
☀️ Test successful - status-appveyor, status-travis |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
This commit completely removes usage of the
panictry!macro fromoutside libsyntax. The macro causes parse errors to be fatal, so using
it in libsyntax_ext caused parse failures within a syntax extension to
be fatal, which is probably not intended.
Furthermore, this commit adds spans to diagnostics emitted by empty
extensions if they were missing, à la #56491.