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

Reduce span to function name in unreachable calls #64229

Merged
merged 1 commit into from Sep 8, 2019

Conversation

@kawa-yoiko
Copy link
Contributor

commented Sep 6, 2019

As title suggests, this might close #64103. Refer to the updated tests for expected output.

There is potential to further improve usability. In particular, is it favourable that the exact diverging expression/statement be pointed out (not only in this case, but for all unreachable code)? Certainly that would deserve another issue, but I'm interested in the opinions.

@rust-highfive

This comment has been minimized.

Copy link
Collaborator

commented Sep 6, 2019

r? @petrochenkov

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

@petrochenkov

This comment has been minimized.

Copy link
Contributor

commented Sep 6, 2019

r? @estebank for reassigning, can't spend time on digging into this right now

@petrochenkov

This comment has been minimized.

Copy link
Contributor

commented Sep 6, 2019

(The PR as is doesn't seem to address the issue?)

@estebank

This comment has been minimized.

Copy link
Contributor

commented Sep 6, 2019

The reduced span to the call itself makes it slightly easier to follow what is going on, but as you point out the only thing that will actually make this clear is to actually point at the diverging expression (panic!(), return, etc.) that is the underlying cause for the call itself to be unreachable. Would you be open to investigate what would be required to accomplish that?

Beyond that r=me on this PR once it's green.

@kawa-yoiko

This comment has been minimized.

Copy link
Contributor Author

commented Sep 7, 2019

Thanks for the prompt reply!

I'd love to dig into this! For now, I think a viable solution is to store the span of the diverging Expr/Stmt/Block inside the Diverges enum, which can aid the lint for all types of unreachable code. All changes are still in crate librustc_typeck.

@Centril

This comment has been minimized.

Copy link
Member

commented Sep 7, 2019

I'd like to not complicate Diverges further at this point and instead go for the simpler fix in this PR.

@kawa-yoiko

This comment has been minimized.

Copy link
Contributor Author

commented Sep 7, 2019

Would you mind explaining the reason? And will it be useful if the diverging expression is pointed out for every unreachable code warning? The only downside I can think of is becoming too verbose, but there is always #[allow(unreachable_code)].

@Centril

This comment has been minimized.

Copy link
Member

commented Sep 7, 2019

Would you mind explaining the reason?

Mainly internal compiler complexity reasons, but if it turns out it does not substantially add to the complexity of understanding how Diverges works then go for it.

@estebank

This comment has been minimized.

Copy link
Contributor

commented Sep 7, 2019

@Centril I can see it being as "cheap" as adding a single span: Cell<Option<Span>> field in FnCtxt pointing at the last divergent path...

@estebank

This comment has been minimized.

Copy link
Contributor

commented Sep 7, 2019

Lets do this, lets merge this PR and @kawa-yoiko can work on the enhancement in a follow up that @Centril and I can review and have a longer convo if needed there. This as is is already a clear, if smaller, improvement.

@bors r+

@bors

This comment has been minimized.

Copy link
Contributor

commented Sep 7, 2019

📌 Commit e1d27eb has been approved by estebank

Centril added a commit to Centril/rust that referenced this pull request Sep 7, 2019
Rollup merge of rust-lang#64229 - kawa-yoiko:unreachable-call-lint, r…
…=estebank

Reduce span to function name in unreachable calls

As title suggests, this might close rust-lang#64103. Refer to the updated tests for expected output.

There is potential to further improve usability. In particular, is it favourable that the exact diverging expression/statement be pointed out (not only in this case, but for all unreachable code)? Certainly that would deserve another issue, but I'm interested in the opinions.
@Centril Centril referenced this pull request Sep 7, 2019
bors added a commit that referenced this pull request Sep 7, 2019
Auto merge of #64269 - Centril:rollup-y4dm32c, r=Centril
Rollup of 5 pull requests

Successful merges:

 - #64052 (Rename test locals to work around LLDB bug)
 - #64066 (Support "soft" feature-gating using a lint)
 - #64177 (resolve: Do not afraid to set current module to enums and traits)
 - #64229 (Reduce span to function name in unreachable calls)
 - #64255 (Add methods for converting `bool` to `Option<T>`)

Failed merges:

r? @ghost

@bors bors merged commit e1d27eb into rust-lang:master Sep 8, 2019

4 checks passed

pr Build #20190906.38 succeeded
Details
pr (Linux mingw-check) Linux mingw-check succeeded
Details
pr (Linux x86_64-gnu-llvm-6.0) Linux x86_64-gnu-llvm-6.0 succeeded
Details
pr (LinuxTools) LinuxTools succeeded
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.