Skip to content

Less maybe_whole_expr #107550

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

Closed

Conversation

nnethercote
Copy link
Contributor

@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. labels Feb 1, 2023
maybe_whole_expr!(self);
if let token::Interpolated(nt) = &self.token.kind {
match &**nt {
token::NtExpr(e) | token::NtLiteral(e) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

e is not actually checked for being a literal here.
What happens if non-literal NtExpr ends up being passed to this function?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good question. I don't know. But the existing code (which uses maybe_whole_expr) also doesn't check if e is a literal. I will try adding a is-literal check and see if anything fails.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Adding a literal check causes various test failures:

    [ui] tests/ui/issues/issue-43250.rs
    [ui] tests/ui/macros/macro-pat-neg-lit.rs
    [ui] tests/ui/macros/vec-macro-in-pattern.rs
    [ui] tests/ui/match/expr_before_ident_pat.rs
    [ui] tests/ui/parser/issues/issue-65122-mac-invoc-in-mut-patterns.rs
    [ui] tests/ui/parser/issues/issue-70050-ntliteral-accepts-negated-lit.rs
    [ui] tests/ui/pattern/issue-92074-macro-ice.rs
    [ui] tests/ui/pattern/patkind-litrange-no-expr.rs
    [ui] tests/ui/proc-macro/expand-expr.rs

Some of these are just changes in error messages, e.g.:

---- [ui] tests/ui/issues/issue-43250.rs stdout ----
diff of stderr:

-	error: arbitrary expressions aren't allowed in patterns
-	  --> $DIR/issue-43250.rs:9:8
+	error: expected pattern, found `y`
+	  --> $DIR/issue-43250.rs:6:17
3	   |
+	LL |             let $a = 0;
+	   |                 ^^ expected pattern
+	...
4	LL |     m!(y);
-	   |        ^
+	   |     ----- in this macro invocation
+	   |
+	   = note: this error originates in the macro `m` (in Nightly builds, run with -Z macro-backtrace for more info)
6	
-	error: arbitrary expressions aren't allowed in patterns
-	  --> $DIR/issue-43250.rs:11:8
+	error: expected pattern, found `C`
+	  --> $DIR/issue-43250.rs:6:17
9	   |
+	LL |             let $a = 0;
+	   |                 ^^ expected pattern
+	...
10	LL |     m!(C);
-	   |        ^
+	   |     ----- in this macro invocation
+	   |
+	   = note: this error originates in the macro `m` (in Nightly builds, run with -Z macro-backtrace for more info)
12	
13	error: aborting due to 2 previous errors
14	

That one now fails while parsing, instead of failing during lowering.

But some check-pass tests now fail to compile, e.g.:

error: unexpected token: `-2`
  --> fake-test-src-base/parser/issues/issue-70050-ntliteral-accepts-negated-lit.rs:5:14
   |
LL |         bar!($a)
   |              ^^
...
LL |     ($b:literal) => {};
   |      ---------- while parsing argument for this `literal` macro fragment
...
LL |     foo!(-2);
   |     -------- in this macro invocation
   |
   = note: this error originates in the macro `foo` (in Nightly builds, run with -Z macro-backtrace for more info)

So I don't think a literal check is appropriate here.

Copy link
Contributor

Choose a reason for hiding this comment

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

The function is called parse_literal_maybe_minus so it's not surprising that it accepts -lit in addition to just lit.
I meant checking for anything except lit and -lit (and maybe ExprKind::Err).

Copy link
Contributor

Choose a reason for hiding this comment

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

We need at least a test executing these case, if not a check.
Perhaps they are reported as errors at some later point, then an extra check won't be necessary.

@petrochenkov petrochenkov 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 Feb 1, 2023
In this context there's no need to look for `NtPath` or `NtBlock`.
@nnethercote nnethercote force-pushed the less-maybe_whole_expr branch from aab9f46 to cee1963 Compare February 2, 2023 00:10
@nnethercote
Copy link
Contributor Author

I have updated the code, with some more minor refactorings. But I didn't add the is-literal check because of the test failures mentioned above.

@petrochenkov petrochenkov added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. 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-author Status: This is awaiting some action (such as code changes or more information) from the author. S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 2, 2023
@bors
Copy link
Collaborator

bors commented Feb 5, 2023

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

@nnethercote
Copy link
Contributor Author

I don't think this is worth the effort, but I opened #109415 for the refactoring of handle_missing_lit.

@nnethercote nnethercote deleted the less-maybe_whole_expr branch June 17, 2024 00:16
bors added a commit to rust-lang-ci/rust that referenced this pull request Jun 17, 2024
… r=<try>

Less `maybe_whole_expr`, take 2

I first tried this in rust-lang#107550. I now think it's worth doing again, as a precursor to #1241414.

r? `@petrochenkov`
bors added a commit to rust-lang-ci/rust that referenced this pull request Jun 25, 2024
… r=<try>

Less `maybe_whole_expr`, take 2

I first tried this in rust-lang#107550. I now think it's worth doing again, as a precursor to rust-lang#124141.

r? `@petrochenkov`
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Jun 26, 2024
…2, r=petrochenkov

Less `maybe_whole_expr`, take 2

I first tried this in rust-lang#107550. I now think it's worth doing again, as a precursor to rust-lang#124141.

r? `@petrochenkov`
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Jun 26, 2024
…2, r=petrochenkov

Less `maybe_whole_expr`, take 2

I first tried this in rust-lang#107550. I now think it's worth doing again, as a precursor to rust-lang#124141.

r? ``@petrochenkov``
jhpratt added a commit to jhpratt/rust that referenced this pull request Jun 27, 2024
…2, r=petrochenkov

Less `maybe_whole_expr`, take 2

I first tried this in rust-lang#107550. I now think it's worth doing again, as a precursor to rust-lang#124141.

r? ```@petrochenkov```
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Jun 27, 2024
Rollup merge of rust-lang#126571 - nnethercote:less-maybe_whole-expr-2, r=petrochenkov

Less `maybe_whole_expr`, take 2

I first tried this in rust-lang#107550. I now think it's worth doing again, as a precursor to rust-lang#124141.

r? ```@petrochenkov```
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants