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

unsized_fn_params should not accept types that don't have a dynamically fixed size (such as extern types) #115709

Open
RalfJung opened this issue Sep 9, 2023 · 3 comments
Labels
F-extern_types `#![feature(extern_types)]` F-unsized_fn_params `#![feature(unsized_fn_params)]` T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@RalfJung
Copy link
Member

RalfJung commented Sep 9, 2023

The following code should not be accepted:

#![feature(extern_types)]
#![feature(unsized_fn_params)]

extern {
    type E;
}

fn test(e: E) {}

fn calltest(e: Box<E>) {
    test(*e)
}

In terms of MIR semantics I think we want to view a function call as making copies of its arguments. This requires at least dynamically determining the size of the argument, and therefore we shouldn't accept extern type arguments.

I was extremely surprised earlier today when I learned that this code is getting accepted.

There is no generic bound to distinguish extern types from other unsized types, so the options we have are

  • require the unsized type to be concrete enough that we can determine its tail, and ensure it is not extern
  • raise the error during monomorphization / codegen

Trying to actually build any such code will lead to ICEs such as

error: internal compiler error: compiler/rustc_codegen_llvm/src/type_of.rs:306:13: TyAndLayout::scalar_pair_element_llty(TyAndLayout { ty: *mut E, layout: Layout { size: Size(8 bytes), align: AbiAndPrefAlign { abi: Align(8 bytes), pref: Align(8 bytes) }, abi: Scalar(Initialized { value: Pointer(AddressSpace(0)), valid_range: 0..=18446744073709551615 }), fields: Primitive, largest_niche: None, variants: Single { index: 0 }, max_repr_align: None, unadjusted_abi_align: Align(8 bytes) } }): not applicable

or

thread 'rustc' panicked at compiler/rustc_codegen_llvm/src/builder.rs:491:9:
assertion `left == right` failed
  left: false
 right: true
@RalfJung RalfJung added the F-unsized_fn_params `#![feature(unsized_fn_params)]` label Sep 9, 2023
@rustbot rustbot added the needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. label Sep 9, 2023
@RalfJung RalfJung added the F-extern_types `#![feature(extern_types)]` label Sep 9, 2023
@SNCPlay42
Copy link
Contributor

SNCPlay42 commented Sep 9, 2023

See also #79409.

There is no generic bound to distinguish extern types from other unsized types

rust-lang/rfcs#2984 and rust-lang/rfcs#3396 are open RFCs that attempt to address this.

@RalfJung
Copy link
Member Author

RalfJung commented Sep 9, 2023

Unfortunately just rejecting locals/arguments that are not known to be dyn-sized as a MIR pass does not work: the forget_unsized function in the standard library would stop compiling.

So currently something post-monomorphization is the only option. Is there even some place that check could be put without being duplicated across all codegen backends (and Miri)?

@RalfJung RalfJung changed the title unsized_fn_params should not accept types that don't have a dynamically fixed size unsized_fn_params should not accept types that don't have a dynamically fixed size (such as extern types) Sep 10, 2023
bors added a commit to rust-lang-ci/rust that referenced this issue Sep 11, 2023
move required_consts check to general post-mono-check function

I was hoping this would simplify the code. That didn't entirely happen, but I still think it could be useful to have a single place for the "required const" check -- and this does simplify some code in codegen, where the const-eval functions no longer have to return a `Result`.

Also this is in preparation for another post-mono check that will be needed for (the current hackfix for) rust-lang#115709: ensuring that all locals are dynamically sized.

I didn't expect this to change diagnostics, but the changes seem either harmless (for the cycle errors) or improvements (where more spans are shown when something goes wrong in a constant).

r? `@oli-obk`
bors added a commit to rust-lang-ci/rust that referenced this issue Sep 11, 2023
move required_consts check to general post-mono-check function

I was hoping this would simplify the code. That didn't entirely happen, but I still think it could be useful to have a single place for the "required const" check -- and this does simplify some code in codegen, where the const-eval functions no longer have to return a `Result`.

Also this is in preparation for another post-mono check that will be needed for (the current hackfix for) rust-lang#115709: ensuring that all locals are dynamically sized.

I didn't expect this to change diagnostics, but the changes seem either harmless (for the cycle errors) or improvements (where more spans are shown when something goes wrong in a constant).

r? `@oli-obk`
bors added a commit to rust-lang-ci/rust that referenced this issue Sep 14, 2023
move required_consts check to general post-mono-check function

I was hoping this would simplify the code. That didn't entirely happen, but I still think it could be useful to have a single place for the "required const" check -- and this does simplify some code in codegen, where the const-eval functions no longer have to return a `Result`.

Also this is in preparation for another post-mono check that will be needed for (the current hackfix for) rust-lang#115709: ensuring that all locals are dynamically sized.

I didn't expect this to change diagnostics, but the changes seem either harmless (for the cycle errors) or improvements (where more spans are shown when something goes wrong in a constant).

r? `@oli-obk`
bors added a commit to rust-lang-ci/rust that referenced this issue Sep 18, 2023
move required_consts check to general post-mono-check function

This factors some code that is common between the interpreter and the codegen backends into shared helper functions. Also as a side-effect the interpreter now uses the same `eval` functions as everyone else to get the evaluated MIR constants.

Also this is in preparation for another post-mono check that will be needed for (the current hackfix for) rust-lang#115709: ensuring that all locals are dynamically sized.

I didn't expect this to change diagnostics, but it's just cycle errors that change.

r? `@oli-obk`
bors added a commit to rust-lang-ci/rust that referenced this issue Sep 18, 2023
move required_consts check to general post-mono-check function

This factors some code that is common between the interpreter and the codegen backends into shared helper functions. Also as a side-effect the interpreter now uses the same `eval` functions as everyone else to get the evaluated MIR constants.

Also this is in preparation for another post-mono check that will be needed for (the current hackfix for) rust-lang#115709: ensuring that all locals are dynamically sized.

I didn't expect this to change diagnostics, but it's just cycle errors that change.

r? `@oli-obk`
github-actions bot pushed a commit to rust-lang/miri that referenced this issue Sep 19, 2023
move required_consts check to general post-mono-check function

This factors some code that is common between the interpreter and the codegen backends into shared helper functions. Also as a side-effect the interpreter now uses the same `eval` functions as everyone else to get the evaluated MIR constants.

Also this is in preparation for another post-mono check that will be needed for (the current hackfix for) rust-lang/rust#115709: ensuring that all locals are dynamically sized.

I didn't expect this to change diagnostics, but it's just cycle errors that change.

r? `@oli-obk`
@Noratrieb Noratrieb added T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. and removed needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. labels Sep 19, 2023
bjorn3 pushed a commit to rust-lang/rustc_codegen_cranelift that referenced this issue Sep 19, 2023
move required_consts check to general post-mono-check function

This factors some code that is common between the interpreter and the codegen backends into shared helper functions. Also as a side-effect the interpreter now uses the same `eval` functions as everyone else to get the evaluated MIR constants.

Also this is in preparation for another post-mono check that will be needed for (the current hackfix for) rust-lang/rust#115709: ensuring that all locals are dynamically sized.

I didn't expect this to change diagnostics, but it's just cycle errors that change.

r? `@oli-obk`
bors added a commit to rust-lang-ci/rust that referenced this issue Sep 24, 2023
detect dyn-unsized arguments before they cause ICEs

"Fixes" rust-lang#115709 in a crude way, with a post-mono check. This is entirely caused by us not having a trait to express "type can have its size determined at runtime".
bors added a commit to rust-lang-ci/rust that referenced this issue Sep 24, 2023
detect dyn-unsized arguments before they cause ICEs

"Fixes" rust-lang#115709 in a crude way, with a post-mono check. This is entirely caused by us not having a trait to express "type can have its size determined at runtime".
flip1995 pushed a commit to flip1995/rust-clippy that referenced this issue Sep 25, 2023
move required_consts check to general post-mono-check function

This factors some code that is common between the interpreter and the codegen backends into shared helper functions. Also as a side-effect the interpreter now uses the same `eval` functions as everyone else to get the evaluated MIR constants.

Also this is in preparation for another post-mono check that will be needed for (the current hackfix for) rust-lang/rust#115709: ensuring that all locals are dynamically sized.

I didn't expect this to change diagnostics, but it's just cycle errors that change.

r? `@oli-obk`
@RalfJung
Copy link
Member Author

RalfJung commented Dec 2, 2023

In #116115 we agreed that this is not worth a post-mono check; this needs proper design to find a way that makes this work without a post-mono check. So for now we are living with the ICEs.

TaKO8Ki added a commit to TaKO8Ki/rust that referenced this issue Dec 4, 2023
…iler-errors

more targeted errors when extern types end up in places they should not

Cc rust-lang#115709 -- this does not fix that bug but it makes the panics less obscure and makes it more clear that this is a deeper issue than just a little codegen oversight. (In rust-lang#116115 we decided we'd stick to causing ICEs here for now, rather than nicer errors. We can't currently show any errors pre-mono and probably we don't want post-mono checks when this gets stabilized anyway.)
rust-timer added a commit to rust-lang-ci/rust that referenced this issue Dec 4, 2023
Rollup merge of rust-lang#118551 - RalfJung:extern-types-bugs, r=compiler-errors

more targeted errors when extern types end up in places they should not

Cc rust-lang#115709 -- this does not fix that bug but it makes the panics less obscure and makes it more clear that this is a deeper issue than just a little codegen oversight. (In rust-lang#116115 we decided we'd stick to causing ICEs here for now, rather than nicer errors. We can't currently show any errors pre-mono and probably we don't want post-mono checks when this gets stabilized anyway.)
github-actions bot pushed a commit to rust-lang/miri that referenced this issue Dec 5, 2023
more targeted errors when extern types end up in places they should not

Cc rust-lang/rust#115709 -- this does not fix that bug but it makes the panics less obscure and makes it more clear that this is a deeper issue than just a little codegen oversight. (In rust-lang/rust#116115 we decided we'd stick to causing ICEs here for now, rather than nicer errors. We can't currently show any errors pre-mono and probably we don't want post-mono checks when this gets stabilized anyway.)
lnicola pushed a commit to lnicola/rust-analyzer that referenced this issue Apr 7, 2024
move required_consts check to general post-mono-check function

This factors some code that is common between the interpreter and the codegen backends into shared helper functions. Also as a side-effect the interpreter now uses the same `eval` functions as everyone else to get the evaluated MIR constants.

Also this is in preparation for another post-mono check that will be needed for (the current hackfix for) rust-lang/rust#115709: ensuring that all locals are dynamically sized.

I didn't expect this to change diagnostics, but it's just cycle errors that change.

r? `@oli-obk`
RalfJung pushed a commit to RalfJung/rust-analyzer that referenced this issue Apr 27, 2024
move required_consts check to general post-mono-check function

This factors some code that is common between the interpreter and the codegen backends into shared helper functions. Also as a side-effect the interpreter now uses the same `eval` functions as everyone else to get the evaluated MIR constants.

Also this is in preparation for another post-mono check that will be needed for (the current hackfix for) rust-lang/rust#115709: ensuring that all locals are dynamically sized.

I didn't expect this to change diagnostics, but it's just cycle errors that change.

r? `@oli-obk`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
F-extern_types `#![feature(extern_types)]` F-unsized_fn_params `#![feature(unsized_fn_params)]` T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

4 participants