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

[const generics] symbol foo is already defined #70408

Closed
oli-obk opened this issue Mar 25, 2020 · 7 comments · Fixed by #99004
Closed

[const generics] symbol foo is already defined #70408

oli-obk opened this issue Mar 25, 2020 · 7 comments · Fixed by #99004
Assignees
Labels
A-const-generics Area: const generics (parameters and arguments) C-bug Category: This is a bug. E-needs-test Call for participation: An issue has been fixed and does not reproduce, but no test has been added. F-adt_const_params `#![feature(adt_const_params)]` requires-nightly This issue requires a nightly compiler in some way. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@oli-obk
Copy link
Contributor

oli-obk commented Mar 25, 2020

Const generics causes monomorphization to compare constants by identity instead of structurally. This means that two different references to equal data will compare as unequal.

The root problem is in

if !visited.lock_mut().insert(starting_point.clone()) {
which puts the Instance of a function into a FxHashSet. Since the Instance's substs contain constants, hashing these constants will cause structurally equal constants to have different hashes.
For HashStable this has been solved by hashing the actual data and not things like pointers. For comparison we have TypeRelation which does a more appropriate version of comparison.

I tried this code:

#![feature(const_generics)]

pub fn function_with_bytes<const BYTES: &'static [u8; 4]>() -> &'static [u8] {
    BYTES
}

pub fn main() {
    assert_eq!(function_with_bytes::<b"AAAA">(), &[0x41, 0x41, 0x41, 0x41]);
    assert_eq!(function_with_bytes::<{&[0x41, 0x41, 0x41, 0x41]}>(), b"AAAA");
}

I expected to see this happen: successful compilation just as if the type were &'static [u8]

Instead, this happened:

error: symbol `_ZN10playground19function_with_bytes17h85b0fd4613beba85E` is already defined
 --> src/main.rs:3:1
  |
3 | / pub fn function_with_bytes<const BYTES: &'static [u8; 4]>() -> &'static [u8] {
4 | |     BYTES
5 | | }
  | |_^

cc @varkor

cc @rust-lang/wg-const-eval It's a footgun that ConstKind and especially ConstValue can be compared for equality. Technically a ConstKind::Unevaluated is value/structurally equal to any other ConstKind. We may need that Eq impl for query return value hashing though, not sure.

@oli-obk oli-obk added C-bug Category: This is a bug. A-const-generics Area: const generics (parameters and arguments) labels Mar 25, 2020
@jonas-schievink jonas-schievink added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Mar 25, 2020
@oli-obk
Copy link
Contributor Author

oli-obk commented Mar 25, 2020

There's two ways that I see that we can use to go forward:

  1. remove Eq, PartialEq, Ord, PartialOrd, Hash from ConstValue and everything that depends on it. This requires some wrapper helpers (forwarding Eq to TypeRelation) around the return values of queries as query return types need Eq impls I think.
  2. manually implement Eq, PartialEq, Ord, PartialOrd, Hash to use TypeRelation and HashStable. This may cause some things to become slow, as more work is done. We could benchmark this as a first try.

@jonas-schievink jonas-schievink added F-const_generics `#![feature(const_generics)]` requires-nightly This issue requires a nightly compiler in some way. labels Mar 25, 2020
@eddyb
Copy link
Member

eddyb commented Mar 25, 2020

TypeRelation isn't relevant here, you should have fully normalized constants by the time of the monomorphization collector (i.e. no CosntKind::Unevaluated), anything else is a bug.

Keep in mind types have the same problem with ty::Projection and ty::Opaque.

@oli-obk
Copy link
Contributor Author

oli-obk commented Mar 25, 2020

Ok, but even ignoring any ConstKind other than ConstKind::Value, the only place that correctly compares ConstKind::Value is TypeRelation, so we can pull that out as a common function for comparing ConstKind::Value. That'll still have to have access to a TyCtxt in order to be able to fetch the Allocation that a const pointer points to.

@eddyb
Copy link
Member

eddyb commented Mar 25, 2020

You shouldn't need TypeRelation to compare fully normalized monomorphic things, interning should deduplicate everything by-typesystem-semantic-value.

@eddyb
Copy link
Member

eddyb commented Mar 26, 2020

Oh, as a side mention, if you need to validate that the typesystem doesn't contain any non-deduplicatable constants (e.g. &STATIC references), we can do it in WF checking, post-#70107.

@varkor
Copy link
Member

varkor commented Sep 13, 2020

I don't think we can run into issues with min_const_generics here, so not marking it as blocking.

@lcnr lcnr added E-needs-test Call for participation: An issue has been fixed and does not reproduce, but no test has been added. F-adt_const_params `#![feature(adt_const_params)]` and removed F-const_generics `#![feature(const_generics)]` labels Jun 28, 2022
@lcnr
Copy link
Contributor

lcnr commented Jun 28, 2022

this is now fixed by valtrees:

needs test though:

#![feature(adt_const_params)]

pub fn function_with_bytes<const BYTES: &'static [u8; 4]>() -> &'static [u8] {
    BYTES
}

pub fn main() {
    assert_eq!(function_with_bytes::<b"AAAA">(), &[0x41, 0x41, 0x41, 0x41]);
    assert_eq!(function_with_bytes::<{&[0x41, 0x41, 0x41, 0x41]}>(), b"AAAA");
}

@TaKO8Ki TaKO8Ki self-assigned this Jul 7, 2022
TaKO8Ki added a commit to TaKO8Ki/rust that referenced this issue Jul 7, 2022
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Jul 7, 2022
bors added a commit to rust-lang-ci/rust that referenced this issue Jul 7, 2022
…askrgr

Rollup of 9 pull requests

Successful merges:

 - rust-lang#97917 (Implement ExitCodeExt for Windows)
 - rust-lang#98844 (Reword comments and rename HIR visiting methods.)
 - rust-lang#98979 (interpret: use AllocRange in UninitByteAccess)
 - rust-lang#98986 (Fix missing word in comment)
 - rust-lang#98994 (replace process exit with more detailed exit in src/bootstrap/*.rs)
 - rust-lang#98995 (Add a test for rust-lang#80471)
 - rust-lang#99002 (suggest adding a derive for #[default] applied to variants)
 - rust-lang#99004 (Add a test for rust-lang#70408)
 - rust-lang#99017 (Replace boolean argument for print_where_clause with an enum to make code more clear)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors closed this as completed in 16ad2f5 Jul 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-const-generics Area: const generics (parameters and arguments) C-bug Category: This is a bug. E-needs-test Call for participation: An issue has been fixed and does not reproduce, but no test has been added. F-adt_const_params `#![feature(adt_const_params)]` requires-nightly This issue requires a nightly compiler in some way. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants