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

RFC: Attributes in formal function parameter position #2565

Open
wants to merge 8 commits into
base: master
from

Conversation

Projects
None yet
@Robbepop
Copy link

Robbepop commented Oct 15, 2018

This RFC proposes to add attributes in formal function parameter position.

Rendered

@Centril Centril added the T-lang label Oct 15, 2018

@Robbepop Robbepop changed the title RFC: attributes in formal function parameter position RFC: Attributes in formal function parameter position Oct 15, 2018

@SimonSapin

This comment has been minimized.

Copy link
Contributor

SimonSapin commented Oct 15, 2018

I think that the underscore prefix convention is well established and that #[unused] is not needed. The rest of this proposal still applies to procedural macros, though I don’t have a strong opinion on that.

@petrochenkov

This comment has been minimized.

Copy link
Contributor

petrochenkov commented Oct 15, 2018

Some questions to answer:

  • Whether macro attributes are supported.
  • Whether cfg is supported.
  • Whether attributes in fn types are supported (type F = fn(#[attr] param: Type); and type F = fn(#[attr] Type);).
  • Future compatibility with attributes on patterns #[attr] Pat and types #[attr] Type.
@Centril

This comment has been minimized.

Copy link
Contributor

Centril commented Oct 15, 2018

Thank you for and congratulations on writing your first RFC!
I and others have left some thoughts for you to address :)

@Centril

This comment has been minimized.

Copy link
Contributor

Centril commented Oct 15, 2018

A use case for this with proptest could be to enable the following:

#[proptest]
fn prop_my_property(#[types(T = u8 | u16)] elem: Vec<T>, ...) {
    ...
)

This would replace T with u8 and u16...

Also:

#[proptest]
fn prop_my_property(#[strat = 0..100] elem: usize, ...) {
    ...
)

Tho I'd probably formulate this instead as:

#[proptest]
#[proptest::types(T = u8 | u16)] 
fn prop_my_property<T>(elem: Vec<T>, ...) {
    ...
)

#[proptest]
fn prop_my_property(elem in 0..100usize, ...) { // this unfortunately won't parse
    ...
)
@Robbepop

This comment has been minimized.

Copy link
Author

Robbepop commented Oct 15, 2018

I think that the underscore prefix convention is well established and that #[unused] is not needed. The rest of this proposal still applies to procedural macros, though I don’t have a strong opinion on that.

You might be right about that. It was just an idea I had. We do not need to have language defined attributes for now.

@Robbepop

This comment has been minimized.

Copy link
Author

Robbepop commented Oct 15, 2018

Some questions to answer:

* Whether macro attributes are supported.

* Whether `cfg` is supported.

* Whether attributes in fn types are supported (`type F = fn(#[attr] param: Type);` and `type F = fn(#[attr] Type);`).

* Future compatibility with attributes on patterns `#[attr] Pat` and types `#[attr] Type`.

Good points!

I haven't concretely thought about macro attributes but I think that they should be supported for convenience if that doesn't mean too much extra work. I cannot see the end of the scope for this to be honest.

I have no clear answer for the cfg support. What do you think?

I would postpone the attributes in fn types for another RFC since at least to my understanding this would force having attributes be part of the type system.

What do you mean by the future compatibility with attributes on patterns and types? So what shouldn't be compatible with them in the future? I am sorry that I do not understand.

@Robbepop

This comment has been minimized.

Copy link
Author

Robbepop commented Oct 15, 2018

A use case for this with proptest could be to enable the following:

#[proptest]
fn prop_my_property(#[types(T = u8 | u16)] elem: Vec<T>, ...) {
    ...
)

This would replace T with u8 and u16...

Also:

#[proptest]
fn prop_my_property(#[strat = 0..100] elem: usize, ...) {
    ...
)

Tho I'd probably formulate this instead as:

#[proptest]
#[proptest::types(T = u8 | u16)] 
fn prop_my_property<T>(elem: Vec<T>, ...) {
    ...
)

#[proptest]
fn prop_my_property(elem in 0..100usize, ...) { // this unfortunately won't parse
    ...
)

Thank you for those interesting ideas around the use case.
I will implement them in the RFC in the motivation section.

Robbepop added some commits Oct 15, 2018

@petrochenkov

This comment has been minimized.

Copy link
Contributor

petrochenkov commented Oct 15, 2018

@Robbepop

I haven't concretely thought about macro attributes but I think that they should be supported for convenience if that doesn't mean too much extra work.

Macro attributes are supported only in a relatively small subset of attribute positions, so it's better to start with not supporting them.

I have no clear answer for the cfg support. What do you think?

cfg is generally supported in all attribute positions (IIRC it's under-implemented for generic parameters though), so it's better to support it.

I would postpone the attributes in fn types for another RFC since at least to my understanding this would force having attributes be part of the type system.

It wouldn't have any effect on type system, no more than attributes on function definitions.
Also, function types basically share the grammar with trait methods (at least on 2015 edition), so it may be more convenient to support attributes in them, at least for named parameters fn(#[attr] param: Type) (see below).

@petrochenkov

This comment has been minimized.

Copy link
Contributor

petrochenkov commented Oct 15, 2018

What do you mean by the future compatibility with attributes on patterns and types?

Right now we support attributes on arbitrary expressions, so it's quite possible that we'll eventually support attributes on arbitrary types and patterns as well.

In this case what the attribute is attached to in

fn f(#[attr] PAT: TYPE) {}

, to the pattern, or to the whole parameter?

I think we'll be able to reattach the attributes from parameters to patterns if pattern attributes become available, but I'm not entirely sure (macros?).

In this sense fn f(#[attr] &self) looks pretty suspicious, since &self is not a pattern, but I guess we'll be able to treat this as a sugar and reattach the attribute in the desugared form #[attr] self: &Self.

That's why fn f(#[attr] TYPE) (attribute on anonymous parameter) looks suspicious as well, so I suggest to not support this, so we don't have to think about attributes on types in addition to all the other issues.

@nielsle

This comment has been minimized.

Copy link

nielsle commented Oct 16, 2018

I think that I would prefer the following

#[ignore_args]
fn foo( #[ignore] bar: bool);

over the following

fn foo( #[ignore] bar: bool);

The first version is longer, but the scope of the macro is more clear. But then again this distinction is probably out of scope for the RFC.

@kennytm

This comment has been minimized.

Copy link
Member

kennytm commented Oct 16, 2018

In this case what the attribute is attached to in

fn f(#[attr] PAT: TYPE) {}

, to the pattern, or to the whole parameter?

Just wanna note that this thing already exists:

fn main() {
    match 3 {
        #[cfg(windows)] 
        3 => println!("a"),
        #[cfg(unix)] 
        3 => println!("b"),
        _ => println!("c"),
    }
}

and the attribute is applied to the whole arm, not the pattern.

@joshtriplett

This comment has been minimized.

Copy link
Member

joshtriplett commented Oct 17, 2018

I'm in favor of allowing attributes on parameters.

Please do drop the bits about replacing the _ convention with #[unused], though.

@Centril

This comment has been minimized.

Copy link
Contributor

Centril commented Oct 17, 2018

Please do drop the bits about replacing the _ convention with #[unused], though.

I believe it's not part of the proposal; rather, it is a hypothetical use case that some person could do in a procedural macro.

@Robbepop

This comment has been minimized.

Copy link
Author

Robbepop commented Oct 17, 2018

In this case what the attribute is attached to in

fn f(#[attr] PAT: TYPE) {}

, to the pattern, or to the whole parameter?

Just wanna note that this thing already exists:

fn main() {
    match 3 {
        #[cfg(windows)] 
        3 => println!("a"),
        #[cfg(unix)] 
        3 => println!("b"),
        _ => println!("c"),
    }
}

and the attribute is applied to the whole arm, not the pattern.

So should we do the same for the parameter attributes so that the attribute is for the PAT and the TYPE?

@Robbepop

This comment has been minimized.

Copy link
Author

Robbepop commented Oct 17, 2018

Please do drop the bits about replacing the _ convention with #[unused], though.

I believe it's not part of the proposal; rather, it is a hypothetical use case that some person could do in a procedural macro.

I should reformulate this to cause no misunderstandings.

@kennytm

This comment has been minimized.

Copy link
Member

kennytm commented Oct 18, 2018

@Robbepop Attributes typically aren't transitive. The attribute is applied to the parameter itself, not the pattern nor the type.

@Centril Centril added the I-nominated label Oct 18, 2018

@pnkfelix pnkfelix self-assigned this Oct 25, 2018

@Centril Centril removed the I-nominated label Oct 25, 2018

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

nikomatsakis commented Oct 25, 2018

I'm in favor of this general idea — I would like to see a few more examples. For example, using #[cfg] to make a parameter conditional. I would also like to see that it's clearly specified whether #[foo] a: B applies to the parameter as a whole (i.e., the pattern a and the type B) or just the pattern. I presume the former.

@Robbepop

This comment has been minimized.

Copy link
Author

Robbepop commented Oct 25, 2018

I will do some rewording of this RFC on the weekend when I have time for it.

I'm in favor of this general idea — I would like to see a few more examples. For example, using #[cfg] to make a parameter conditional. I would also like to see that it's clearly specified whether #[foo] a: B applies to the parameter as a whole (i.e., the pattern a and the type B) or just the pattern. I presume the former.

To my understanding the attribute should apply to both as you have guessed.

I am in favor of adding more examples - just need to be more creative on my side ...

@sfackler

This comment has been minimized.

Copy link
Member

sfackler commented Oct 25, 2018

One use case I'm looking forward to is use in a Jax-Rs-style HTTP API:

#[resource(path = "/foo/bar")]
impl MyResource {
    #[get("/person/:name")]
    fn get_person(
        &self,
        #[path_param = "name"] name: String,
        #[query_param = "limit"] limit: Option<u32>,
    )
    { ... }
}
@Centril

This comment has been minimized.

Copy link
Contributor

Centril commented Nov 20, 2018

@Robbepop Any progress? :)

@Robbepop

This comment has been minimized.

Copy link
Author

Robbepop commented Nov 20, 2018

I am so sorry for the delay. I will try to work on it as soon as possible.

@TedDriggs

This comment has been minimized.

Copy link

TedDriggs commented Jan 14, 2019

I'd be interested in using this in wasm_bindgen to add more specific TypeScript types to function arguments. For example:

#[wasm_bindgen]
impl RustLayoutEngine {
    #[wasm_bindgen(constructor)]
    pub fn new() -> Self {
        Default::default()
    }

    #[wasm_bindgen(typescript(return_type = "MapNode[]"))]
    pub fn layout(&self, 
    #[wasm_bindgen(typescript(type = "MapNode[]"))]
    nodes: Vec<JsValue>, 
    #[wasm_bindgen(typescript(type = "MapEdge[]"))]
    edges: Vec<JsValue>) -> Vec<JsValue> {
        unimplemented!()
    }
}

Currently, the arguments and return type of this function are all any[]. These annotations don't change the need for our Rust code to examine the objects received from JS at runtime, but the tighter types would help us catch problems at compile time that could turn into UI bugs later.

@Centril

This comment has been minimized.

Copy link
Contributor

Centril commented Feb 17, 2019

@Robbepop I've taken the liberty to polish the RFC a bit, provide clarifications, and so on. I hope that's OK. Could you give the changes a look? After that I think we should be in a position to move ahead with this proposal.

@Centril Centril added the I-nominated label Feb 27, 2019

@Robbepop

This comment has been minimized.

Copy link
Author

Robbepop commented Mar 4, 2019

@Robbepop I've taken the liberty to polish the RFC a bit, provide clarifications, and so on. I hope that's OK. Could you give the changes a look? After that I think we should be in a position to move ahead with this proposal.

I am so sorry - saw this just now.
Thank you for taking this over!

Edit:
@Centril I have read through the entire updated RFC. This got huge over time and it looks really good to me.
Thanks so much for taking it over and pushing it!

@Centril Centril removed the I-nominated label Mar 5, 2019

@Centril

This comment has been minimized.

Copy link
Contributor

Centril commented Mar 5, 2019

This RFC represents an extension to where attributes are permitted in the language. The extension is small, consistently applied, and has notable use cases as per comments in this RFC. I believe that the extension of attributes to other places in the language affords users to encode the EDSLs that they like. As this RFC is an important step on that road, I think we should move ahead with it and so therefore, I propose that we:

@rfcbot merge

@rfcbot

This comment has been minimized.

Copy link

rfcbot commented Mar 5, 2019

Team member @Centril has proposed to merge this. The next step is review by the rest of the tagged team members:

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.