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

Don't ICE for kind mismatches during error rendering #123673

Merged
merged 3 commits into from
Apr 17, 2024

Conversation

oli-obk
Copy link
Contributor

@oli-obk oli-obk commented Apr 9, 2024

fixes #123457

also some test suite cleanups to make backtraces easier to read

@rustbot
Copy link
Collaborator

rustbot commented Apr 9, 2024

r? @estebank

rustbot has assigned @estebank.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added A-testsuite Area: The testsuite used to check the correctness of rustc S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Apr 9, 2024
@rustbot
Copy link
Collaborator

rustbot commented Apr 9, 2024

Some changes occurred in src/tools/compiletest

cc @jieyouxu

Copy link
Member

@jieyouxu jieyouxu left a comment

Choose a reason for hiding this comment

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

Thanks! Super minor nits, then r=me
r? @jieyouxu

@@ -0,0 +1,23 @@
//! THis test used to ICE in typeck because of the type/const mismatch,
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
//! THis test used to ICE in typeck because of the type/const mismatch,
//! This test used to ICE in typeck because of the type/const mismatch,

@@ -0,0 +1,23 @@
//! THis test used to ICE in typeck because of the type/const mismatch,
//! even though wfcheck already errored.
Copy link
Member

Choose a reason for hiding this comment

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

Please add an issue number for ICE test:

Suggested change
//! even though wfcheck already errored.
//! even though wfcheck already errored.
//! issue: rust-lang/rust#123457

Copy link
Contributor

Choose a reason for hiding this comment

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

As nice as it is to have the issue in the test itself (and helps with traceability in the future even when files move around), I find that 99% of the time I'm able to get to the ticket by finding the PR that introduced the file. It's good practice to add as much info as possible inline, but we should avoid holding up a PR when that's the only comment to address (some new contributors will just not have the bandwidth to address comments and might get put off by such a request, where accepting their PRs directly increases the likelihood of them coming back).

I'm wondering if we could try and automate this, make .x,py tidy check for test files that are introduced in the current branch that aren't in master, check against github to see if there's an open PR for the current branch, and if that PR says Fixes #XXXXXX. If so, complain about the lack of a link :)

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, probably a good idea to make this somewhat automated

@rustbot rustbot assigned jieyouxu and unassigned estebank Apr 9, 2024
@bors
Copy link
Contributor

bors commented Apr 9, 2024

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

Copy link
Contributor

@estebank estebank left a comment

Choose a reason for hiding this comment

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

r=me after rebase and addressing comments

@oli-obk
Copy link
Contributor Author

oli-obk commented Apr 16, 2024

@bors r=jieyouxu,estebank

@bors
Copy link
Contributor

bors commented Apr 16, 2024

📌 Commit 0a88339 has been approved by jieyouxu,estebank

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 16, 2024
bors added a commit to rust-lang-ci/rust that referenced this pull request Apr 17, 2024
…llaumeGomez

Rollup of 7 pull requests

Successful merges:

 - rust-lang#123673 (Don't ICE for kind mismatches during error rendering)
 - rust-lang#123675 (Taint const qualifs if a static is referenced that didn't pass wfcheck)
 - rust-lang#123975 (Port the 2 `rust-lld` run-make tests to `rmake`)
 - rust-lang#124000 (Use `/* value */` as a placeholder)
 - rust-lang#124013 (Box::into_raw: make Miri understand that this is a box-to-raw cast)
 - rust-lang#124027 (Prefer identity equality over equating types during coercion.)
 - rust-lang#124036 (Remove `default_hidden_visibility: false` from wasm targets)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 4aaa8f9 into rust-lang:master Apr 17, 2024
12 checks passed
@rustbot rustbot added this to the 1.79.0 milestone Apr 17, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Apr 17, 2024
Rollup merge of rust-lang#123673 - oli-obk:sig_wfcheck_ice, r=jieyouxu,estebank

Don't ICE for kind mismatches during error rendering

fixes rust-lang#123457

also some test suite cleanups to make backtraces easier to read
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-testsuite Area: The testsuite used to check the correctness of rustc S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ICE: relating different kinds: {const error}: &'static str KeyNil
5 participants