Make assert! ensure the macro is parsed completely #60039

rasendubi · 2019-04-17T12:35:27Z

rust-highfive · 2019-04-17T12:35:38Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @alexcrichton (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

jonas-schievink · 2019-04-17T13:19:56Z

Is there any reason why assert! is built into the compiler instead of just being defined normally in libcore?

petrochenkov · 2019-04-17T13:39:38Z

@jonas-schievink
It was moved from libcore to compiler as a first step in implementing #44838 (the second step never happened though).

alexcrichton · 2019-04-17T14:33:56Z

Thanks @rasendubi! To be double extra sure I think we'll want to run this through crater to ensure we don't break too many crates by accident, so...

@bors: try

bors · 2019-04-17T14:34:15Z

⌛ Trying commit dfc0861 with merge 78b0bd8441ef961f6475e3d47fe3bca38cb83432...

bors · 2019-04-17T17:00:19Z

☀️ Try build successful - checks-travis
Build commit: 78b0bd8441ef961f6475e3d47fe3bca38cb83432

src/test/ui/macros/assert-trailing-junk.rs

alexcrichton · 2019-04-19T16:27:01Z

@craterbot run start=master#efe2f32a6b8217425f361ec7c206910c611c03ee end=try#78b0bd8441ef961f6475e3d47fe3bca38cb83432 mode=check-only

craterbot · 2019-04-19T16:27:04Z

👌 Experiment pr-60039 created and queued.
🔍 You can check out the queue and this experiment's details.

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

craterbot · 2019-04-19T16:27:07Z

🚧 Experiment pr-60039 is now running on agent aws-2.

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

rasendubi · 2019-04-21T07:45:05Z

The crater agent is unreachable for a day now. Is that normal?

Last heartbeat: 2019-04-20 06:26:03 UTC

estebank · 2019-04-22T16:18:16Z

CC @pietroalbini ^^

craterbot · 2019-04-22T16:35:51Z

🚨 Experiment pr-60039 has encountered an error: docker is not running
🛠️ If the error is fixed use the retry command.

🆘 Can someone from the infra team check in on this? @rust-lang/infra
ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

pietroalbini · 2019-04-22T16:41:30Z

@craterbot retry

The Crater machine crashed, now it should work again.

craterbot · 2019-04-22T16:41:31Z

🛠️ Experiment pr-60039 queued again.

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

craterbot · 2019-04-22T16:41:32Z

🚧 Experiment pr-60039 is now running on agent aws-2.

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

craterbot · 2019-04-24T05:34:10Z

🎉 Experiment pr-60039 is completed!
📊 18 regressed and 1 fixed (60951 total)
📰 Open the full report.

⚠️ If you notice any spurious failure please add them to the blacklist!
ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

alexcrichton · 2019-04-24T16:45:41Z

That looks like an unfortuante amount of breakage :(

Given all that I think that the best thing to do might be to switch this to a warning, and then perhaps send some PRs to upstream crates to fix the issues? After awhile we can then switch to an error

estebank · 2019-04-24T17:47:21Z

@alexcrichton a significant amount of these (8) seem to be coming from mop-structs having the equivalent of assert!(a == b "error message") (note missing comma), which we could explicitly detect and unconditionally warn on. q1tsim and carrier have the same kind of error.

static-linkedlist has assert!(error;).

bluenrg, cargo-generate (and it's transitive error) and b-r-oleary.passwords were a bit more bizarre on first look without context, but looking a bit deeper I see they are similar to static-linkedlist:

https://github.com/danielgallagher0/bluenrg/blob/a47f1f57277995937e1cf6ae8c8f58d8e7254187/tests/l2cap.rs#L64-L69

https://github.com/ashleygwilliams/cargo-generate/blob/54a85ed98ee4ad06c8228dd60ac070ff483291dd/tests/integration/basics.rs#L447-L450

https://github.com/b-r-oleary/passwords/blob/c85100139d8301f72c6948b672e98247571c8a51/src/generators/phrase.rs#L195-L200

I think if we accept trailing ; and trailing &'static strs in assert, potentially linting about these, we can merge the fix.

On the plus side none of the errors seems to be intentional misuse of the "feature".

src/libsyntax_ext/assert.rs

There is a bug in rustc that allows adding invalid trailing tokens to the `assert!` macro call. They are currently ignored but are going to produce errors in the future. Fix assert! macro usage to remove extra tokens. For more information, see rust-lang/rust#60024 and rust-lang/rust#60039

There is a bug in rustc that allows adding invalid trailing tokens to the `assert!` macro call. They are currently ignored but are going to produce errors in the future. Fix assert! macro usage to add missing comma. For more information, see rust-lang/rust#60024 and rust-lang/rust#60039

There is a bug in rustc that allows adding invalid trailing tokens to the `assert!` macro call. They are currently ignored but are going to produce errors in the future. Fix assert! macro usage to remove extra tokens. For more information, see rust-lang/rust#60024 and rust-lang/rust#60039

There is a bug in rustc that allows adding invalid trailing tokens to the `assert!` macro call. They are currently ignored but are going to produce errors in the future. Fix assert! macro usage to add missing comma. For more information, see rust-lang/rust#60024 and rust-lang/rust#60039

rasendubi · 2019-04-25T08:07:42Z

PRs to upstream crates:

craterbot · 2019-04-27T05:33:38Z

🎉 Experiment pr-60039-1 is completed!
📊 1 regressed and 0 fixed (60951 total)
📰 Open the full report.

⚠️ If you notice any spurious failure please add them to the blacklist!
ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

rasendubi · 2019-04-27T12:04:01Z

The one regression reported by crater does not seem relevant to the change. From logs:

[INFO] [stderr] error[E0061]: this function takes 2 parameters but 0 parameters were supplied
[INFO] [stderr]   --> src/bin/main.rs:29:24
[INFO] [stderr]    |
[INFO] [stderr] 29 |     let mut computer = Computer::new();
[INFO] [stderr]    |                        ^^^^^^^^^^^^^^^ expected 2 parameters
[INFO] [stderr] 
[INFO] [stderr] error[E0061]: this function takes 2 parameters but 1 parameter was supplied
[INFO] [stderr]   --> src/bin/main.rs:43:15
[INFO] [stderr]    |
[INFO] [stderr] 43 |         match tokenize(&buffer) {
[INFO] [stderr]    |               ^^^^^^^^^^^^^^^^^ expected 2 parameters
[INFO] [stderr] 
[INFO] [stderr] error: aborting due to 2 previous errors

alexcrichton · 2019-04-29T14:06:26Z

@bors: r+

This looks like a great compromise, thanks @rasendubi!

bors · 2019-04-29T14:06:28Z

📌 Commit f29e9a5 has been approved by alexcrichton

bors · 2019-04-29T14:50:25Z

⌛ Testing commit f29e9a5 with merge 96d565b...

Make assert! ensure the macro is parsed completely Fixes #60024

bors · 2019-04-29T17:44:09Z

☀️ Test successful - checks-travis, status-appveyor
Approved by: alexcrichton
Pushing 96d565b to master...

rasendubi · 2019-04-29T17:55:36Z

This is my first contribution to rust (besides some minor docs improvements). Thank you guys for all the support! 🎉

Make assert! ensure the macro is parsed completely

dfc0861

rust-highfive assigned alexcrichton Apr 17, 2019

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Apr 17, 2019

rasendubi mentioned this pull request Apr 17, 2019

assert! allows invalid trailing tokens in expression #60024

Closed

estebank reviewed Apr 19, 2019

View reviewed changes

src/test/ui/macros/assert-trailing-junk.rs Show resolved Hide resolved

craterbot removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Apr 19, 2019

craterbot added the S-waiting-on-crater Status: Waiting on a crater run to be completed. label Apr 19, 2019

craterbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-crater Status: Waiting on a crater run to be completed. labels Apr 24, 2019

estebank reviewed Apr 24, 2019

View reviewed changes

src/libsyntax_ext/assert.rs Show resolved Hide resolved

Handle common assert! misuses

f29e9a5

rasendubi mentioned this pull request Apr 25, 2019

Fix assert! macro usage Q1tBV/q1tsim#1

Closed

rasendubi mentioned this pull request Apr 25, 2019

Fix assert! macro usage arkap/rs-static-linkedlist#1

Merged

rasendubi mentioned this pull request Apr 25, 2019

Fix assert! macro usage aep/carrier-legacy#1

Open

rasendubi mentioned this pull request Apr 25, 2019

Fix assert! macro usage b-r-oleary/passwords#1

Open

rasendubi mentioned this pull request Apr 25, 2019

Fix assert! macro usage ecsumed/wrench#1

Merged

rasendubi mentioned this pull request Apr 25, 2019

Fix assert! macro usage focusaurus/cody#1

Closed

rasendubi mentioned this pull request Apr 25, 2019

Fix assert! macro usage wduquette/bonaventure#2

Merged

craterbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-crater Status: Waiting on a crater run to be completed. labels Apr 27, 2019

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 29, 2019

bors added a commit that referenced this pull request Apr 29, 2019

Auto merge of #60039 - rasendubi:assert-trailing-junk, r=alexcrichton

96d565b

Make assert! ensure the macro is parsed completely Fixes #60024

bors mentioned this pull request Apr 29, 2019

Update clippy #60355

Merged

bors added the merged-by-bors This PR was explicitly merged by bors. label Apr 29, 2019

bors merged commit f29e9a5 into rust-lang:master Apr 29, 2019

adlerd mentioned this pull request Feb 27, 2020

Revisit trailing tokens in assert! #69531

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make assert! ensure the macro is parsed completely #60039

Make assert! ensure the macro is parsed completely #60039

rasendubi commented Apr 17, 2019

rust-highfive commented Apr 17, 2019

jonas-schievink commented Apr 17, 2019

petrochenkov commented Apr 17, 2019

alexcrichton commented Apr 17, 2019

bors commented Apr 17, 2019

bors commented Apr 17, 2019

alexcrichton commented Apr 19, 2019

craterbot commented Apr 19, 2019

craterbot commented Apr 19, 2019

rasendubi commented Apr 21, 2019

estebank commented Apr 22, 2019

craterbot commented Apr 22, 2019

pietroalbini commented Apr 22, 2019

craterbot commented Apr 22, 2019

craterbot commented Apr 22, 2019

craterbot commented Apr 24, 2019

alexcrichton commented Apr 24, 2019

estebank commented Apr 24, 2019 •

edited

Loading

rasendubi commented Apr 25, 2019

craterbot commented Apr 27, 2019

rasendubi commented Apr 27, 2019

alexcrichton commented Apr 29, 2019

bors commented Apr 29, 2019

bors commented Apr 29, 2019

bors commented Apr 29, 2019

rasendubi commented Apr 29, 2019

Make assert! ensure the macro is parsed completely #60039

Make assert! ensure the macro is parsed completely #60039

Conversation

rasendubi commented Apr 17, 2019

rust-highfive commented Apr 17, 2019

jonas-schievink commented Apr 17, 2019

petrochenkov commented Apr 17, 2019

alexcrichton commented Apr 17, 2019

bors commented Apr 17, 2019

bors commented Apr 17, 2019

alexcrichton commented Apr 19, 2019

craterbot commented Apr 19, 2019

craterbot commented Apr 19, 2019

rasendubi commented Apr 21, 2019

estebank commented Apr 22, 2019

craterbot commented Apr 22, 2019

pietroalbini commented Apr 22, 2019

craterbot commented Apr 22, 2019

craterbot commented Apr 22, 2019

craterbot commented Apr 24, 2019

alexcrichton commented Apr 24, 2019

estebank commented Apr 24, 2019 • edited Loading

rasendubi commented Apr 25, 2019

craterbot commented Apr 27, 2019

rasendubi commented Apr 27, 2019

alexcrichton commented Apr 29, 2019

bors commented Apr 29, 2019

bors commented Apr 29, 2019

bors commented Apr 29, 2019

rasendubi commented Apr 29, 2019

estebank commented Apr 24, 2019 •

edited

Loading