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.

Sign up for GitHub

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

Jump to bottom

rustc_parse: fix spans on cast and range exprs with attrs #77780

Merged
merged 2 commits into from
Oct 16, 2020
Merged

rustc_parse: fix spans on cast and range exprs with attrs #77780

merged 2 commits into from
Oct 16, 2020

Conversation

calebcartwright
Copy link
Member

@calebcartwright calebcartwright commented Oct 10, 2020 •
edited
Loading

Currently the span for cast and range expressions does not include the span of attributes associated to the lhs which is causing some issues for us in rustfmt.

fn foo() -> i64 {
    #[attr]
    1u64 as i64
}

fn bar() -> Range<i32> {
    #[attr]
    1..2
}

This corrects the span for cast and range expressions to fully include the span of child nodes

@rust-highfive
Copy link
Collaborator

r? @oli-obk

(rust_highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 10, 2020
@jyn514 jyn514 added A-parser Area: The parsing of Rust source code to an AST T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Oct 10, 2020
oli-obk
oli-obk reviewed Oct 12, 2020
View reviewed changes
compiler/rustc_parse/src/parser/expr.rs Outdated
@@ -569,6 +569,12 @@ impl<'a> Parser<'a> {
lhs_span: Span,
expr_kind: fn(P<Expr>, P<Ty>) -> ExprKind,
) -> PResult<'a, P<Expr>> {
let lhs_span = lhs
Copy link
Contributor

Choose a reason for hiding this comment

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

Please deduplicate this code with all other similar sites (e.g. https://github.com/rust-lang/rust/blob/6a54bfc44b5a1b47506fcdf4f074671328d963dd/compiler/rustc_parse/src/parser/expr.rs#L251) by putting it in a function and calling it at all those sites

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks! Good spot and requested change. The overall diff is small enough that I elected to just squash everything into a single commit instead of adding a separate one with the updates, hope that's alright

Copy link
Member Author

Choose a reason for hiding this comment

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

Noticed the same issue could occur on range expressions, so added a fix for that one as well (though we haven't had any issue reports for range expressions like we have for casts)

Copy link
Contributor

Choose a reason for hiding this comment

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

That's great, thanks for looking into other sites, too!

@calebcartwright calebcartwright changed the title rustc_parse: correct span on cast expr with attrs rustc_parse: fix spans on cast and range exprs with attrs Oct 12, 2020
@oli-obk
Copy link
Contributor

oli-obk commented Oct 15, 2020

@bors r+

@bors
Copy link
Contributor

bors commented Oct 15, 2020

📌 Commit 4e82da4 has been approved by oli-obk

@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 Oct 15, 2020
@oli-obk
Copy link
Contributor

oli-obk commented Oct 15, 2020

@bors rollup

JohnTitor added a commit to JohnTitor/rust that referenced this pull request Oct 15, 2020
@JohnTitor
Rollup merge of rust-lang#77780 - calebcartwright:cast-expr-attr-span…
659c85c 
…, r=oli-obk

rustc_parse: fix spans on cast and range exprs with attrs

Currently the span for cast and range expressions does not include the span of attributes associated to the lhs which is causing some issues for us in rustfmt.

```rust
fn foo() -> i64 {
    #[attr]
    1u64 as i64
}

fn bar() -> Range<i32> {
    #[attr]
    1..2
}
```

This corrects the span for cast and range expressions to fully include the span of child nodes
@JohnTitor JohnTitor mentioned this pull request Oct 15, 2020
Closed
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request Oct 15, 2020
@Dylan-DPC
Rollup merge of rust-lang#77780 - calebcartwright:cast-expr-attr-span…
7523b4b 
…, r=oli-obk

rustc_parse: fix spans on cast and range exprs with attrs

Currently the span for cast and range expressions does not include the span of attributes associated to the lhs which is causing some issues for us in rustfmt.

```rust
fn foo() -> i64 {
    #[attr]
    1u64 as i64
}

fn bar() -> Range<i32> {
    #[attr]
    1..2
}
```

This corrects the span for cast and range expressions to fully include the span of child nodes
This was referenced Oct 15, 2020
Closed
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Oct 16, 2020
@bors
Auto merge of rust-lang#78001 - Dylan-DPC:rollup-zq3kxth, r=Dylan-DPC
b6e2dc6 
Rollup of 14 pull requests

Successful merges:

 - rust-lang#75023 (ensure arguments are included in count mismatch span)
 - rust-lang#75265 (Add `str::{Split,RSplit,SplitN,RSplitN,SplitTerminator,RSplitTerminator,SplitInclusive}::as_str` methods)
 - rust-lang#75675 (mangling: mangle impl params w/ v0 scheme)
 - rust-lang#76084 (Refactor io/buffered.rs into submodules)
 - rust-lang#76119 (Stabilize move_ref_pattern)
 - rust-lang#77493 (ICEs should always print the top of the query stack)
 - rust-lang#77619 (Use futex-based thread-parker for Wasm32.)
 - rust-lang#77646 (For backtrace, use StaticMutex instead of a raw sys Mutex.)
 - rust-lang#77648 (Static mutex is static)
 - rust-lang#77657 (Cleanup cloudabi mutexes and condvars)
 - rust-lang#77672 (Simplify doc-cfg rendering based on the current context)
 - rust-lang#77780 (rustc_parse: fix spans on cast and range exprs with attrs)
 - rust-lang#77935 (BTreeMap: make PartialCmp/PartialEq explicit and tested)
 - rust-lang#77980 (Fix intra doc link for needs_drop)

Failed merges:

r? `@ghost`
@bors bors merged commit 9d8bf44 into rust-lang:master Oct 16, 2020
@rustbot rustbot added this to the 1.49.0 milestone Oct 16, 2020
@calebcartwright calebcartwright deleted the cast-expr-attr-span branch October 20, 2020 02:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@oli-obk oli-obk oli-obk left review comments

Assignees

@oli-obk oli-obk

Labels
A-parser Area: The parsing of Rust source code to an AST 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
Milestone
1.49.0
Development

Successfully merging this pull request may close these issues.
6 participants
@calebcartwright @rust-highfive @oli-obk @bors @jyn514 @rustbot