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 TooGeneric variant to LayoutError and emit Unknown #135158

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

FedericoBruzzone
Copy link
Contributor

@FedericoBruzzone FedericoBruzzone commented Jan 6, 2025

What's in this PR?

  • Add TooGeneric variant to LayoutError and emit Unknown one
  • Return ErrorGuaranteed from items_of_instance query

With this PR these issues and their respective ICEs are resolved:

For instance, given this code:

#![feature(trivial_bounds)]

fn return_str()
where
    str: Sized,
{
    [(); { let _a: Option<str> = None; 0 }];
}

the following error is generated

error[E0080]: evaluation of constant value failed
 --> ice-135138.rs:7:16
  |
7 |     [(); { let _a: Option<str> = None; 0 }];
  |                ^^ cannot determine the layout of the type `Option<str>`

Then, according to @lukas-code: "Keeping this assertion in some sensible manner seems like it would be more effort than it's worth". So, I decided to remove it and the ICE related to #135138 has been fixed.

if !field.ty.is_sized(cx.tcx(), cx.typing_env) {
cx.tcx().dcx().delayed_bug(format!(
"encountered unexpected unsized field in layout of {ty:?}: {field:#?}"
));
}

@rustbot
Copy link
Collaborator

rustbot commented Jan 6, 2025

r? @cjgillot

rustbot has assigned @cjgillot.
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 S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. labels Jan 6, 2025
@rustbot
Copy link
Collaborator

rustbot commented Jan 6, 2025

HIR ty lowering was modified

cc @fmease

@FedericoBruzzone
Copy link
Contributor Author

Feel free to ask me anything.
In particular, let me know if you would like the addition of a test within the tests/ui/layout/ folder.

@rust-log-analyzer

This comment has been minimized.

@jieyouxu
Copy link
Member

jieyouxu commented Jan 6, 2025

(The fluent slug needs to be sorted lexicographically)

@jieyouxu
Copy link
Member

jieyouxu commented Jan 6, 2025

error[E0080]: evaluation of constant value failed
 --> ice-135138.rs:7:16
  |
7 |     [(); { let _a: Option<str> = None; 0 }];
  |                ^^ cannot determine the layout of the type `Option<str>`

seems somewhat unfortunate diagnostics-wise because:

  • The error is quite generic and isn't super helpful to the user (e.g. why can't the compiler determine the layout?), because it doesn't actually point to the "root" problem, which is Option<str>.
  • The problem here is that Option<str> doesn't have a size known at, er, const-eval time?

Introducing this UnexpectedUnsized variant to LayoutError and removing the assertion seems a bit strange to me, won't this paper over genuine implementation bugs, even if this variant is only constructed under trivial_bounds? I do appreciate that the layout calculation is not trivial, so, not a blocker.

Anyway, cc @lukas-code.

@rust-log-analyzer

This comment has been minimized.

Copy link
Member

@lukas-code lukas-code left a comment

Choose a reason for hiding this comment

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

In particular, let me know if you would like the addition of a test within the tests/ui/layout/ folder.

Yes, this definitely needs tests, for both of the issues. See here for a guide on adding tests.

compiler/rustc_ty_utils/src/layout.rs Outdated Show resolved Hide resolved
compiler/rustc_ty_utils/src/layout.rs Show resolved Hide resolved
compiler/rustc_middle/src/ty/layout.rs Outdated Show resolved Hide resolved
@rust-log-analyzer

This comment has been minimized.

@FedericoBruzzone FedericoBruzzone force-pushed the master branch 2 times, most recently from 3463d0f to 3d8a88a Compare January 6, 2025 23:58
@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@rustbot
Copy link
Collaborator

rustbot commented Jan 8, 2025

Some changes occurred to the CTFE machinery

cc @rust-lang/wg-const-eval

@rust-log-analyzer

This comment has been minimized.

@FedericoBruzzone FedericoBruzzone changed the title Fix two ICEs by introducing a new layout error variant for unexpected unsized Fix two ICEs by introducing a new layout error variant Jan 8, 2025
@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@FedericoBruzzone FedericoBruzzone force-pushed the master branch 5 times, most recently from 87473c4 to 0940c4e Compare January 14, 2025 15:20
@rust-log-analyzer

This comment has been minimized.

@FedericoBruzzone
Copy link
Contributor Author

If it isn't immediately required for this PR, I'd prefer to remove it or move it to another PR for now.

We should be there, let me know @lukas-code :D

@@ -317,13 +317,25 @@ fn layout_of_uncached<'tcx>(
if count.has_aliases() {
count = tcx.normalize_erasing_regions(cx.typing_env, count);
if count.has_aliases() {
return Err(error(cx, LayoutError::Unknown(ty)));
return Err(error(cx, LayoutError::TooGeneric(ty)));
Copy link
Member

Choose a reason for hiding this comment

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

I had missed this case before, but there can actually be unevaluated constants that are rigid but not generic. In this case the LayoutError::TooGeneric here is still wrong:

#![feature(trivial_bounds)]

trait Project {
    const ASSOC: usize;
}

fn foo()
where
    (): Project,
{
    [(); <() as Project>::ASSOC];
}

We already normalize above in fn layout_of so checking for aliases and normalizing here looks redundant anyway -- can we just remove it?

Copy link
Contributor Author

@FedericoBruzzone FedericoBruzzone Jan 15, 2025

Choose a reason for hiding this comment

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

Yes, we can just remove it.

This is currently an ICE:

error: internal compiler error: Missing value for constant, but no error reported?
  --> tests/ui/layout/uneval-const-rigid.rs:11:5
   |
11 |     [(); <() as Project>::ASSOC];
   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
   |
note: delayed at compiler/rustc_trait_selection/src/traits/const_evaluatable.rs:132:44 - disabled backtrace
  --> tests/ui/layout/uneval-const-rigid.rs:11:5
   |
11 |     [(); <() as Project>::ASSOC];
   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^

Can we fix it in another PR? I will open an issue if yes. @lukas-code

Copy link
Member

Choose a reason for hiding this comment

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

Ah yes, I see now that this is actually a different issue. So yes, feel free to open a new issue and you don't have to fix it here.

But then I'm also wondering whether the case where count is unevaluated but doesn't have params is actually reachable, because now I can't come up with an example for that.

Copy link
Contributor Author

@FedericoBruzzone FedericoBruzzone Jan 17, 2025

Choose a reason for hiding this comment

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

I will investigate as soon as possible. That case may be unreachable, but I am not sure. @lukas-code

compiler/rustc_ty_utils/src/layout.rs Outdated Show resolved Hide resolved
compiler/rustc_ty_utils/src/layout.rs Outdated Show resolved Hide resolved
compiler/rustc_ty_utils/src/layout.rs Outdated Show resolved Hide resolved
src/librustdoc/html/templates/type_layout.html Outdated Show resolved Hide resolved
src/librustdoc/html/templates/type_layout.html Outdated Show resolved Hide resolved
tests/ui/transmute/transmute-type-parameters.stderr Outdated Show resolved Hide resolved
tests/ui/polymorphic-const-issue-105249.rs Outdated Show resolved Hide resolved
compiler/rustc_middle/src/query/mod.rs Outdated Show resolved Hide resolved
@bors
Copy link
Contributor

bors commented Jan 15, 2025

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

@FedericoBruzzone FedericoBruzzone force-pushed the master branch 2 times, most recently from e860813 to 3605306 Compare January 15, 2025 18:20
@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@oli-obk
Copy link
Contributor

oli-obk commented Jan 17, 2025

@bors delegate=lukas-code

@bors
Copy link
Contributor

bors commented Jan 17, 2025

✌️ @lukas-code, you can now approve this pull request!

If @oli-obk told you to "r=me" after making some further change, please make that change, then do @bors r=@oli-obk

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

- `check-pass` test for a MRE of rust-lang#135020
- fail test for rust-lang#135138
- switch to `TooGeneric` for checking CMSE fn signatures
- switch to `TooGeneric` for compute `SizeSkeleton` (for transmute)
- fix broken tests
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue.
Projects
None yet
8 participants