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

Usage of errorneous constant can be omitted on nightly and beta #67083

Closed
AnthonyMikh opened this issue Dec 6, 2019 · 9 comments
Closed

Usage of errorneous constant can be omitted on nightly and beta #67083

AnthonyMikh opened this issue Dec 6, 2019 · 9 comments
Assignees
Labels
A-associated-items Area: Associated items (types, constants & functions) A-const-eval Area: Constant evaluation (MIR interpretation) C-bug Category: This is a bug. P-high High priority regression-from-stable-to-beta Performance or correctness regression from stable to beta. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@AnthonyMikh
Copy link
Contributor

Consider the following code:

trait ZeroSized: Sized {
    #[deny(const_err)]
    const I_AM_ZERO_SIZED: ();
    fn requires_zero_size(self);
}

impl<T: Sized> ZeroSized for T {
    const I_AM_ZERO_SIZED: ()  = [()][std::mem::size_of::<Self>()];
    fn requires_zero_size(self) {
        #![deny(const_err)]
        let () = Self::I_AM_ZERO_SIZED;
        println!("requires_zero_size called");
    }
}

fn main() {
    ().requires_zero_size();
    42_u32.requires_zero_size();
}

The intent of code is to prevent requires_zero_size from being called on types with non-zero size (duh). This goal is achieved on stable: trying to compile this code with stable compiler (1.39.0) gives the following error:

error: any use of this value will cause an error
 --> src/main.rs:8:34
  |
8 |     const I_AM_ZERO_SIZED: ()  = [()][std::mem::size_of::<Self>()];
  |     -----------------------------^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^-
  |                                  |
  |                                  index out of bounds: the len is 1 but the index is 4
  |
  = note: `#[deny(const_err)]` on by default

I expected the same with some newer unstable versiosns. However, this code compiles just fine on latest beta (2019-12-02 1463fedbddfc0ee087ec) and latest nightly (2019-12-05 710a362dc7634fce4288) at the moment.

Compiling in debug vs release gives no difference in behaviour.

@jonas-schievink jonas-schievink added A-associated-items Area: Associated items (types, constants & functions) A-const-eval Area: Constant evaluation (MIR interpretation) C-bug Category: This is a bug. regression-from-stable-to-beta Performance or correctness regression from stable to beta. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. I-nominated T-lang Relevant to the language team, which will review and decide on the PR/issue. labels Dec 6, 2019
@Centril
Copy link
Contributor

Centril commented Dec 6, 2019

cc @wesleywiser @oli-obk

@oli-obk
Copy link
Contributor

oli-obk commented Dec 7, 2019

Oh oh. Investigating...

@oli-obk
Copy link
Contributor

oli-obk commented Dec 7, 2019

Wild guess: const prop based on miri does not actually read from zst constants... and confirmed by making the constant's type i32 instead of ()

@AnthonyMikh
Copy link
Contributor Author

AnthonyMikh commented Dec 7, 2019

Interesting: just changing type to i32 is not sufficient, you should actually bind the value to name. This one fails to compile:

trait ZeroSized: Sized {
    #[deny(const_err)]
    const I_AM_ZERO_SIZED: i32;
    fn requires_zero_size(self);
}

impl<T: Sized> ZeroSized for T {
    const I_AM_ZERO_SIZED: i32  = [0][std::mem::size_of::<Self>()];
    fn requires_zero_size(self) {
        #![deny(const_err)]
        let _unused = Self::I_AM_ZERO_SIZED;
        println!("requires_zero_size called");
    }
}

fn main() {
    ().requires_zero_size();
    42_u32.requires_zero_size();
}

Making a slight change:

11 -        let _unused = Self::I_AM_ZERO_SIZED;
11 +        let _ = Self::I_AM_ZERO_SIZED;

...And this compiles again on beta and nightly.

@oli-obk oli-obk self-assigned this Dec 7, 2019
@oli-obk
Copy link
Contributor

oli-obk commented Dec 7, 2019

It's not const prop. Const prop doesn't know about this yet. If you allow all the lints on stable, you get a segfault in the running program. I think monomorphization needs to hard error on invalid constants. Trying out now

@RalfJung
Copy link
Member

RalfJung commented Dec 9, 2019

The intent of code is to prevent requires_zero_size from being called on types with non-zero size (duh). This goal is achieved on stable

Notice that seems quite fragile though, as this is only prevented by a deny-by-default lint. With --cap-lints (as it is used for dependencies) or when someone uses your crate with #[allow(const_err)], this code will compile on stable already.

@oli-obk
Copy link
Contributor

oli-obk commented Dec 9, 2019

Usage of a broken constant is not just a lint, it is a hard error in monomorphic code. What you are thinking about is promotion, where we never report a hard error. Using a constant whose evaluation would fail is a hard error, it's just that declaring said constant and not using it will just cause warnings.

JohnTitor added a commit to JohnTitor/rust that referenced this issue Dec 10, 2019
Ensure that we get a hard error on generic ZST constants if their bod…

…y causes an error during evaluation

cc rust-lang#67083 (does not fix because we still need the beta backport)

r? @wesleywiser

cc @RalfJung
@pnkfelix
Copy link
Member

pnkfelix commented Dec 12, 2019

triage: P-high, removing nomination

@pnkfelix pnkfelix added P-high High priority and removed I-nominated labels Dec 12, 2019
@Centril Centril removed the T-lang Relevant to the language team, which will review and decide on the PR/issue. label Dec 19, 2019
@pnkfelix
Copy link
Member

fixed by beta backport PR #67297 which was backport of PR #67134

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-associated-items Area: Associated items (types, constants & functions) A-const-eval Area: Constant evaluation (MIR interpretation) C-bug Category: This is a bug. P-high High priority regression-from-stable-to-beta Performance or correctness regression from stable to beta. 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

6 participants