Skip to content

Point to enclosing block/fn on nested unsafe#39202

Merged
bors merged 1 commit into
rust-lang:masterfrom
estebank:nested-unsafe
Mar 11, 2017
Merged

Point to enclosing block/fn on nested unsafe#39202
bors merged 1 commit into
rust-lang:masterfrom
estebank:nested-unsafe

Conversation

@estebank

Copy link
Copy Markdown
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
Copy Markdown
Contributor

r? @arielb1

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

@estebank

Copy link
Copy Markdown
Contributor Author

Another option is to use span_label instead of span_help:

@durka

durka commented Jan 21, 2017

Copy link
Copy Markdown
Contributor

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

@estebank

Copy link
Copy Markdown
Contributor Author

cc @nikomatsakis @jonathandturner

@nikomatsakis

Copy link
Copy Markdown
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

estebank commented Feb 2, 2017

Copy link
Copy Markdown
Contributor Author

@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
Copy Markdown
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

arielb1 commented Feb 26, 2017

Copy link
Copy Markdown
Contributor

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

@arielb1

arielb1 commented Feb 26, 2017

Copy link
Copy Markdown
Contributor

The code seems ok. I think r? @jonathandturner

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

Copy link
Copy Markdown
Contributor

Looks good.

@bors r+

@bors

bors commented Feb 26, 2017

Copy link
Copy Markdown
Collaborator

📌 Commit f31c8b7 has been approved by jonathandturner

@bors

bors commented Feb 26, 2017

Copy link
Copy Markdown
Collaborator

⌛ Testing commit f31c8b7 with merge 2ae8de7...

@bors

bors commented Feb 26, 2017

Copy link
Copy Markdown
Collaborator

💔 Test failed - status-travis

@sophiajt

Copy link
Copy Markdown
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
Copy Markdown
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

estebank commented Mar 5, 2017

Copy link
Copy Markdown
Contributor Author

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

@sophiajt

sophiajt commented Mar 6, 2017

Copy link
Copy Markdown
Contributor

@bors r+

@bors

bors commented Mar 6, 2017

Copy link
Copy Markdown
Collaborator

📌 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

bors commented Mar 10, 2017

Copy link
Copy Markdown
Collaborator

🔒 Merge conflict

@alexcrichton

Copy link
Copy Markdown
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

bors commented Mar 10, 2017

Copy link
Copy Markdown
Collaborator

🔒 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
Copy Markdown
Member

@bors: r=jonathandturner

@bors

bors commented Mar 10, 2017

Copy link
Copy Markdown
Collaborator

📌 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