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

Macros should be able to expand to methods #4621

Closed
catamorphism opened this Issue Jan 25, 2013 · 15 comments

Comments

Projects
None yet
8 participants
@catamorphism
Contributor

catamorphism commented Jan 25, 2013

Requested in a FIXME (formerly XXX) in rustc::middle::lang_items

More specifically, here is (an abstraction of) the code pattern that the FIXME is noting:

pub enum E { Enil, Ergt, Elft, Eall, /* ... */ Eend, }
pub struct S { items: [int, ..5] }

pub impl S {
    pub fn name(index:uint) -> &'static str {
        match index {
            0 => "nil", 1 => "lft", 2 => "rgt", 3 => "all",
            4 => "end", _ => "???"
        }
    }

    // The cases below are where a macro that expands into a method
    // definition is of interest, since they are all very regular.
    pub fn nil_item(&self) -> int { self.items[Enil as uint] }

    pub fn lft_item(&self) -> int { self.items[Elft as uint] }

    pub fn rgt_item(&self) -> int { self.items[Ergt as uint] }

    pub fn all_item(&self) -> int { self.items[Eall as uint] }

    pub fn end_item(&self) -> int { self.items[Eend as uint] }
}

    // Presumably the macro invocation version would look something like:
    //   macro_rules! e_method( 
    //      $method $variant
    //      =>
    //      pub fn $method(&self) -> int { self.items[$variant as uint] }
    //    )
    //
    // pub impl S {
    //   pub fn name(index:uint) -> &'static str { ... }
    //   e_method!(nil_item Enil);
    //   e_method!(lft_item Elft);
    //   e_method!(rgt_item Ergt);
    //   e_method!(all_item Eall);
    //   e_method!(end_item Eend);
    // }


fn main() {
    let s = S{ items: [0, 2, 1, 3, 4]};
    io::println(fmt!("Hello World: %?", s.all_item()));
}
@pnkfelix

This comment has been minimized.

Show comment
Hide comment
@pnkfelix

pnkfelix Apr 29, 2013

Member

�While I could not find a way to directly express the desired form, there is a way to get some abstraction here: Namely, instead of trying only to make a macro that expand into a method definition (which do not currently appear in a macro-expansion context, the first of a number of obstacles in the way of resolving the above), one can make a macro that expands into the whole impl itself.

My first attempt to do this tried to factor the impl into its non-repetitive (e.g. the name method in the example I added to the issue description) and repetitive components, and then feed both of those sequences of pseudo-items into a single macro that would expand into the final impl. However, this attempt failed, since I was not able to coerce the expander into accepting my factored name method definition as a proper component to be spliced into the final impl item.

So I gave up on that more general factoring, and instead came up with the below, which does seem to work. Its pattern might be applicable to the repetitive code from rustc::middle::lang_items, at the cost of making it a little bit more obtuse for a casual reader. (But anyone who is used to seeing similar abstractions in C macros probably will not mind it.) Here is the code:

macro_rules! e_class(
    ($S_name:ident,
     $( $method_name:ident $enum_name:ident ),*)
    =>
    {
        pub impl $S_name {
            pub fn name(index:uint) -> &'static str {
                match index {
                    0 => "nil", 1 => "lft", 2 => "rgt", 3 => "all",
                    4 => "end", _ => "???"
                }
            }

            $( pub fn $method_name (&self) -> int { self.items[$enum_name as uint] } )*
        }
    }
)

e_class!( S,
          nil_item Enil ,
          rgt_item Ergt ,
          lft_item Elft ,
          all_item Eall ,
          end_item Eend )
Member

pnkfelix commented Apr 29, 2013

�While I could not find a way to directly express the desired form, there is a way to get some abstraction here: Namely, instead of trying only to make a macro that expand into a method definition (which do not currently appear in a macro-expansion context, the first of a number of obstacles in the way of resolving the above), one can make a macro that expands into the whole impl itself.

My first attempt to do this tried to factor the impl into its non-repetitive (e.g. the name method in the example I added to the issue description) and repetitive components, and then feed both of those sequences of pseudo-items into a single macro that would expand into the final impl. However, this attempt failed, since I was not able to coerce the expander into accepting my factored name method definition as a proper component to be spliced into the final impl item.

So I gave up on that more general factoring, and instead came up with the below, which does seem to work. Its pattern might be applicable to the repetitive code from rustc::middle::lang_items, at the cost of making it a little bit more obtuse for a casual reader. (But anyone who is used to seeing similar abstractions in C macros probably will not mind it.) Here is the code:

macro_rules! e_class(
    ($S_name:ident,
     $( $method_name:ident $enum_name:ident ),*)
    =>
    {
        pub impl $S_name {
            pub fn name(index:uint) -> &'static str {
                match index {
                    0 => "nil", 1 => "lft", 2 => "rgt", 3 => "all",
                    4 => "end", _ => "???"
                }
            }

            $( pub fn $method_name (&self) -> int { self.items[$enum_name as uint] } )*
        }
    }
)

e_class!( S,
          nil_item Enil ,
          rgt_item Ergt ,
          lft_item Elft ,
          all_item Eall ,
          end_item Eend )
@pnkfelix

This comment has been minimized.

Show comment
Hide comment
@pnkfelix

pnkfelix Apr 29, 2013

Member

@PaulS @catamorphism There are a couple different issues that crop up here. E.g., I had some difficulty feeding in a method definition to be plugged in from the outside world; that might just be my own inexperience, I don't know.

But the more important overall question raised by this Issue is: Do we want the method definition points within impl's to be subject to macro expansion? I suspect we do (and thus am not closing this bug yet); but if we don't, then I assume something like the workaround I outlined in my previous comment would be suitable here, and we could close this bug (or whittle it down to the other narrower issues I alluded to already).

Member

pnkfelix commented Apr 29, 2013

@PaulS @catamorphism There are a couple different issues that crop up here. E.g., I had some difficulty feeding in a method definition to be plugged in from the outside world; that might just be my own inexperience, I don't know.

But the more important overall question raised by this Issue is: Do we want the method definition points within impl's to be subject to macro expansion? I suspect we do (and thus am not closing this bug yet); but if we don't, then I assume something like the workaround I outlined in my previous comment would be suitable here, and we could close this bug (or whittle it down to the other narrower issues I alluded to already).

@pnkfelix

This comment has been minimized.

Show comment
Hide comment
@pnkfelix

pnkfelix May 2, 2013

Member

Nominating for milestone Far Future.

Member

pnkfelix commented May 2, 2013

Nominating for milestone Far Future.

@graydon

This comment has been minimized.

Show comment
Hide comment
@graydon

graydon May 2, 2013

Contributor

accepted

Contributor

graydon commented May 2, 2013

accepted

@catamorphism

This comment has been minimized.

Show comment
Hide comment
@catamorphism

catamorphism Jul 8, 2013

Contributor

(bug triage) Revisited. Far-future still seems reasonable.

Contributor

catamorphism commented Jul 8, 2013

(bug triage) Revisited. Far-future still seems reasonable.

@huonw

This comment has been minimized.

Show comment
Hide comment
@huonw

huonw Nov 3, 2013

Member

Traige: no changes, although I've written a brief proposal at #7727 (comment) about how we could make macros more flexible.

Member

huonw commented Nov 3, 2013

Traige: no changes, although I've written a brief proposal at #7727 (comment) about how we could make macros more flexible.

@chris-morgan

This comment has been minimized.

Show comment
Hide comment
@chris-morgan

chris-morgan Dec 6, 2013

Member

With #10832, this is not as important as it was previously, as the lang_items.rs use case is no longer of interest.

Member

chris-morgan commented Dec 6, 2013

With #10832, this is not as important as it was previously, as the lang_items.rs use case is no longer of interest.

@pnkfelix

This comment has been minimized.

Show comment
Hide comment
@pnkfelix

pnkfelix Dec 11, 2013

Member

#10832 is basically adopting the work-around I proposed in my comment above.

So I'm in favor of closing this unless people can find arguments for making method definition points within impl's to be subject to macro expansion. (I'm ambivalent on the matter.)

Member

pnkfelix commented Dec 11, 2013

#10832 is basically adopting the work-around I proposed in my comment above.

So I'm in favor of closing this unless people can find arguments for making method definition points within impl's to be subject to macro expansion. (I'm ambivalent on the matter.)

@Skrylar

This comment has been minimized.

Show comment
Hide comment
@Skrylar

Skrylar Dec 25, 2013

I just ran in to this and there is at least one good use case: Library binding.

For example in SDL there are a lot of times where you simply want to bind Window.DoSomething to SDL_DoSomething(self.handle), and a macro allows you to define that function form once. Further methods which have the same common signature can just invoke the macro, which cuts down the amount of typing as well as functions with completely identical structures.

Maintaining by hand, you get a lot of functions like this:

impl Window {
  pub fn DoSomething(&self) {
    unsafe { ffi::SDL_DoSomething(self.handle); }
  }
}

Which is exactly the kind of thing you want a macro producing a function for: replacing large amounts of boilerplate which has the same structure in each invocation.

Skrylar commented Dec 25, 2013

I just ran in to this and there is at least one good use case: Library binding.

For example in SDL there are a lot of times where you simply want to bind Window.DoSomething to SDL_DoSomething(self.handle), and a macro allows you to define that function form once. Further methods which have the same common signature can just invoke the macro, which cuts down the amount of typing as well as functions with completely identical structures.

Maintaining by hand, you get a lot of functions like this:

impl Window {
  pub fn DoSomething(&self) {
    unsafe { ffi::SDL_DoSomething(self.handle); }
  }
}

Which is exactly the kind of thing you want a macro producing a function for: replacing large amounts of boilerplate which has the same structure in each invocation.

@sfackler

This comment has been minimized.

Show comment
Hide comment
@sfackler

sfackler Dec 25, 2013

Member

@Skrylar is there any reason that you can't structure the macro to generate the entire impl block? That works fine right now.

Member

sfackler commented Dec 25, 2013

@Skrylar is there any reason that you can't structure the macro to generate the entire impl block? That works fine right now.

@Skrylar

This comment has been minimized.

Show comment
Hide comment
@Skrylar

Skrylar Dec 25, 2013

@sfackler It just seems more idiomatic after reading the manual to be able to macro out the common case directly where you need it, instead of flipping around the entire block as a macro chalk full of "special case" conditions for that macro which simply insert raw functions in. Since you inevitably have that one function in every library that takes in a weird number of oddly typed parameters, you still have to key in functions that there's only one of with that type signature.

Skrylar commented Dec 25, 2013

@sfackler It just seems more idiomatic after reading the manual to be able to macro out the common case directly where you need it, instead of flipping around the entire block as a macro chalk full of "special case" conditions for that macro which simply insert raw functions in. Since you inevitably have that one function in every library that takes in a weird number of oddly typed parameters, you still have to key in functions that there's only one of with that type signature.

@sfackler

This comment has been minimized.

Show comment
Hide comment
@sfackler

sfackler Dec 26, 2013

Member

I'm still not sure what you're saying. What exactly is wrong with a setup like this?

macro_rules! build_window_ffi(
    ($name:ident, $raw:ident, $ret:ty, $($arg_name:ident: $arg_ty:ty => $raw_arg:expr),*) => (
        impl Window {
            pub fn $name(&self, $($arg_name: $arg_ty),+) -> $ret {
               unsafe { ffi::$raw(self.handle, $($raw_arg),+) }
            }
        }
    )
)

build_window_ffi(DoSomething, SDL_DoSomething, ())
build_window_ffi(DoSomethingElse, SDL_DoSomethingElse, c_int)
Member

sfackler commented Dec 26, 2013

I'm still not sure what you're saying. What exactly is wrong with a setup like this?

macro_rules! build_window_ffi(
    ($name:ident, $raw:ident, $ret:ty, $($arg_name:ident: $arg_ty:ty => $raw_arg:expr),*) => (
        impl Window {
            pub fn $name(&self, $($arg_name: $arg_ty),+) -> $ret {
               unsafe { ffi::$raw(self.handle, $($raw_arg),+) }
            }
        }
    )
)

build_window_ffi(DoSomething, SDL_DoSomething, ())
build_window_ffi(DoSomethingElse, SDL_DoSomethingElse, c_int)
@Skrylar

This comment has been minimized.

Show comment
Hide comment
@Skrylar

Skrylar Dec 26, 2013

@sfackler It wasn't clear that multiple impl blocks with identical signatures would mix without issue, so I thought you were suggesting generating the entire implementation block for the type in one function.

If having multiple implementation blocks is acceptable behavior, I recommend the reference manual mention this. I just re-checked and this is not mentioned under 6.1.9

Skrylar commented Dec 26, 2013

@sfackler It wasn't clear that multiple impl blocks with identical signatures would mix without issue, so I thought you were suggesting generating the entire implementation block for the type in one function.

If having multiple implementation blocks is acceptable behavior, I recommend the reference manual mention this. I just re-checked and this is not mentioned under 6.1.9

@huonw

This comment has been minimized.

Show comment
Hide comment
@huonw

huonw Dec 27, 2013

Member

@Skrylar fwiw, generating the entire impl block in one macro isn't necessarily crazy, e.g. the compiler does it in this macro, creating this impl with some generated methods. (Of course, you may have a design that's not amenable to this set-up, in which case, ignore me.)

Member

huonw commented Dec 27, 2013

@Skrylar fwiw, generating the entire impl block in one macro isn't necessarily crazy, e.g. the compiler does it in this macro, creating this impl with some generated methods. (Of course, you may have a design that's not amenable to this set-up, in which case, ignore me.)

bors added a commit that referenced this issue Mar 11, 2014

auto merge of #12780 : zslayton/rust/json-nav, r=alexcrichton
This is my first non-docs contribution to Rust, so please let me know what I can fix. I probably should've submitted this to the mailing list first for comments, but it didn't take too long to implement so I figured I'd just give it a shot.

These changes are modeled loosely on the [JsonNode API](http://jackson.codehaus.org/1.7.9/javadoc/org/codehaus/jackson/JsonNode.html) provided by the [Jackson JSON processor](http://jackson.codehaus.org/).

Many common use cases for parsing JSON involve pulling one or more fields out of an object, however deeply nested. At present, this requires writing a pyramid of match statements. The added methods in this PR aim to make this a more painless process.

Example JSON:
```json
{
    "successful" : true,
    "status" : 200,
    "error" : null,
    "content" : {
        "vehicles" : [
            {"make" : "Toyota", "model" : "Camry", "year" : 1997},
            {"make" : "Honda", "model" : "Accord", "year" : 2003}
        ]
    }
}
```

Accessing "successful":
```rust
 let example_json : Json = from_str("...above json...").unwrap();
 let was_successful: Option<bool> = example_json.get_boolean(&~"successful");
```

Accessing "status":
```rust
 let example_json : Json = from_str("...above json...").unwrap();
 let status_code : Option<f64> = example_json.get_number(&~"status");
```

Accessing "vehicles":
```rust
 let example_json : Json = from_str("...above json...").unwrap();
 let vehicle_list: Option<&[Json]> = example_json.find_list(&~"vehicles");
```

Accessing "vehicles" with an explicit path:
```rust
 let example_json : Json = from_str("...above json...").unwrap();
 let vehicle_list: Option<&[Json]> = example_json.get_path_list(&[&~"content", &~"vehicles"]);
```

Accessing "error", which might be null or a string:
```rust
 let example_json : Json = from_str("...above json...").unwrap();
 let error: Option<Json> = example_json.get(&~"error");
 if error.is_null() { // This would be nicer as a match, I'm just illustrating the boolean test methods
    println!("Error is null, everything's fine.");
 } else if error.is_str(){
    println!("Something went wrong: {}", error.as_string().unwrap());
}
```

Some notes:
* Macros would help to eliminate some of the repetitiveness of the implementation, but I couldn't use them due to #4621.
* Would it be better to name methods after the Json enum type (e.g. `get_string`) or the associated Rust built-in? (e.g. `get_str`)
* TreeMap requires its keys to be &~str. Because of this, all of the new methods required &~str for their parameters. I'm uncertain what the best approach to fixing this is: neither demanding an owned pointer nor allocating within the methods to appease TreeMap's find() seems desirable. If I were able to take &str, people could put together paths easily with `"foo.bar.baz".split('.').collect();`
* At the moment, the `find_<sometype>` methods all find the first match for the provided key and attempt to return that value if it's of the specified type. This makes sense to me, but it's possible that users would interpret a call to `find_boolean("successful")` as looking for the first "successful" item that was a boolean rather than looking for the first "successful" and returning None if it isn't boolean.

I hope this is helpful. Any feedback is appreciated!

bors added a commit that referenced this issue Mar 11, 2014

auto merge of #12780 : zslayton/rust/json-nav, r=alexcrichton
This is my first non-docs contribution to Rust, so please let me know what I can fix. I probably should've submitted this to the mailing list first for comments, but it didn't take too long to implement so I figured I'd just give it a shot.

These changes are modeled loosely on the [JsonNode API](http://jackson.codehaus.org/1.7.9/javadoc/org/codehaus/jackson/JsonNode.html) provided by the [Jackson JSON processor](http://jackson.codehaus.org/).

Many common use cases for parsing JSON involve pulling one or more fields out of an object, however deeply nested. At present, this requires writing a pyramid of match statements. The added methods in this PR aim to make this a more painless process.

**Edited to reflect final implementation**

Example JSON:
```json
{
    "successful" : true,
    "status" : 200,
    "error" : null,
    "content" : {
        "vehicles" : [
            {"make" : "Toyota", "model" : "Camry", "year" : 1997},
            {"make" : "Honda", "model" : "Accord", "year" : 2003}
        ]
    }
}
```

Accessing "successful":
```rust
 let example_json : Json = from_str("...above json...").unwrap();
 let was_successful: Option<bool> = example_json.find(&~"successful").and_then(|j| j.as_boolean());
```

Accessing "status":
```rust
 let example_json : Json = from_str("...above json...").unwrap();
 let status_code : Option<f64> = example_json.find(&~"status").and_then(|j| j.as_number());
```

Accessing "vehicles":
```rust
 let example_json : Json = from_str("...above json...").unwrap();
 let vehicle_list: Option<List> = example_json.search(&~"vehicles").and_then(|j| j.as_list());
```

Accessing "vehicles" with an explicit path:
```rust
 let example_json : Json = from_str("...above json...").unwrap();
 let vehicle_list: Option<List> = example_json.find_path(&[&~"content", &~"vehicles"]).and_then(|j| j.as_list());
```

Accessing "error", which might be null or a string:
```rust
 let example_json : Json = from_str("...above json...").unwrap();
 let error: Option<Json> = example_json.find(&~"error");
 if error.is_null() { // This would be nicer as a match, I'm just illustrating the boolean test methods
    println!("Error is null, everything's fine.");
 } else if error.is_str(){
    println!("Something went wrong: {}", error.as_string().unwrap());
}
```

Some notes:
* Macros would help to eliminate some of the repetitiveness of the implementation, but I couldn't use them due to #4621. (**Edit**: There is no longer repetitive impl. Methods were simplified to make them more composable.)
* Would it be better to name methods after the Json enum type (e.g. `get_string`) or the associated Rust built-in? (e.g. `get_str`)
* TreeMap requires its keys to be &~str. Because of this, all of the new methods required &~str for their parameters. I'm uncertain what the best approach to fixing this is: neither demanding an owned pointer nor allocating within the methods to appease TreeMap's find() seems desirable. If I were able to take &str, people could put together paths easily with `"foo.bar.baz".split('.').collect();` (**Edit**: Follow on investigation into making TreeMap able to search by Equiv would be worthwhile.)
* At the moment, the `find_<sometype>` methods all find the first match for the provided key and attempt to return that value if it's of the specified type. This makes sense to me, but it's possible that users would interpret a call to `find_boolean("successful")` as looking for the first "successful" item that was a boolean rather than looking for the first "successful" and returning None if it isn't boolean. (**Edit**: No longer relevant.)

I hope this is helpful. Any feedback is appreciated!

jbclements added a commit to jbclements/rust that referenced this issue Jul 13, 2014

jbclements added a commit to jbclements/rust that referenced this issue Jul 13, 2014

@bors bors closed this in aee5917 Jul 13, 2014

@hugoduncan

This comment has been minimized.

Show comment
Hide comment
@hugoduncan

hugoduncan Feb 11, 2015

As far as I can tell, this doesn't work for pub fn in impls.

macro_rules! defn {
    // A match without return type
    ($n:ident) => (
            // Adding pub here results in: 
            //   error: expected one of `extern`, `fn`, or `unsafe`, found `pub`
            /* pub */ fn $n (&self) {
              println!("hi");
        })
}

pub struct Struct;

impl Struct {
  // Adding pub here doesn't give an error, but the
  // function ends up not being public.
  pub defn!(f);
}

hugoduncan commented Feb 11, 2015

As far as I can tell, this doesn't work for pub fn in impls.

macro_rules! defn {
    // A match without return type
    ($n:ident) => (
            // Adding pub here results in: 
            //   error: expected one of `extern`, `fn`, or `unsafe`, found `pub`
            /* pub */ fn $n (&self) {
              println!("hi");
        })
}

pub struct Struct;

impl Struct {
  // Adding pub here doesn't give an error, but the
  // function ends up not being public.
  pub defn!(f);
}
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment