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

encode ; stmt without expr as StmtKind::Empty #69506

Merged
merged 1 commit into from
Mar 3, 2020

Conversation

Centril
Copy link
Contributor

@Centril Centril commented Feb 27, 2020

Instead of encoding ; statements without a an expression as a tuple in AST, encode it as ast::StmtKind::Empty.

r? @petrochenkov

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Feb 27, 2020
@Centril
Copy link
Contributor Author

Centril commented Feb 27, 2020

Let's see whether this happens to regress perf: @bors try @rust-timer queue

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion

@bors
Copy link
Contributor

bors commented Feb 27, 2020

⌛ Trying commit fcea184 with merge 1d82a25...

bors added a commit that referenced this pull request Feb 27, 2020
encode `;` stmt without expr as `StmtKind::Semi(None)`

Instead of encoding `;` statements without a an expression as a tuple in AST, we modify `ast::StmtKind::Semi` to accept an `Option<_>` and then encode the `;` with `StmtKind::Semi(None)`.

r? @petrochenkov
@bors
Copy link
Contributor

bors commented Feb 27, 2020

☀️ Try build successful - checks-azure
Build commit: 1d82a25 (1d82a2587146bc8a4282105545cd257ba2e3647b)

@rust-timer
Copy link
Collaborator

Queued 1d82a25 with parent 0c15adc, future comparison URL.

@Centril
Copy link
Contributor Author

Centril commented Feb 28, 2020

Seems like this resulted in a perf win rather than a regression, ostensibly because we avoid span_to_snippet.

@petrochenkov
Copy link
Contributor

Let's make it a separate StmtKind instead.

The semicolon statement is subtly different from EXPR;, i.e. in the next example

macro_rules! empty { () => {} }

fn foo() -> u8 {
    { 0 }
    
    empty!();
}

empty!(); expands into a semicolon statement in the token-based expansion model (#61733), but { 0 } is still treated as a trailing return statement in this case.

The PR that turned redundant semicolons into statements in AST for diagnostics walked some dangerous road, I really hope it didn't introduce anything incompatible with the macro expansion plans.

@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 29, 2020
@Centril
Copy link
Contributor Author

Centril commented Feb 29, 2020

Let's make it a separate StmtKind instead.

Hmm... Do you have any naming suggestions?

@petrochenkov
Copy link
Contributor

I really hope it didn't introduce anything incompatible with the macro expansion plans.

It actually regressed some things, but it looks like it's possible to fix them backward-compatibly.

macro_rules! empty { () => {} }

fn foo() -> u8 {
    { 0 }
    
    empty!();; // Works on 1.38, error on 1.39
}

@petrochenkov
Copy link
Contributor

Hmm... Do you have any naming suggestions?

Well.
StmtKind::Empty?

In C++ it's officially known as null statement, but I can't say I like that naming.

@Centril
Copy link
Contributor Author

Centril commented Feb 29, 2020

::Empty works for me; the other idea I had was ::Semi + ::SemiExpr(..) but ::Empty feels less like talking about lexical syntax, so it's nicer.

@Centril Centril changed the title encode ; stmt without expr as StmtKind::Semi(None) encode ; stmt without expr as StmtKind::Empty Feb 29, 2020
@Centril Centril added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Feb 29, 2020
@Centril
Copy link
Contributor Author

Centril commented Feb 29, 2020

Changed to ::Empty and fixed the lint naming.

@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 29, 2020
@petrochenkov
Copy link
Contributor

r=me with the remaining nit addressed.

@Centril
Copy link
Contributor Author

Centril commented Feb 29, 2020

@bors r=petrochenkov

@bors
Copy link
Contributor

bors commented Feb 29, 2020

📌 Commit 6a5de134879c7781a713d9b0d23a22aa5fdd5131 has been approved by petrochenkov

@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 Feb 29, 2020
@bors

This comment has been minimized.

@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 Mar 1, 2020
@Centril
Copy link
Contributor Author

Centril commented Mar 1, 2020

@bors r=petrochenkov

@bors
Copy link
Contributor

bors commented Mar 1, 2020

📌 Commit 176fe3f has been approved by petrochenkov

@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 Mar 1, 2020
@bors
Copy link
Contributor

bors commented Mar 3, 2020

⌛ Testing commit 176fe3f with merge 4ad6248...

@bors
Copy link
Contributor

bors commented Mar 3, 2020

☀️ Test successful - checks-azure
Approved by: petrochenkov
Pushing 4ad6248 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Mar 3, 2020
@bors bors merged commit 4ad6248 into rust-lang:master Mar 3, 2020
@rust-highfive
Copy link
Collaborator

📣 Toolstate changed by #69506!

Tested on commit 4ad6248.
Direct link to PR: #69506

💔 clippy-driver on windows: test-pass → test-fail (cc @mcarton @oli-obk @Manishearth @flip1995 @yaahc @phansch @llogiq).
💔 clippy-driver on linux: test-pass → test-fail (cc @mcarton @oli-obk @Manishearth @flip1995 @yaahc @phansch @llogiq).

rust-highfive added a commit to rust-lang-nursery/rust-toolstate that referenced this pull request Mar 3, 2020
Tested on commit rust-lang/rust@4ad6248.
Direct link to PR: <rust-lang/rust#69506>

💔 clippy-driver on windows: test-pass → test-fail (cc @mcarton @oli-obk @Manishearth @flip1995 @yaahc @phansch @llogiq).
💔 clippy-driver on linux: test-pass → test-fail (cc @mcarton @oli-obk @Manishearth @flip1995 @yaahc @phansch @llogiq).
@Centril Centril deleted the stmt-semi-none branch March 3, 2020 23:51
flip1995 added a commit to flip1995/rust-clippy that referenced this pull request Mar 3, 2020
bors added a commit to rust-lang/rust-clippy that referenced this pull request Mar 4, 2020
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Mar 4, 2020
Changes:
````
Rustup to rust-lang#69506
Revive rls integration test
use question mark operator
Add regression test
Use `try_eval_usize` over `eval_usize`
Add path for display trait
Use lang items instead of get_trait_def_id where possible
Update stderr
Don't lint debug formatting in debug impl
Whitelist unused attribute for use items.
add test for rust-lang#5238
````
bors added a commit that referenced this pull request Mar 4, 2020
submodules: update clippy from 8b7f7e6 to 74eae9d

Changes:
````
Rustup to #69506
Revive rls integration test
use question mark operator
Add regression test
Use `try_eval_usize` over `eval_usize`
Add path for display trait
Use lang items instead of get_trait_def_id where possible
Update stderr
Don't lint debug formatting in debug impl
Whitelist unused attribute for use items.
add test for #5238
````

Makes clippy tests pass again.

r? @oli-obk
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Mar 4, 2020
Changes:
````
Apply suggestions from code review
Simplify if_chain.
Move NumericLiteral to its own module.
Included binary and octal cases.
Resolve false positives for hex int cast.
Test for unnecessary_cast of hex int literal.
run-rustfix
Lint `if let Some` in question_mark lint
Add restrictive pat use in full binded struct
Update test case answers to match cargo dev fmt
Ran cargo dev fmt
Rustup to rust-lang#69506
Recommended changes from flip1995
Revive rls integration test
use question mark operator
Add regression test
Use `try_eval_usize` over `eval_usize`
Add path for display trait
Use lang items instead of get_trait_def_id where possible
Update stderr
Don't lint debug formatting in debug impl
Whitelist unused attribute for use items.
Fix one last test issue
Refactor suggested by krishna-veerareddy
Fixed compile error from merging
Changed test output to reflect cargo fmt
Run cargo dev fmt
Finished checking for cases of absolute values
add test for rust-lang#5238
Some bugfixing
Created floating point abs lint and test, but not yet run
````
bors added a commit that referenced this pull request Mar 5, 2020
submodules: update clippy from 8b7f7e6 to 74eae9d

Changes:
````
Rustup to #69506
Revive rls integration test
use question mark operator
Add regression test
Use `try_eval_usize` over `eval_usize`
Add path for display trait
Use lang items instead of get_trait_def_id where possible
Update stderr
Don't lint debug formatting in debug impl
Whitelist unused attribute for use items.
add test for #5238
````

Makes clippy tests pass again.

r? @oli-obk
flip1995 pushed a commit to flip1995/rust-clippy that referenced this pull request May 5, 2020
Changes:
````
Apply suggestions from code review
Simplify if_chain.
Move NumericLiteral to its own module.
Included binary and octal cases.
Resolve false positives for hex int cast.
Test for unnecessary_cast of hex int literal.
run-rustfix
Lint `if let Some` in question_mark lint
Add restrictive pat use in full binded struct
Update test case answers to match cargo dev fmt
Ran cargo dev fmt
Rustup to rust-lang/rust#69506
Recommended changes from flip1995
Revive rls integration test
use question mark operator
Add regression test
Use `try_eval_usize` over `eval_usize`
Add path for display trait
Use lang items instead of get_trait_def_id where possible
Update stderr
Don't lint debug formatting in debug impl
Whitelist unused attribute for use items.
Fix one last test issue
Refactor suggested by krishna-veerareddy
Fixed compile error from merging
Changed test output to reflect cargo fmt
Run cargo dev fmt
Finished checking for cases of absolute values
add test for rust-lang#5238
Some bugfixing
Created floating point abs lint and test, but not yet run
````
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants