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

vec![] should support the [value, ..repeat] syntax #15587

Closed
Kimundi opened this issue Jul 10, 2014 · 16 comments · Fixed by #20771
Closed

vec![] should support the [value, ..repeat] syntax #15587

Kimundi opened this issue Jul 10, 2014 · 16 comments · Fixed by #20771

Comments

@Kimundi
Copy link
Member

Kimundi commented Jul 10, 2014

Rust now supports [] in macro invocations, with a driving use case being able to write vec![0, 0, 0] as analogy to the fixed sized vector syntax [0, 0, 0].

However, there is no analogy to the [0, ..n] repeat syntax for it: vec![0, ..n] does not work.

For it to be added, vec! would either need to become a procedural macro, or something like rust-lang/rfcs#88 needs to be implemented.

@lilyball
Copy link
Contributor

I don't think rust-lang/rfcs#88 is relevant here; that's relevant to vec![1,2,3] because it provides a good way to initialize the Vec with the correct capacity for the number of arguments. But with your vec![0, ..n] there's no use for counting arguments.

The [0, ..n] array syntax requires the element to be Copy. Does your proposed vec![0, ..n] syntax have the same requirement? Or does it require Clone instead? Offhand it looks like it's just shorthand for Vec::from_elem(), which uses the Clone bound. I'm also not convinced that we need any shorthand for Vec::from_elem(). It's already reasonably short. vec![] itself is useful because there's no one-line equivalent that can be written as a function (because it needs to move each value, so it can't take a slice, and we don't have integral generics, so it can't take a fixed-size array).

FWIW, extending vec![] to support vec![0, ..n] syntax does not actually require a procedural macro, as long as it's being translated into Vec::from_elem(). It just needs a new matcher added to the top of the macro (not the bottom, that doesn't work), making the whole macro look like

macro_rules! vec{
    ($e:expr, ..$n:expr) => { ::std::vec::Vec::from_elem($n, $e) };
    ($($e:expr),*) => {{
        let mut _temp = ::std::vec::Vec::new();
        $(_temp.push($e);)*
        _temp
    }};
    ($($e:expr),+,) => (vec!($($e),+));
}

@Kimundi
Copy link
Member Author

Kimundi commented Jul 10, 2014

Oh right, I totally got the dependency on rust-lang/rfcs#88 backwards.

I still think it would make sense to mirror the syntax of [foo, ..bar], it could just dispatch to from_elem() as you said.

This should either involve a Clone bound and an arbitrary uint value on the premise that Vec can only be constructed at runtime, or a Copy bound and a requirement for the uint to be a constexpr, to be semantically and syntactically identical to [expr, ..constexr]

Whether or not to extend vec! at all depends on how idiomatic its use is considered. Personally I always use vec![] instead of Vec::new(), for example.

@lilyball
Copy link
Contributor

Personally, I think supporting vec![expr, ..n] is a reasonable idea. And I think it should be literally equivalent to Vec::from_elem(n, expr), i.e. no constexpr, and Clone instead of Copy. IIRC we want to eventually be able to provide an implementation of Clone for all types that are Copy (if they don't otherwise provide it), but in the meantime we tend to assume that any type that needs some sort of copying like that needs to implement Clone. There are many good reasons why array construction uses Copy instead of Clone, but those reasons don't apply to Vec.

Given that, I would support a PR that modifies the definition of vec![] as I listed above.

@huonw
Copy link
Member

huonw commented Jul 10, 2014

What about expanding to

($init: expr, .. $n: expr) => {{
    let n = $n;
    let v = Vec::with_capacity(n);
    for _ in range(0, n) { v.push($init) }
    v
}}

i.e. don't require Clone or Copy.

@lilyball
Copy link
Contributor

@huonw That's inconsistent with array literals. I submitted a PR a while back as #14272 (closed due to inactivity) that added an ary![expr, ..n] procedural macro that did have that behavior (of duplicating the expression instead of copying the resulting value), although it was not clear if this was behavior that anyone besides me actually wanted. But that was very explicitly trying to diverge from array initialization semantics; the request here for vec![] would be to try to match the semantics as reasonably as possible except using a Vec instead of an array.

Note that with your version the following code:

let x = vec![{println!("wat"); 3u}, ..5];

would print "wat" 5 times, but the equivalent array expression only prints "wat" once.

@lilyball
Copy link
Contributor

FWIW, if you want that expression-duplicating behavior with Vec, we already have Vec::from_fn() that will do it (using a closure).

@Kimundi
Copy link
Member Author

Kimundi commented Jul 11, 2014

Yeah, if we want to emulate the [expr, ..expr] syntax, then it should only evaluate the first expression once, or else you get weird and very rare bugs.

@japaric
Copy link
Member

japaric commented Oct 11, 2014

After #17920 introduced into_vec and changed the definition of vec!, the desired syntax is pretty close to just work.

Today, you'd expect vec![0u, ..16] to expand to:

{
    use std::slice::BoxedSlice;
    let xs: ::std::boxed::Box<[_]> = box() [0u, ..16];
    xs.into_vec()
}

Except that it doesn't because the macro definition expects $($e:expr),*, (expressions separated by commas) as arguments, but ..16 is not an expression (because .. is parsed as a token).

To make this work, we'll have to change vec! from being a MBE to a "procedural macro", where it retains pretty much the same expansion it has today, but it doesn't parse the arguments as a list, instead it just passes the full token tree to box () [$TOKEN_TREE]. (I'm skimming the details of this last part, but I think is doable)

Anyhow, such change probably merits an RFC?

@thestinger
Copy link
Contributor

@japaric: I don't think it needs an RFC. The syntax is defined as part of the core language for fixed-size arrays and vec![] should just be passing through to those.

@japaric
Copy link
Member

japaric commented Oct 11, 2014

I correct myself, this can be done using macros by example. This appears to work:

#![feature(macro_rules)]

macro_rules! my_vec {
    ($value:expr, ..$repeat:expr) => ({
        use std::slice::BoxedSlice;
        let xs: ::std::boxed::Box<[_]> = box() [$value, ..$repeat];
        xs.into_vec()
    });
    ($($x:expr),*) => ({
        use std::slice::BoxedSlice;
        let xs: ::std::boxed::Box<[_]> = box() [$($x),*];
        xs.into_vec()
    });
}

fn main() {
    println!("{}", my_vec![0u, ..16]);
    println!("{}", my_vec![0u, 1]);
}

Prints:

[0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0]
[0, 1]

(I would send a PR, but I'm on my way out)

@thestinger
Copy link
Contributor

It would be nicer if it could just pass through all of the tokens opaquely. I don't know how to do that or if it's actually possible.

@japaric
Copy link
Member

japaric commented Oct 11, 2014

It would be nicer if it could just pass through all of the tokens opaquely.

Is there any advantage of doing it that way? I think the macro definition above covers all the initialization patterns we currently have. (modulo trailing comma, but that's easy to add)

I don't know how to do that or if it's actually possible.

I'll be working with syntax extensions later today, I'll check whether this is feasible or not, and report it here.

@japaric
Copy link
Member

japaric commented Oct 12, 2014

It's certainly doable, here's an out of tree implementation. I'll reproduce the important part of the README here for posteriority:


tor!($SOMETHING) expands into:

{
    use std::slice::BoxedSlice;
    use std::boxed::HEAP;
    let xs = box (HEAP) [$SOMETHING];
    xs.into_vec()
}

Both tor![0u, 1] and tor![0u, ..4] work as expected, and things like tor![0u, ..] and tor![0u 1] fail to parse, and the error message points to the right span. The implementation is way longer (100 lines) than the current macro by example though.

Do let me know if I should send a PR with the syntax extension version or with the macro by example version.

@huonw
Copy link
Member

huonw commented Oct 12, 2014

It seems to me that this can be done via the following (the expr! hack is a work around for #9364).

#![feature(macro_rules)]

macro_rules! expr {
    ($e: expr) => { $e }
}
macro_rules! vec {
    ($($contents: tt)*) => {
       {
            use std::slice::BoxedSlice;
            use std::boxed::HEAP;
            let xs = expr!(box (HEAP) [$($contents)*]);
            xs.into_vec()
        }
    }
}

fn main() {
    println!("{}", vec![1u, 2, 3]);
    println!("{}", vec![1u, .. 10]);
}

@japaric
Copy link
Member

japaric commented Oct 12, 2014

If it can be done with just macro_rules!, we should use that.

@icorderi
Copy link
Contributor

+1 to the syntax addition

let x = vec![0u8,..1024*1024];

Kimundi added a commit to Kimundi/rust that referenced this issue Jan 8, 2015
bors added a commit to rust-lang-ci/rust that referenced this issue Nov 13, 2023
Fix autoimport does nothing when importing trait that is as _ imports

Potentially fixes rust-lang#15128

There are two cases of imports:
1. With simple path
2. With use tree list (or say complex path).

On deeper inspection, the [`recursive_merge`](https://github.com/rust-lang/rust-analyzer/blob/994df3d6a31d39f11600f30a6df0b744b13937c1/crates/ide-db/src/imports/merge_imports.rs#L87) function (called by [`try_merge_trees_mut`)](https://github.com/rust-lang/rust-analyzer/blob/994df3d6a31d39f11600f30a6df0b744b13937c1/crates/ide-db/src/imports/merge_imports.rs#L69) is meaningful only in the case of complex path (i.e when the UseTree contains a UseTreeList).

The [`recursive_merge`](https://github.com/rust-lang/rust-analyzer/blob/994df3d6a31d39f11600f30a6df0b744b13937c1/crates/ide-db/src/imports/merge_imports.rs#L87) function has [match with `Ok` arm](https://github.com/rust-lang/rust-analyzer/blob/994df3d6a31d39f11600f30a6df0b744b13937c1/crates/ide-db/src/imports/merge_imports.rs#L106), that is only executed when both LHS and RHS has `PathSegment` with same `NameRef`. The removal of underscore is implemented in this arm in the case of complex path.

For simple paths, the underscore is removed by checking if both LHS and RHS are simple paths and if their `Path` is same (the check is done [here](https://github.com/rust-lang/rust-analyzer/blob/994df3d6a31d39f11600f30a6df0b744b13937c1/crates/ide-db/src/imports/merge_imports.rs#L74)) and remove the underscore if one is found (I made an assumption here that RHS will always be what rust-analyzer suggests to import, because at this point I'm not sure how to remove underscore with help of `ted::replace`).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants