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

No way to reference primitives by absolute path #44865

Closed
sgrif opened this issue Sep 26, 2017 · 11 comments · Fixed by #67637
Closed

No way to reference primitives by absolute path #44865

sgrif opened this issue Sep 26, 2017 · 11 comments · Fixed by #67637
Labels
C-feature-accepted Category: A feature request that has been accepted pending implementation. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.

Comments

@sgrif
Copy link
Contributor

sgrif commented Sep 26, 2017

I got a bug report on Diesel from someone who has a database column called bool. This lead to us attempting to generate the following code:

pub struct bool;

impl QueryId for bool {
    type QueryId = Self;
    const HAS_STATIC_QUERY_ID: bool = true;
}

which causes a type mismatch because their struct is shadowing the primitive. I'd like to change the generated code to be const HAS_STATIC_QUERY_ID: ::std::bool, which is the path that the rustdoc for libstd indicates it should be, but the type doesn't exist there, or anywhere else I can find. I would assume the same issue exists for str, char, and all the numeric types.

@kennytm
Copy link
Member

kennytm commented Sep 26, 2017

Since the std::char module already exists, the type cannot be called std::char. But it can be aliased as std::primitives::char I guess.

// in libcore or libstd:

#[allow(non_camel_case_types)]
pub mod primitives {
    mod details {
        pub type Bool = bool;
        pub type Char = char;
        // ...
    }
    pub type bool = details::Bool;
    pub type char = details::Char;
    // ...
}

@sgrif
Copy link
Contributor Author

sgrif commented Sep 26, 2017

std::prelude::char would make sense as well

@kennytm
Copy link
Member

kennytm commented Sep 26, 2017

Related: #32131

The reason these primitives are not put into prelude was because

Although, it's not entirely convenient to put them into prelude right now due to temporary conflicts like use prelude::v1::*; use usize; in libcore/libstd, I'd better wait for proper glob shadowing before doing it.

but I think this is no longer a problem? (cc @petrochenkov)

@TimNN TimNN added the C-feature-request Category: A feature request, i.e: not implemented / a PR. label Sep 27, 2017
@petrochenkov petrochenkov self-assigned this Sep 27, 2017
@petrochenkov petrochenkov added the T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. label Sep 30, 2017
@petrochenkov
Copy link
Contributor

petrochenkov commented Sep 30, 2017

Tagging this as a library issue.

The solution (if it happens on our side) is to add a module core::primitives or core::builtins (and reexport it from std as well) as described in #44865 (comment).

In the meantime Diesel can privately define such a module itself and use it when unintended shadowing can happen.

It doesn't make much sense to put primitive types into the std prelude because they are already in a prelude actually, it's just not the std prelude, but an additional language prelude one level below (as it was decided in #32131 and #32274).

@petrochenkov petrochenkov removed their assignment Sep 30, 2017
@dtolnay dtolnay added C-feature-accepted Category: A feature request that has been accepted pending implementation. and removed C-feature-request Category: A feature request, i.e: not implemented / a PR. labels Nov 14, 2017
@dtolnay
Copy link
Member

dtolnay commented Nov 14, 2017

I agree that primitives should be exposed at some absolute path. std::primitive sounds reasonable.

@sgrif
Copy link
Contributor Author

sgrif commented Jan 17, 2018

@dtolnay I'd be happy to work on this. What's needed besides the implementation? I'm not entirely sure what a test for this should look like. Just a run-pass test that shadows bool and references std::primitive::bool? Do we need one for every type?

@dtolnay
Copy link
Member

dtolnay commented Jan 17, 2018

I would start with a PR that has:

  • Implementation,
  • Module-level documentation for std::primitive with a helpful example that communicates why you may want to use this,
  • One run-pass test for a shadowed bool as you suggested.

@sgrif
Copy link
Contributor Author

sgrif commented Jan 17, 2018

Sounds good. I'll try to get that PR done this week.

@sgrif
Copy link
Contributor Author

sgrif commented Jan 17, 2018

Does this need a feature gate?

@dtolnay
Copy link
Member

dtolnay commented Jan 17, 2018

Yes I believe so.

@steveklabnik
Copy link
Member

Triage: not aware of anything happening here.

Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this issue Feb 25, 2020
…olnay

Add primitive module to libcore

This re-exports the primitive types from libcore at `core::primitive` to allow
macro authors to have a reliable location to use them from.

Fixes rust-lang#44865
@bors bors closed this as completed in 0860f5a Feb 26, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-feature-accepted Category: A feature request that has been accepted pending implementation. T-libs-api Relevant to the library API 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