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

lint: use transparent_newtype_field to avoid ICE #74340

Conversation

davidtwco
Copy link
Member

Fixes #73747.

This PR re-uses the transparent_newtype_field function instead of manually calling is_zst on normalized fields to determine which field in a transparent type is the non-zero-sized field, thus avoiding an ICE.

This commit re-uses the `transparent_newtype_field` function instead of
manually calling `is_zst` on normalized fields to determine which field
in a transparent type is the non-zero-sized field, thus avoiding an ICE.

Signed-off-by: David Wood <[email protected]>
@rust-highfive
Copy link
Collaborator

r? @ecstatic-morse

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

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 14, 2020
@pnkfelix
Copy link
Member

@bors r+ rollup=true

@bors
Copy link
Contributor

bors commented Jul 14, 2020

📌 Commit cccc310 has been approved by pnkfelix

@bors
Copy link
Contributor

bors commented Jul 14, 2020

🌲 The tree is currently closed for pull requests below priority 5, 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 Jul 14, 2020
bors added a commit to rust-lang-ci/rust that referenced this pull request Jul 14, 2020
…arth

Rollup of 11 pull requests

Successful merges:

 - rust-lang#73759 (Add missing Stdin and StdinLock examples)
 - rust-lang#74211 (Structured suggestion when not using struct pattern)
 - rust-lang#74228 (Provide structured suggestion on unsized fields and fn params)
 - rust-lang#74252 (Don't allow `DESTDIR` to influence LLVM builds)
 - rust-lang#74263 (Slight reorganization of sys/(fast_)thread_local)
 - rust-lang#74271 (process_unix: prefer i32::*_be_bytes over manually shifting bytes)
 - rust-lang#74272 (pprust: support multiline comments within lines)
 - rust-lang#74332 (Update cargo)
 - rust-lang#74334 (bootstrap: Improve wording on docs for `verbose-tests`)
 - rust-lang#74336 (typeck: use `item_name` in cross-crate packed diag)
 - rust-lang#74340 (lint: use `transparent_newtype_field` to avoid ICE)

Failed merges:

r? @ghost
@LeSeulArtichaut
Copy link
Contributor

Just in case #74342 fails, should we bump the priority as a fix to a P-critical bug?

self.cx.param_env,
field.ty(self.cx.tcx, substs),
);
if field_ty.is_zst(self.cx.tcx, field.did) {
Copy link
Member

Choose a reason for hiding this comment

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

What does is_zst even do? I'm surprised to see that method on Ty, and suspicious of how it might be implemented.

Copy link
Member Author

Choose a reason for hiding this comment

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

I added it in an earlier PR (or I just made it with existing logic, I forget), it’s predominantly used in lints which care about transparent types. Its logic is the same as where transparent types are checked in typeck (but it wasn’t trivial to factor it out there too) - it basically gets the layout of the type and calls is_zst on that, returning false if any of the intermediate stuff fails.

Copy link
Member

Choose a reason for hiding this comment

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

I don't think Ty should have methods that interact with layout, feels like an abstraction violation.
If something relies on layout that should be clear (i.e. is_zst callers should use layout_of themselves - not to mention that e.g. LateLintContext should have better layout_of integration than doing it on Ty)

@bors bors merged commit dbe7ed3 into rust-lang:master Jul 14, 2020
@davidtwco davidtwco deleted the issue-73747-improper-ctypes-defns-is-zst-with-params branch July 15, 2020 06:14
@pnkfelix pnkfelix added the beta-nominated Nominated for backporting to the compiler in the beta channel. label Jul 16, 2020
@pnkfelix
Copy link
Member

@pnkfelix pnkfelix added the beta-accepted Accepted for backporting to the compiler in the beta channel. label Jul 16, 2020
@Mark-Simulacrum Mark-Simulacrum removed the beta-nominated Nominated for backporting to the compiler in the beta channel. label Aug 7, 2020
bors added a commit to rust-lang-ci/rust that referenced this pull request Aug 7, 2020
…ulacrum

[beta] backports

* Forbid non-derefable types explicitly in unsizing casts rust-lang#75136
* forbid `#[track_caller]` on main rust-lang#75130
* Fix #[track_caller] shims for trait objects. rust-lang#74784
* rustc_target: Add a target spec option for disabling `--eh-frame-hdr` rust-lang#74631
* Disable Azure Pipelines except for macOS rust-lang#74620
* Upload builds from GHA instead of Azure Pipelines rust-lang#74565
* Add the aarch64-apple-darwin target rust-lang#74541
* Use `ReEmpty(U0)` as the implicit region bound in typeck rust-lang#74509
* rustbuild: drop tool::should_install rust-lang#74457
* lint: use `transparent_newtype_field` to avoid ICE rust-lang#74340
* Don't panic if the lhs of a div by zero is not statically known rust-lang#74221
* improper_ctypes_definitions: allow `Box` rust-lang#74448
* typeck: check for infer before type impls trait rust-lang#73965
RalfJung added a commit to RalfJung/rust that referenced this pull request Sep 19, 2020
…ion, r=eddyb

lint/ty: move fns to avoid abstraction violation

This PR moves `transparent_newtype_field` and `is_zst` to `LateContext` where they are used, rather than being on the `VariantDef` and `TyS` types, hopefully addressing @eddyb's concern [from this comment](rust-lang#74340 (comment)).
RalfJung added a commit to RalfJung/rust that referenced this pull request Sep 19, 2020
…ion, r=eddyb

lint/ty: move fns to avoid abstraction violation

This PR moves `transparent_newtype_field` and `is_zst` to `LateContext` where they are used, rather than being on the `VariantDef` and `TyS` types, hopefully addressing @eddyb's concern [from this comment](rust-lang#74340 (comment)).
@cuviper cuviper added this to the 1.47.0 milestone May 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
beta-accepted Accepted for backporting to the compiler in the beta channel. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Nightly ICEs trying to normalize during a cast
9 participants