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.

Sign up for GitHub

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

Jump to bottom

Fix ICE in improper_ctypes_definitions lint with all-ZST transparent types #87529

Merged
merged 1 commit into from
Sep 17, 2021
Merged

Fix ICE in improper_ctypes_definitions lint with all-ZST transparent types #87529

merged 1 commit into from
Sep 17, 2021

Conversation

FabianWolff
Copy link
Contributor

Fixes #87496. There is also another function in the same file that looks fishy, but I haven't been able to produce an ICE there, and in any case, it's not related to #87496:

rust/compiler/rustc_lint/src/types.rs

Lines 720 to 734 in fd853c0

fn get_nullable_type<'tcx>(cx: &LateContext<'tcx>, ty: Ty<'tcx>) -> Option<Ty<'tcx>> {
let tcx = cx.tcx;
Some(match *ty.kind() {
ty::Adt(field_def, field_substs) => {
let inner_field_ty = {
let first_non_zst_ty =
field_def.variants.iter().filter_map(|v| transparent_newtype_field(cx.tcx, v));
debug_assert_eq!(
first_non_zst_ty.clone().count(),
1,
"Wrong number of fields for transparent type"
);
first_non_zst_ty
.last()
.expect("No non-zst fields in transparent type.")

r? @JohnTitor

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 27, 2021
@FabianWolff FabianWolff mentioned this pull request Jul 27, 2021
Closed
hellow554
hellow554 reviewed Jul 28, 2021
View reviewed changes
src/test/ui/repr/repr-transparent-issue-87496.rs Outdated
Comment on lines 6 to 10
struct TransparentCustomZst(());
extern "C" {
fn good17(p: TransparentCustomZst);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

you could make the struct and extern fn pub to avoid warnings :)

@JohnTitor
Copy link
Member

LGTM, thanks for the follow-up! But I'm not part of t-compiler, so r? @nikomatsakis for the final review

@camelid camelid added the A-FFI Area: Foreign function interface (FFI) label Jul 29, 2021
nikomatsakis
nikomatsakis requested changes Jul 30, 2021
View reviewed changes
Copy link
Contributor

@nikomatsakis nikomatsakis left a comment

Choose a reason for hiding this comment

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

Per @camelid's point, the behavior for "all ZST fields" seems incorrect.

@FabianWolff
Copy link
Contributor Author

@rustbot ready

@JohnCSimon JohnCSimon added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 15, 2021
@inquisitivecrystal inquisitivecrystal added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Aug 24, 2021
@JohnCSimon
Copy link
Member

Triage:
@FabianWolff can you mark the change request from nikomatsakis as completed if you've completed it? Thanks.

@FabianWolff
Copy link
Contributor Author

Triage:
@FabianWolff can you mark the change request from nikomatsakis as completed if you've completed it? Thanks.

@JohnCSimon What exactly do you mean by this? I thought that's what the @rustbot ready is for (though it doesn't really seem to have had an effect above...). In any case, I've clicked on "Re-request review" for now.

@nikomatsakis
Copy link
Contributor

Ugh! Sorry @FabianWolff, reading now!

@nikomatsakis
Copy link
Contributor

@bors r+

@bors
Copy link
Contributor

bors commented Sep 15, 2021

📌 Commit 4e76c38 has been approved by nikomatsakis

@bors
Copy link
Contributor

bors commented Sep 15, 2021

🌲 The tree is currently closed for pull requests below priority 100. This pull request will be tested once the tree is reopened.

@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 Sep 15, 2021
Manishearth added a commit to Manishearth/rust that referenced this pull request Sep 16, 2021
@Manishearth
Rollup merge of rust-lang#87529 - FabianWolff:issue-87496, r=nikomats…
4078cbf 
…akis

Fix ICE in `improper_ctypes_definitions` lint with all-ZST transparent types

Fixes rust-lang#87496. There is also another function in the same file that looks fishy, but I haven't been able to produce an ICE there, and in any case, it's not related to rust-lang#87496:
https://github.com/rust-lang/rust/blob/fd853c00e255559255885aadff9e93a1760c8728/compiler/rustc_lint/src/types.rs#L720-L734

r? `@JohnTitor`
@Manishearth Manishearth mentioned this pull request Sep 16, 2021
Closed
GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this pull request Sep 16, 2021
@GuillaumeGomez
Rollup merge of rust-lang#87529 - FabianWolff:issue-87496, r=nikomats…
dc7a66c 
…akis

Fix ICE in `improper_ctypes_definitions` lint with all-ZST transparent types

Fixes rust-lang#87496. There is also another function in the same file that looks fishy, but I haven't been able to produce an ICE there, and in any case, it's not related to rust-lang#87496:
https://github.com/rust-lang/rust/blob/fd853c00e255559255885aadff9e93a1760c8728/compiler/rustc_lint/src/types.rs#L720-L734

r? ``@JohnTitor``
@GuillaumeGomez GuillaumeGomez mentioned this pull request Sep 16, 2021
Closed
@JohnTitor JohnTitor mentioned this pull request Sep 17, 2021
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Sep 17, 2021
@bors
Auto merge of rust-lang#89037 - JohnTitor:rollup-rd9btbs, r=JohnTitor
1c03f0d 
Rollup of 10 pull requests

Successful merges:

 - rust-lang#86382 (Make diagnostics clearer for `?` operators)
 - rust-lang#87529 (Fix ICE in `improper_ctypes_definitions` lint with all-ZST transparent types)
 - rust-lang#88339 (Add TcpListener::into_incoming and IntoIncoming)
 - rust-lang#88735 (Don't lint about missing code examples in derived traits)
 - rust-lang#88751 (Couple of changes to FileSearch and SearchPath)
 - rust-lang#88883 (Move some tests to more reasonable directories - 7)
 - rust-lang#88887 (Const Deref)
 - rust-lang#88911 (Improve error message for type mismatch in generator arguments)
 - rust-lang#89014 (PassWrapper: handle separate Module*SanitizerPass)
 - rust-lang#89033 (Set the library path in sysroot-crates-are-unstable)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 81dc216 into rust-lang:master Sep 17, 2021
@rustbot rustbot added this to the 1.57.0 milestone Sep 17, 2021
@ChrisDenton ChrisDenton mentioned this pull request Jan 28, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@camelid camelid camelid left review comments

@hellow554 hellow554 hellow554 left review comments

@JohnTitor JohnTitor JohnTitor approved these changes

@nikomatsakis nikomatsakis nikomatsakis approved these changes

Assignees

@nikomatsakis nikomatsakis

Labels
A-FFI Area: Foreign function interface (FFI) S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
1.57.0
Development

Successfully merging this pull request may close these issues.

ICE: compiler/rustc_lint/src/types.rs:859:17: malformed transparent type
10 participants
@FabianWolff @JohnTitor @JohnCSimon @nikomatsakis @bors @hellow554 @camelid @rust-highfive @inquisitivecrystal @rustbot