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

Point to enclosing block/fn on nested unsafe #39202

Merged
merged 1 commit into from
Mar 11, 2017

Conversation

estebank
Copy link
Contributor

When declaring nested unsafe blocks (unsafe {unsafe {}}) that trigger
the "unnecessary unsafe block" error, point out the enclosing unsafe block or unsafe fn that makes it unnecessary.

Fixes #39144.

@rust-highfive
Copy link
Collaborator

r? @arielb1

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

@estebank
Copy link
Contributor Author

Another option is to use span_label instead of span_help:

@durka
Copy link
Contributor

durka commented Jan 21, 2017

Might want to carve out an exception for unsafe blocks within macros.

@estebank
Copy link
Contributor Author

cc @nikomatsakis @jonathandturner

@nikomatsakis
Copy link
Contributor

@estebank this seems nice; but it'd be nicer still if we used a more targeted span (i.e., just the unsafe keyword).

@estebank
Copy link
Contributor Author

estebank commented Feb 2, 2017

@nikomatsakis There's only one reason I'm reticent to that suggestion: The current presentation will only be as shown if the block is less than 8 lines (showing only the first line that contains the keyword otherwise), and I'm looking into making the multiline presentation smarter about longer blocks (instead of using the first line, showing the first X and last Y lines). I feel it is useful to know both the start and end of the enclosing block (as not everyone will know about "jump to closing brace" in their editor) when possible.

What do you think?

@nikomatsakis
Copy link
Contributor

@estebank I think people can find the end of their blocks ok, personally. But also, I sort of like the "collapsed" error where both of the unsafe blocks are shown simultaneously, but

  • we should make sure to put a label on the primary span (i.e., ^^^ unnecessary unsafe block), since it looks really weird otherwise
  • I guess it just seems kind of heavy with the ascii art; I'm very happy we have it, but I'd prefer for it to be more unusual

@arielb1
Copy link
Contributor

arielb1 commented Feb 26, 2017

Personally, I don't like errors with separate spans - it blasts the console too much.

@arielb1
Copy link
Contributor

arielb1 commented Feb 26, 2017

The code seems ok. I think r? @jonathandturner

@rust-highfive rust-highfive assigned sophiajt and unassigned arielb1 Feb 26, 2017
@sophiajt
Copy link
Contributor

Looks good.

@bors r+

@bors
Copy link
Contributor

bors commented Feb 26, 2017

📌 Commit f31c8b7 has been approved by jonathandturner

@bors
Copy link
Contributor

bors commented Feb 26, 2017

⌛ Testing commit f31c8b7 with merge 2ae8de7...

@bors
Copy link
Contributor

bors commented Feb 26, 2017

💔 Test failed - status-travis

@sophiajt
Copy link
Contributor

Seems like a legit failure:

error: no field `map` on type `rustc::ty::TyCtxt<'_, '_, '_>`

   --> /checkout/src/librustc_lint/unused.rs:194:36

    |

194 |             let parent_id = cx.tcx.map.get_parent_node(id);

    |                                    ^^^ unknown field

error: no field `map` on type `rustc::ty::TyCtxt<'_, '_, '_>`

   --> /checkout/src/librustc_lint/unused.rs:201:30

    |

201 |                 })) = cx.tcx.map.find(parent_id) {

    |                              ^^^ unknown field

error: no field `map` on type `rustc::ty::TyCtxt<'_, '_, '_>`

   --> /checkout/src/librustc_lint/unused.rs:219:41

    |

219 |                     db.span_note(cx.tcx.map.span(id),

    |                                         ^^^ unknown field

@hanna-kruppe
Copy link
Contributor

FYI: The fix for this is simply using tcx.hir instead of tcx.map.

@estebank estebank force-pushed the nested-unsafe branch 2 times, most recently from f0c6be0 to 3a9560a Compare March 5, 2017 17:51
@estebank
Copy link
Contributor Author

estebank commented Mar 5, 2017

@jonathandturner fixed merge conflict and added label to the block.

@sophiajt
Copy link
Contributor

sophiajt commented Mar 6, 2017

@bors r+

@bors
Copy link
Contributor

bors commented Mar 6, 2017

📌 Commit 3a9560a has been approved by jonathandturner

frewsxcv added a commit to frewsxcv/rust that referenced this pull request Mar 6, 2017
…turner

Point to enclosing block/fn on nested unsafe

When declaring nested unsafe blocks (`unsafe {unsafe {}}`) that trigger
the "unnecessary `unsafe` block" error, point out the enclosing `unsafe
block` or `unsafe fn` that makes it unnecessary.

<img width="621" alt="" src="https://cloud.githubusercontent.com/assets/1606434/22139922/26ad468a-de9e-11e6-8884-2945be882ea8.png">

Fixes rust-lang#39144.
@bors
Copy link
Contributor

bors commented Mar 10, 2017

🔒 Merge conflict

@alexcrichton
Copy link
Member

@bors: retry

alexcrichton added a commit to alexcrichton/rust that referenced this pull request Mar 10, 2017
…turner

Point to enclosing block/fn on nested unsafe

When declaring nested unsafe blocks (`unsafe {unsafe {}}`) that trigger
the "unnecessary `unsafe` block" error, point out the enclosing `unsafe
block` or `unsafe fn` that makes it unnecessary.

<img width="621" alt="" src="https://cloud.githubusercontent.com/assets/1606434/22139922/26ad468a-de9e-11e6-8884-2945be882ea8.png">

Fixes rust-lang#39144.
@bors
Copy link
Contributor

bors commented Mar 10, 2017

🔒 Merge conflict

When declaring nested unsafe blocks (`unsafe {unsafe {}}`) that trigger
the "unnecessary `unsafe` block" error, point out the enclosing `unsafe
block` or `unsafe fn` that makes it unnecessary.
@alexcrichton
Copy link
Member

@bors: r=jonathandturner

@bors
Copy link
Contributor

bors commented Mar 10, 2017

📌 Commit ac2bc7c has been approved by jonathandturner

alexcrichton added a commit to alexcrichton/rust that referenced this pull request Mar 10, 2017
…turner

Point to enclosing block/fn on nested unsafe

When declaring nested unsafe blocks (`unsafe {unsafe {}}`) that trigger
the "unnecessary `unsafe` block" error, point out the enclosing `unsafe
block` or `unsafe fn` that makes it unnecessary.

<img width="621" alt="" src="https://cloud.githubusercontent.com/assets/1606434/22139922/26ad468a-de9e-11e6-8884-2945be882ea8.png">

Fixes rust-lang#39144.
arielb1 pushed a commit to arielb1/rust that referenced this pull request Mar 10, 2017
…turner

Point to enclosing block/fn on nested unsafe

When declaring nested unsafe blocks (`unsafe {unsafe {}}`) that trigger
the "unnecessary `unsafe` block" error, point out the enclosing `unsafe
block` or `unsafe fn` that makes it unnecessary.

<img width="621" alt="" src="https://cloud.githubusercontent.com/assets/1606434/22139922/26ad468a-de9e-11e6-8884-2945be882ea8.png">

Fixes rust-lang#39144.
arielb1 pushed a commit to arielb1/rust that referenced this pull request Mar 10, 2017
…turner

Point to enclosing block/fn on nested unsafe

When declaring nested unsafe blocks (`unsafe {unsafe {}}`) that trigger
the "unnecessary `unsafe` block" error, point out the enclosing `unsafe
block` or `unsafe fn` that makes it unnecessary.

<img width="621" alt="" src="https://cloud.githubusercontent.com/assets/1606434/22139922/26ad468a-de9e-11e6-8884-2945be882ea8.png">

Fixes rust-lang#39144.
arielb1 pushed a commit to arielb1/rust that referenced this pull request Mar 10, 2017
…turner

Point to enclosing block/fn on nested unsafe

When declaring nested unsafe blocks (`unsafe {unsafe {}}`) that trigger
the "unnecessary `unsafe` block" error, point out the enclosing `unsafe
block` or `unsafe fn` that makes it unnecessary.

<img width="621" alt="" src="https://cloud.githubusercontent.com/assets/1606434/22139922/26ad468a-de9e-11e6-8884-2945be882ea8.png">

Fixes rust-lang#39144.
alexcrichton added a commit to alexcrichton/rust that referenced this pull request Mar 10, 2017
…turner

Point to enclosing block/fn on nested unsafe

When declaring nested unsafe blocks (`unsafe {unsafe {}}`) that trigger
the "unnecessary `unsafe` block" error, point out the enclosing `unsafe
block` or `unsafe fn` that makes it unnecessary.

<img width="621" alt="" src="https://cloud.githubusercontent.com/assets/1606434/22139922/26ad468a-de9e-11e6-8884-2945be882ea8.png">

Fixes rust-lang#39144.
@bors bors merged commit ac2bc7c into rust-lang:master Mar 11, 2017
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.

10 participants