Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

Allow for missing invisible close delim when reparsing an expression. #139298

Closed
wants to merge 1 commit into from

Conversation

nnethercote
Copy link
Contributor

@nnethercote nnethercote commented Apr 3, 2025

This can happen when invalid syntax is passed to a declarative macro. We shouldn't be too strict about the token stream position once the parser has rejected the invalid syntax.

Fixes #139248.

r? @petrochenkov


try-job: test-various

@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 Apr 3, 2025
@petrochenkov
Copy link
Contributor

@bors r+ rollup

@bors
Copy link
Collaborator

bors commented Apr 3, 2025

📌 Commit a2a081a has been approved by petrochenkov

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 3, 2025
@nnethercote
Copy link
Contributor Author

#137874 is similar to #139248, there might be a common fix slightly different to what I've done here.

@bors r-

@bors bors 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-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Apr 3, 2025
@nnethercote
Copy link
Contributor Author

#137874 is similar to #139248, there might be a common fix slightly different to what I've done here.

No, it needs a separate fix.

@bors r=petrochenkov

@bors
Copy link
Collaborator

bors commented Apr 3, 2025

📌 Commit a2a081a has been approved by petrochenkov

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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Apr 3, 2025
Zalathar added a commit to Zalathar/rust that referenced this pull request Apr 4, 2025
…nkov

Allow for missing invisible close delim when reparsing an expression.

This can happen when invalid syntax is passed to a declarative macro. We shouldn't be too strict about the token stream position once the parser has rejected the invalid syntax.

Fixes rust-lang#139248.

r? `@petrochenkov`
bors added a commit to rust-lang-ci/rust that referenced this pull request Apr 4, 2025
Rollup of 14 pull requests

Successful merges:

 - rust-lang#137869 (Demote i686-pc-windows-gnu to Tier 2)
 - rust-lang#137880 (Autodiff batching)
 - rust-lang#138546 (Add integer to string formatting tests)
 - rust-lang#138947 (Refactor Apple version handling in the compiler)
 - rust-lang#138950 (replace extra_filename with strict version hash in metrics file names)
 - rust-lang#139213 (Run coretests and alloctests with cg_clif in CI)
 - rust-lang#139274 (Rustdoc: typecheck settings.js)
 - rust-lang#139295 (Remove creation of duplicate `AnonPipe`)
 - rust-lang#139298 (Allow for missing invisible close delim when reparsing an expression.)
 - rust-lang#139313 (Deduplicate some `rustc_middle` function bodies by calling the `rustc_type_ir` equivalent)
 - rust-lang#139317 (compiletest: Encapsulate all of the code that touches libtest)
 - rust-lang#139322 (Add helper function for checking LLD usage to `run-make-support`)
 - rust-lang#139335 (Pass correct param-env to `error_implies`)
 - rust-lang#139342 (Add a mailmap entry for myself)

Failed merges:

 - rust-lang#138949 (Rename `is_like_osx` to `is_like_darwin`)

r? `@ghost`
`@rustbot` modify labels: rollup
@Zalathar
Copy link
Contributor

Zalathar commented Apr 4, 2025

Possibly failed in rollup: #139344 (comment)

@bors r-

@bors bors 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-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Apr 4, 2025
@Zalathar
Copy link
Contributor

Zalathar commented Apr 4, 2025

@bors try

@bors
Copy link
Collaborator

bors commented Apr 4, 2025

⌛ Trying commit a2a081a with merge 46cc200...

bors added a commit to rust-lang-ci/rust that referenced this pull request Apr 4, 2025
Allow for missing invisible close delim when reparsing an expression.

This can happen when invalid syntax is passed to a declarative macro. We shouldn't be too strict about the token stream position once the parser has rejected the invalid syntax.

Fixes rust-lang#139248.

r? `@petrochenkov`

---
try-job: test-various
@rust-log-analyzer

This comment has been minimized.

@bors
Copy link
Collaborator

bors commented Apr 4, 2025

💔 Test failed - checks-actions

@nnethercote
Copy link
Contributor Author

I don't understand what's going wrong. The test passes locally and the diff on CI is strange:

41    |
42 LL | thread_local! { static a : () = (if b) }
43    | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ expected expression
-   --> $SRC_DIR/std/src/sys/thread_local/native/mod.rs:LL:COL
45    |
46    = note: while parsing argument for this `expr` macro fragment
47    |

@Zalathar
Copy link
Contributor

Zalathar commented Apr 4, 2025

Oh, I wonder if this is wasm causing problems because the path of the source file defining the macro is different.

In the raw stderr, the path on line 44 is:

    --> /rustc/FAKE_PREFIX/library/std/src/sys/thread_local/statik.rs:28:18

@nnethercote
Copy link
Contributor Author

How is wasm involved?

I could rewrite the test to use a local macro instead of thread_local!, that might help.

@nnethercote
Copy link
Contributor Author

@bors try

bors added a commit to rust-lang-ci/rust that referenced this pull request Apr 6, 2025
Allow for missing invisible close delim when reparsing an expression.

This can happen when invalid syntax is passed to a declarative macro. We shouldn't be too strict about the token stream position once the parser has rejected the invalid syntax.

Fixes rust-lang#139248.

r? `@petrochenkov`

---
try-job: test-various
@bors
Copy link
Collaborator

bors commented Apr 6, 2025

⌛ Trying commit ad9d8f9 with merge 281c69c...

@rust-log-analyzer

This comment has been minimized.

@nnethercote
Copy link
Contributor Author

@bors try

@bors
Copy link
Collaborator

bors commented Apr 6, 2025

⌛ Trying commit 296b484 with merge 9840dcf...

bors added a commit to rust-lang-ci/rust that referenced this pull request Apr 6, 2025
Allow for missing invisible close delim when reparsing an expression.

This can happen when invalid syntax is passed to a declarative macro. We shouldn't be too strict about the token stream position once the parser has rejected the invalid syntax.

Fixes rust-lang#139248.

r? `@petrochenkov`

---
try-job: test-various
@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@nnethercote
Copy link
Contributor Author

@bors try

@bors
Copy link
Collaborator

bors commented Apr 7, 2025

⌛ Trying commit fbdf9f8 with merge 732ba88...

bors added a commit to rust-lang-ci/rust that referenced this pull request Apr 7, 2025
Allow for missing invisible close delim when reparsing an expression.

This can happen when invalid syntax is passed to a declarative macro. We shouldn't be too strict about the token stream position once the parser has rejected the invalid syntax.

Fixes rust-lang#139248.

r? `@petrochenkov`

---
try-job: test-various
@bors
Copy link
Collaborator

bors commented Apr 7, 2025

☀️ Try build successful - checks-actions
Build commit: 732ba88 (732ba88caf76736237d422d1580a6865c071b6ff)

@nnethercote
Copy link
Contributor Author

I could rewrite the test to use a local macro instead of thread_local!, that might help.

That worked. I still don't understand what went wrong with the thread_local! version.

@bors r=petrochenkov

@bors
Copy link
Collaborator

bors commented Apr 7, 2025

📌 Commit fbdf9f8 has been approved by petrochenkov

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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Apr 7, 2025
@Zalathar
Copy link
Contributor

Zalathar commented Apr 7, 2025

That worked. I still don't understand what went wrong with the thread_local! version.

I believe the problem was:

  • When using thread_local!, the error message includes a path into standard library source where thread_local_inner is defined ($SRC_DIR/std/src/sys/thread_local/native/mod.rs:LL:COL).
  • Some targets use a different definition of thread_local_inner that is in a different file, so the path into standard library source is different.
  • That different path causes the snapshots to not match, even though the difference isn't important to what this test is testing.

All of that is a distraction from what this test actually cares about, so I think switching to a local macro was the right call.

Zalathar added a commit to Zalathar/rust that referenced this pull request Apr 7, 2025
…nkov

Allow for missing invisible close delim when reparsing an expression.

This can happen when invalid syntax is passed to a declarative macro. We shouldn't be too strict about the token stream position once the parser has rejected the invalid syntax.

Fixes rust-lang#139248.

r? `@petrochenkov`

---
try-job: test-various
@nnethercote
Copy link
Contributor Author

so I think switching to a local macro was the right call.

Thanks for the explanation!

bors added a commit to rust-lang-ci/rust that referenced this pull request Apr 7, 2025
Rollup of 4 pull requests

Successful merges:

 - rust-lang#138314 (fix usage of `autodiff` macro with inner functions)
 - rust-lang#138766 (coverage: Deal with unused functions and their names in one place)
 - rust-lang#139298 (Allow for missing invisible close delim when reparsing an expression.)
 - rust-lang#139426 (Make the UnifyKey and UnifyValue imports non-nightly)

r? `@ghost`
`@rustbot` modify labels: rollup
This can happen when invalid syntax is passed to a declarative macro. We
shouldn't be too strict about the token stream position once the parser
has rejected the invalid syntax.

Fixes rust-lang#139248.
@petrochenkov
Copy link
Contributor

This is included into #139464 so I'll close.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ICE: no close delim when reparsing Expr
6 participants