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

INSP: Implement unreachable code detection #6829

Merged
merged 2 commits into from Mar 25, 2021
Merged

Conversation

artemmukhin
Copy link
Member

@artemmukhin artemmukhin commented Feb 11, 2021

This PR introduces Unreachable code inspection to detect and remove unreachable code.

unreachable_code

Note that now it might produce some false-positive warnings due to #6749 and #6797, so probably it's better to try fixing these issues before merging this.

Some implementation details:

  • Based on the control-flow graph which is already used for several inspections (move analysis, liveness analysis)
  • Found unreachable elements are cached by org.rust.lang.core.types.ExtensionsKt#CONTROL_FLOW_KEY as well as the graph itself
  • Does not highlight each unreachable element separately, but merges their text ranges instead in order to highlight the widest range

Part of #1612.

changelog: Add Unreachable code inspection and quick-fix

@artemmukhin artemmukhin force-pushed the ortem/unreachable-code-insp branch 2 times, most recently from 9206e64 to 0b7bb51 Compare February 28, 2021 00:43
@artemmukhin artemmukhin requested a review from dima74 March 16, 2021 17:22
@artemmukhin
Copy link
Member Author

Note, although no regressions are found with CI (because the regression tests only search for error annotations, not warnings), there are some false positive warnings in real projects, mostly due to #6749, as I mentioned in the PR description.

Here is what I found:

As we just briefly investigated with @vlad20012, CoerceMany should be implemented in order to fix #6749. Currently, IJ Rust uses a simplified and rough approach to handle this (see RsTypeInferenceWalker#getMoreCompleteType). So #6749 is probably not very easy to fix right now, and I don't know a good and simple workaround to avoid analyzing such problematic match expressions since many of them are expanded from macros.

However, I think we can merge this inspection because the number of false positives is not so much. @Undin WDYT?

@Undin
Copy link
Member

Undin commented Mar 22, 2021

However, I think we can merge this inspection because the number of false positives is not so much. @Undin WDYT?

As we've discussed in person - let's try to fix type inference first not to introduce new false positive annotations for users. Looks like we can come up with at least some hack to make type inference work in described cases.
As an alternative solution, I may suggest merging the inspection right now but disabled by default. And enable it when type inference will be fixed. It may help to avoid merge conflicts

@Undin
Copy link
Member

Undin commented Mar 22, 2021

@ortem BTW tests fail

@Undin
Copy link
Member

Undin commented Mar 23, 2021

@ortem #6749 is fixed so I suppose we can merge this PR

@artemmukhin
Copy link
Member Author

@Undin I'd like not to merge this PR right now because some false-positive warnings remained, with #6749 fixed:

  • cargo: no false-positive warnings
  • diesel: 1 file affected due to unsupported #[allow(warnings)] attribute Compiler lints support #5888
  • tokio: 2 files are still affected due to tokio::select! macro, 1 file affected due to an unevaluated cfg option
  • rust-analyzer: no false-positive warnings

So the most interesting case is tokio. I've found a problem with the processing of break expanded from a macro:

macro_rules! break_macro {
    () => { break };
}

fn main() {
    loop {
        break_macro!();
    }
    1; // false-positive "unreachable"
}

I'm investigating the problem. If I don't find a solution soon then I'll disable the inspection by default and merge this PR.

@artemmukhin
Copy link
Member Author

I opened an issue for this case: #7016.
Now it is fixed and there are no false positives with tokio::select! macro in tokio. So this can be merged I suppose.

@Undin
Copy link
Member

Undin commented Mar 24, 2021

@ortem don't insist but I'd personally prefer to create a separate PR for #7016. For example, it's convenient for changelog creation

@artemmukhin
Copy link
Member Author

bors r=dima74

bors bot added a commit that referenced this pull request Mar 25, 2021
6829: INSP: Implement unreachable code detection r=dima74 a=ortem

This PR introduces `Unreachable code` inspection to detect and remove unreachable code.

![unreachable_code](https://user-images.githubusercontent.com/4854600/107639621-2ce9cc00-6c82-11eb-8a06-114953a733ab.gif)

Note that now it might produce some false-positive warnings due to #6749 and #6797, so probably it's better to try fixing these issues before merging this. 

Some implementation details:
* Based on the control-flow graph which is already used for several inspections (move analysis, liveness analysis)
* Found unreachable elements are cached by `org.rust.lang.core.types.ExtensionsKt#CONTROL_FLOW_KEY` as well as the  graph itself
* Does not highlight each unreachable element separately, but merges their text ranges instead in order to highlight the widest range

Part of #1612.

changelog: Add `Unreachable code` inspection and quick-fix

6969: COMP: Improve dependency completion when using crates local index r=ortem a=avrong

<img width="400" src="https://user-images.githubusercontent.com/6342851/111127949-6c5b4f00-8585-11eb-8a1f-e1284d32009d.gif">

changelog: Improve performance of dependencies completion in `Cargo.toml` using crates local index. This feature is disabled by default for now. To use it, enable `org.rust.crates.local.index` experimental feature


Co-authored-by: ortem <ortem00@gmail.com>
Co-authored-by: Aleksei Trifonov <avrong@avrong.me>
@bors
Copy link
Contributor

bors bot commented Mar 25, 2021

This PR was included in a batch that timed out, it will be automatically retried

@bors
Copy link
Contributor

bors bot commented Mar 25, 2021

Build succeeded:

@bors bors bot merged commit 81a9337 into master Mar 25, 2021
@bors bors bot deleted the ortem/unreachable-code-insp branch March 25, 2021 20:50
@github-actions github-actions bot added this to the v145 milestone Mar 25, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants