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

Preserve span when lowering ExprKind::Paren #33683

Merged
merged 4 commits into from
May 21, 2016
Merged

Conversation

sanxiyn
Copy link
Member

@sanxiyn sanxiyn commented May 17, 2016

Fix #33681.

@rust-highfive
Copy link
Collaborator

r? @nikomatsakis

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

@nikomatsakis
Copy link
Contributor

@bors r+ rollup

@bors
Copy link
Contributor

bors commented May 17, 2016

📌 Commit 0f257df has been approved by nikomatsakis

@Manishearth
Copy link
Member

Manishearth commented May 18, 2016

@bors r- , fails travis

failures:
    [compile-fail] compile-fail/associated-const-private-impl.rs
    [compile-fail] compile-fail/issue-2392.rs
    [compile-fail] compile-fail/private-struct-field-cross-crate.rs
    [compile-fail] compile-fail/private-struct-field.rs

test result: FAILED. 2237 passed; 4 failed; 11 ignored; 0 measured

@sanxiyn
Copy link
Member Author

sanxiyn commented May 18, 2016

This is trickier than I thought. The problem is illustrated by the following code:

macro_rules! paren {
    ($e:expr) => (($e))
}

mod m {
    pub struct S {
        x: i32
    }
    pub fn make() -> S {
        S { x: 0 }
    }
}

fn main() {
    let s = m::make();
    paren!(s.x);
}

Before:

error: field `x` of struct `m::S` is private
  --> test.rs:16:12
16 |>     paren!(s.x);
   |>            ^^^

After:

error: field `x` of struct `m::S` is private
 --> test.rs:2:19
2 |>     ($e:expr) => (($e))
  |>                   ^^^^

@sanxiyn
Copy link
Member Author

sanxiyn commented May 18, 2016

Since Rust macro is not text substitution, there is never a need to parenthesize like ($e), unlike in C. Still, assert_eq! does that. By the way, why do we have assert_eq! in compile-fail tests?!

@nikomatsakis
Copy link
Contributor

@sanxiyn

By the way, why do we have assert_eq! in compile-fail tests?!

Why wouldn't we?

Still, assert_eq! does that.

Hmm. So... it seems like your highlighting code was doing the right thing, but perhaps assert_eq ought to be modified. I guess we can expect other macros to get suboptimal highlighting in a similar fashion.

@sanxiyn
Copy link
Member Author

sanxiyn commented May 18, 2016

Modified assert_eq!. Now passes cfail locally.

@sanxiyn
Copy link
Member Author

sanxiyn commented May 18, 2016

Now suboptimal highlighting is avoided.

@nikomatsakis
Copy link
Contributor

@bors r+ rollup

I'm not sure which is better, but this seems good too.

@bors
Copy link
Contributor

bors commented May 19, 2016

📌 Commit edf1773 has been approved by nikomatsakis

GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this pull request May 20, 2016
bors added a commit that referenced this pull request May 20, 2016
Rollup of 6 pull requests

- Successful merges: #33668, #33676, #33683, #33734, #33739, #33745
- Failed merges: #33578
@bors bors merged commit edf1773 into rust-lang:master May 21, 2016
@sanxiyn sanxiyn deleted the paren-span branch May 21, 2016 11:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants