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

Add custom ICE message that points to Clippy repo #4588

Merged
merged 8 commits into from Nov 29, 2019

Conversation

phansch
Copy link
Member

@phansch phansch commented Sep 27, 2019

changelog: Link to Clippy issue tracker in ICE messages

This utilizes rust-lang/rust#60584 by setting
our own panic_hook and pointing to our own issue tracker instead of
the rustc issue tracker.

This also adds a new internal lint to test the ICE message.

Potential downsides

  • This essentially copies rustc's report_ice function as
    report_clippy_ice. I think that's how it's meant to be implemented, but
    maybe @jonas-schievink could have a look as well =)

    The downside of more-or-less copying this function is that we have to
    maintain it as well now.
    The original function can be found here.

  • driver now depends directly on rustc and rustc_errors

Closes #2734

src/driver.rs Outdated Show resolved Hide resolved
@phansch
Copy link
Member Author

@phansch phansch commented Sep 27, 2019

Looks like the test fails because locally I've been running without RUST_BACKTRACE=1, but travis runs all tests with RUST_BACKTRACE=1.

Is there a way to set the backtrace level in a compiletest header? If I just re-record the stderr output with RUST_BACKTRACE=1, it will fail for people locally when running without backtraces enabled.

@phansch phansch added the S-waiting-on-author label Sep 27, 2019
@jonas-schievink
Copy link
Member

@jonas-schievink jonas-schievink commented Sep 27, 2019

This essentially copies rustc's report_ice function as
report_clippy_ice. I think that's how it's meant to be implemented, but
maybe @jonas-schievink could have a look as well =)

You can also call rustc_driver::report_ice and pass your custom URL, as long as that's all you want to customize.

fn is_trigger_fn(fn_kind: FnKind<'_>) -> bool {
match fn_kind {
FnKind::ItemFn(ident, ..) | FnKind::Method(ident, ..) => {
ident.name.as_str() == "should_trigger_an_ice_in_clippy"
Copy link
Member

@flip1995 flip1995 Sep 27, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Though Clippy internal lints are never emitted by Clippy, they still run all the time. I don't think that anyone would name a function should_trigger_an_ice_in_clippy, but I suggest to change this name to one of the following:

  1. should_trigger_an_ice_when_clippy_is_run
  2. Make it a Clippy easteregg and give it a cool name 😎 it_looks_like_you_are_trying_to_kill_clippy or similar. (https://twitter.com/ManishEarth/status/1107297057604145152)

Copy link
Member

@Manishearth Manishearth Oct 4, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ooh i like the easter egg idea

Copy link
Member

@Manishearth Manishearth left a comment

r+ with the function name changed 😄

@phansch
Copy link
Member Author

@phansch phansch commented Oct 9, 2019

I updated the function name and hopefully fixed the test 🙏

I decided to keep the copy of rustc's report_ice function, because that allows us to print the Clippy version in the ICE message. I think that's worth the extra code (and I could take ownership of that, if something breaks).

@bors r=Manishearth

@bors
Copy link
Contributor

@bors bors commented Oct 9, 2019

📌 Commit 1814964 has been approved by Manishearth

bors added a commit that referenced this issue Oct 9, 2019
Add custom ICE message that points to Clippy repo

changelog: Link to Clippy issue tracker in ICE messages

This utilizes rust-lang/rust#60584 by setting
our own `panic_hook` and pointing to our own issue tracker instead of
the rustc issue tracker.

This also adds a new internal lint to test the ICE message.

**Potential downsides**

* This essentially copies rustc's `report_ice` function as
  `report_clippy_ice`. I think that's how it's meant to be implemented, but
  maybe @jonas-schievink could have a look as well =)

  The downside of more-or-less copying this function is that we have to
  maintain it as well now.
  The original function can be found [here][original].
* `driver` now depends directly on `rustc` and `rustc_errors`

Closes #2734

[original]: https://github.com/rust-lang/rust/blob/59367b074f1523353dddefa678ffe3cac9fd4e50/src/librustc_driver/lib.rs#L1185
@bors
Copy link
Contributor

@bors bors commented Oct 9, 2019

Testing commit 1814964 with merge e261df4...

@bors
Copy link
Contributor

@bors bors commented Oct 9, 2019

💡 This pull request was already approved, no need to approve it again.

  • This pull request is currently being tested. If there's no response from the continuous integration service, you may use retry to trigger a build again.

@bors
Copy link
Contributor

@bors bors commented Oct 9, 2019

📌 Commit 1814964 has been approved by Manishearth

bors added a commit that referenced this issue Oct 9, 2019
Add custom ICE message that points to Clippy repo

changelog: Link to Clippy issue tracker in ICE messages

This utilizes rust-lang/rust#60584 by setting
our own `panic_hook` and pointing to our own issue tracker instead of
the rustc issue tracker.

This also adds a new internal lint to test the ICE message.

**Potential downsides**

* This essentially copies rustc's `report_ice` function as
  `report_clippy_ice`. I think that's how it's meant to be implemented, but
  maybe @jonas-schievink could have a look as well =)

  The downside of more-or-less copying this function is that we have to
  maintain it as well now.
  The original function can be found [here][original].
* `driver` now depends directly on `rustc` and `rustc_errors`

Closes #2734

[original]: https://github.com/rust-lang/rust/blob/59367b074f1523353dddefa678ffe3cac9fd4e50/src/librustc_driver/lib.rs#L1185
@bors
Copy link
Contributor

@bors bors commented Oct 9, 2019

Testing commit 1814964 with merge 64d77b2...

@bors
Copy link
Contributor

@bors bors commented Oct 9, 2019

💔 Test failed - checks-travis

@phansch
Copy link
Member Author

@phansch phansch commented Oct 9, 2019

Dogfood tests are failing, will fix them later today

@jonas-schievink
Copy link
Member

@jonas-schievink jonas-schievink commented Oct 9, 2019

@phansch did you see #4588 (comment)?

@phansch
Copy link
Member Author

@phansch phansch commented Oct 9, 2019

@phansch did you see #4588 (comment)?

Yup, thanks! It's just that it would be nice to also show the Clippy version instead of the rustc version in the ICE message. I don't think that's possible when using rustc_driver::report_ice?

@Manishearth
Copy link
Member

@Manishearth Manishearth commented Oct 9, 2019

@phansch
Copy link
Member Author

@phansch phansch commented Oct 9, 2019

We don't really version Clippy separately

Right, I meant the specific commit hash of the used Clippy. This could be useful for debugging nightly Clippy issues?

@jonas-schievink
Copy link
Member

@jonas-schievink jonas-schievink commented Oct 9, 2019

I see, that makes sense. One could also figure that out by looking at the rustc commit, but that's more work.

@phansch phansch added S-waiting-on-review and removed S-waiting-on-author labels Oct 11, 2019
@bors
Copy link
Contributor

@bors bors commented Oct 14, 2019

The latest upstream changes (presumably #4663) made this pull request unmergeable. Please resolve the merge conflicts.

@phansch phansch added S-waiting-on-author and removed S-waiting-on-review labels Nov 3, 2019
@phansch phansch removed the S-waiting-on-author label Nov 16, 2019
@bors
Copy link
Contributor

@bors bors commented Nov 23, 2019

The latest upstream changes (presumably #4825) made this pull request unmergeable. Please resolve the merge conflicts.

@phansch phansch added the S-waiting-on-author label Nov 28, 2019
@phansch phansch self-assigned this Nov 28, 2019
@phansch
Copy link
Member Author

@phansch phansch commented Nov 29, 2019

@bors r=Manishearth

@bors
Copy link
Contributor

@bors bors commented Nov 29, 2019

📌 Commit 6b5dd79 has been approved by Manishearth

@bors
Copy link
Contributor

@bors bors commented Nov 29, 2019

🔒 Merge conflict

This pull request and the master branch diverged in a way that cannot be automatically merged. Please rebase on top of the latest master branch, and let the reviewer approve again.

How do I rebase?

Assuming self is your fork and upstream is this repository, you can resolve the conflict following these steps:

  1. git checkout add_custom_ice_hook (switch to your branch)
  2. git fetch upstream master (retrieve the latest master)
  3. git rebase upstream/master -p (rebase on top of it)
  4. Follow the on-screen instruction to resolve conflicts (check git status if you got lost).
  5. git push self add_custom_ice_hook --force-with-lease (update this PR)

You may also read Git Rebasing to Resolve Conflicts by Drew Blessing for a short tutorial.

Please avoid the "Resolve conflicts" button on GitHub. It uses git merge instead of git rebase which makes the PR commit history more difficult to read.

Sometimes step 4 will complete without asking for resolution. This is usually due to difference between how Cargo.lock conflict is handled during merge and rebase. This is normal, and you should still perform step 5 to update this PR.

Error message
Auto-merging clippy_lints/src/lib.rs
Auto-merging Cargo.toml
CONFLICT (content): Merge conflict in Cargo.toml
Automatic merge failed; fix conflicts and then commit the result.

@bors
Copy link
Contributor

@bors bors commented Nov 29, 2019

The latest upstream changes (presumably #4837) made this pull request unmergeable. Please resolve the merge conflicts.

phansch added 8 commits Nov 29, 2019
This utilizes rust-lang/rust#60584 by setting
our own `panic_hook` and pointing to our own issue tracker instead of
the rustc issue tracker.

This also adds a new internal lint to test the ICE message.

**Potential downsides**

* This essentially copies rustc's `report_ice` function as
  `report_clippy_ice`. I think that's how it's meant to be implemented, but
  maybe @jonas-schievink could have a look as well =)

  The downside of more-or-less copying this function is that we have to
  maintain it as well now.
  The original function can be found [here][original].
* `driver` now depends directly on `rustc` and `rustc_errors`

Closes rust-lang#2734

[original]: https://github.com/rust-lang/rust/blob/59367b074f1523353dddefa678ffe3cac9fd4e50/src/librustc_driver/lib.rs#L1185
@phansch
Copy link
Member Author

@phansch phansch commented Nov 29, 2019

@bors r=Manishearth

@bors
Copy link
Contributor

@bors bors commented Nov 29, 2019

📌 Commit c1ccba0 has been approved by Manishearth

bors added a commit that referenced this issue Nov 29, 2019
Add custom ICE message that points to Clippy repo

changelog: Link to Clippy issue tracker in ICE messages

This utilizes rust-lang/rust#60584 by setting
our own `panic_hook` and pointing to our own issue tracker instead of
the rustc issue tracker.

This also adds a new internal lint to test the ICE message.

**Potential downsides**

* This essentially copies rustc's `report_ice` function as
  `report_clippy_ice`. I think that's how it's meant to be implemented, but
  maybe @jonas-schievink could have a look as well =)

  The downside of more-or-less copying this function is that we have to
  maintain it as well now.
  The original function can be found [here][original].
* `driver` now depends directly on `rustc` and `rustc_errors`

Closes #2734

[original]: https://github.com/rust-lang/rust/blob/59367b074f1523353dddefa678ffe3cac9fd4e50/src/librustc_driver/lib.rs#L1185
@bors
Copy link
Contributor

@bors bors commented Nov 29, 2019

Testing commit c1ccba0 with merge 0e66a78...

@bors
Copy link
Contributor

@bors bors commented Nov 29, 2019

☀️ Test successful - checks-travis, status-appveyor
Approved by: Manishearth
Pushing 0e66a78 to master...

@bors bors merged commit c1ccba0 into rust-lang:master Nov 29, 2019
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-author
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants