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

Constants and statics are not required to be Sized. #34390

Closed
eddyb opened this issue Jun 20, 2016 · 10 comments · Fixed by #34443
Closed

Constants and statics are not required to be Sized. #34390

eddyb opened this issue Jun 20, 2016 · 10 comments · Fixed by #34443
Assignees
Labels
I-ICE Issue: The compiler panicked, giving an Internal Compilation Error (ICE) ❄️ T-lang Relevant to the language team, which will review and decide on the PR/issue.

Comments

@eddyb
Copy link
Member

eddyb commented Jun 20, 2016

This looks unintentional, and has been broken in all cases, until a few releases ago, AFAICT.
Should this be covered by WF rules? cc @rust-lang/lang @ubsan @solson

Unsized constants cause an ICE, in both old trans and MIR trans:

// rustc_trans/consts.rs:445: const expr(35: &S) of type &str has size 8 instead of 16
// rustc_trans/mir/constant.rs:175: loading from `str`
//     ((i8*:i8* getelementptr inbounds ([3 x i8], [3 x i8]* @str7144, i32 0, i32 0)))
//     in constant
const S: str = *"foo";
fn main() { println!("{}", &S); }

Unsized statics appear to work, but only in old trans:

use std::fmt::Debug;
static FOO: Debug+Sync = *(&1 as &(Debug+Sync));
static S: str = *"foo";
fn main() { println!("{:?} {}", &FOO, &S); }

I believe this is possible because a constant DST lvalue in old trans is merely the fat pointer.
It seems useful to allow an unsized static, but IMO it should require a sized RHS instead, i.e.:

static A: [i32] = [1, 2, 3];
fn main() { let x = A[0]; ... }
// as sugar for:
static A_SIZED: [i32; 3] = [1, 2, 3];
static A: &'static [i32] = &A_SIZED;
fn main() { let x = (*A)[0]; ... }

And, of course, this should all go through the RFC process, to figure out all the finer details.

@eddyb eddyb added I-ICE Issue: The compiler panicked, giving an Internal Compilation Error (ICE) ❄️ I-nominated T-lang Relevant to the language team, which will review and decide on the PR/issue. labels Jun 20, 2016
@Ericson2314
Copy link
Contributor

Ericson2314 commented Jun 21, 2016

Should it be

// as sugar for:
const A: &'static [i32] = &[1, 2, 3];
fn main() { let x = (*A)[0]; ... }

? I don't think A needs to be an lvalue.

@eddyb
Copy link
Member Author

eddyb commented Jun 21, 2016

It makes more sense for constants, but I gave the example with static because that's the only one that works today (although by accident).

@Ericson2314
Copy link
Contributor

Hmm? I mean the static is desugared into a const. const A: &'static [i32] = &[1, 2, 3]; works today.

@Ericson2314
Copy link
Contributor

The idea being that a static and a const reference both give access to one lvalue.

@eddyb
Copy link
Member Author

eddyb commented Jun 21, 2016

@Ericson2314 No, static has different semantics: it allows an UnsafeCell somewhere within and is guaranteed not to be duplicated.

@Ericson2314
Copy link
Contributor

Ericson2314 commented Jun 21, 2016

Hmm?

#![feature(const_fn)]

use std::fmt::Debug;
use std::cell::UnsafeCell;

#[derive(Debug)]
struct Doop(UnsafeCell<u32>);

unsafe impl Send for Doop {} 
unsafe impl Sync for Doop {} 


//const  ASDF: &'static (Debug + Sync) = &Doop(UnsafeCell::new(1)); 
  static ASDF: &'static (Debug + Sync) = &Doop(UnsafeCell::new(1)); 

fn main() {
    println!("{:#?}", ASDF);
}

Both of these gives me a "error: cannot borrow a constant which contains interior mutability, create a static instead". This is what I'd suspect as the lvalue from the promoted Doop(..) has nothing to do with ASDF being a static or a const.

@eddyb
Copy link
Member Author

eddyb commented Jun 21, 2016

@Ericson2314 Let me rewrite my desugaring to account for that, oops.

@Ericson2314
Copy link
Contributor

#![feature(const_fn)]

use std::fmt::Debug;
use std::cell::UnsafeCell;

#[derive(Debug)]
struct Doop(UnsafeCell<u32>);

unsafe impl Send for Doop {} 
unsafe impl Sync for Doop {} 

static ASDF_INNER: Doop = Doop(UnsafeCell::new(1));
//static ASDF: &'static (Debug + Sync) = &ASDF_INNER; 
  const ASDF: &'static (Debug + Sync) = &ASDF_INNER; 

fn main() {
    println!("{:#?}", ASDF);
}

Oh and my confusion really just stemmed from forgetting about the constants referring to statics rule. [Kind of a funny rule when const S: &'static T = &... works, but this is an orthogonal issue.]

@Aatch
Copy link
Contributor

Aatch commented Jun 21, 2016

I think in the meantime unsized statics should probably error, perhaps dependent on a crater run, though I doubt there's many people relying on said bug. It's much easier to relax restrictions than tighten them.

@eddyb eddyb self-assigned this Jun 23, 2016
@eddyb eddyb removed the I-nominated label Jun 23, 2016
@eddyb
Copy link
Member Author

eddyb commented Jun 23, 2016

Will prepare a patch to turn this into an error, hopefully nobody has been abusing it.

bors added a commit that referenced this issue Jul 2, 2016
Disallow constants and statics from having unsized types.

This is a `[breaking-change]` which fixes #34390 by banning unsized `const` and `static`, e.g.:
```rust
const A: [i32] = *(&[0, 1, 2] as &[i32]);
static B: str = *"foo";
```

This was not intentionally allowed, and other than for `static` since some versions ago, it ICE'd.
If you've been taking advantage of this with `static`, you should be able to just use references instead.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
I-ICE Issue: The compiler panicked, giving an Internal Compilation Error (ICE) ❄️ T-lang Relevant to the language team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants