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 for attributes on statements and blocks. #16

Merged
merged 7 commits into from
Jul 15, 2014

Conversation

huonw
Copy link
Member

@huonw huonw commented Mar 20, 2014

Allow attributes on more places inside functions, such as statements and blocks.

```rust
fn name(method: SslMethod) -> &'static str {
match method {
Sslv2 => "SSLv2",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing attribute?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is meant to be a bad example:

i.e. the following is invalid:

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I misread

@emberian
Copy link
Member

👍

@andrew-d
Copy link

👍 - this would be nice to have 😄

@esummers
Copy link

Looks unnatural to me if extended to if/else. My random thought is that maybe if/else should look more like a match:

if foo => { },
#[cfg(foo)] if bar > 10 => println!("Hello World."),
_ => println!("So Long, and Thanks for All the Fish.");

@huonw
Copy link
Member Author

huonw commented Mar 22, 2014

I've been thinking about it a little more, and I actually think my second proposed if/else extension (i.e. else #[attr] { ... } and else #[attr] if ...) is the most natural (other than completely changing the if syntax... which seems like a non-starter); it's natural because if/else look like

if ... {
     ...
} else X

where X can be a block { ... } or another if ... { ... chain, and these two things get attributes from this RFC already, so (theoretically) there won't even need to be a special case for parsing those attributes: they come automatically out of the new grammar.

@esummers
Copy link

I think that looks the best too. else is basically a four character wide comma.

@andrew-d
Copy link

So, what's the process to get a consensus on this? Having this merged would be super useful for me right now.

@alexcrichton
Copy link
Member

@andrew-d, the process is outlined in RFC #1, the relevant part being:

Eventually, somebody on the core team will either accept the RFC by merging the pull request and assigning the RFC a number, at which point the RFC is 'active', or reject it by closing the pull request.

Discussions of RFCs are predominately occurring during weekly meetings right now, but this is not a hard requirement.

@nikomatsakis
Copy link
Contributor

I'm generally in favor. I'm vaguely concerned about parser ambiguities but it should probably be ok. (Oh, my kingdom for a proper grammar.)

@chris-morgan
Copy link
Member

@nikomatsakis I don't see any ambiguities: attributes and only attributes start with #. This is still LL(1).

@nikomatsakis
Copy link
Contributor

@chris-morgan I generally agree, I've just learned to be cautious when it comes to annotations that precede blocks etc.

@flaper87
Copy link

I'm in favor 👍 🍰

@lilyball
Copy link
Contributor

lilyball commented Apr 1, 2014

👍

@pnkfelix
Copy link
Member

pnkfelix commented Apr 8, 2014

I don't think its explicitly spelled out in the RFC, so I will ask: Are only outer-attributes allowed to be used in this manner? Or are inner-attributes allowed as well?

Supporting inner-attributes could help improve the appearance of e.g. the if/else examples, depending on what one's opinion is of the examples that were presented.

So, If the intent is that inner-attributes should also be allowed, then we would need to resolve questions of what their scope is (I'm guessing it would be the most tightly enclosing block), and it would probably be good to require all inner-attributes to occur at the start of the block, before any items/declarations/outer-attributes etc).

But I would be fine if inner-attributes are not supported, or are not supported in the first iteration of this feature.

} else {
}
}
```
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could this include blocks?

#[attr] {
    // ...
}

{
    #![attr]
    // ...
}

@alexcrichton
Copy link
Member

There was a good amount of discussion in today's meeting about this RFC.

The general opinion is that it seems a little odd to limit attributes to just statements. One big reason is that many things which look like statements are actually expressions, such as:

fn foo() {
    if some_condition() { println!("test") } // this is an expression, not a statement
}

There are some concerns about how far-reaching it would be to modify the grammar to allow attributes on all expressions (which is the ideal ending point for something like this).

In the meantime, how would you feel about paring down the RFC to just attributes on match arms? That seems have a very real and pressing use case, and it pretty much works as expected because it can appear anywhere a match can appear.

@huonw
Copy link
Member Author

huonw commented Apr 16, 2014

Should I cut it down here? Or open a new RFC, since it will be a rather large simplification (maybe this RFC can be co-opted for the more general statements/expressions attributes).

@alexcrichton
Copy link
Member

I think either way is ok. It sounds like statements/expressions will want to be a separate RFC, so feel free to pick either one to be this one (whichever is easiest)

@huonw
Copy link
Member Author

huonw commented Apr 17, 2014

I opened #49 since pretty much none of the discussion here is about match arms. (I'll update this to be statement/expression specific at some point.)

@huonw
Copy link
Member Author

huonw commented May 17, 2014

Updated.

@huonw huonw changed the title RFC for attributes on match arms and statements. RFC for attributes on statements and blocks. May 17, 2014
@anasazi
Copy link

anasazi commented Jun 3, 2014

+1 Attributes on all the things!

@emberian
Copy link
Member

emberian commented Jun 3, 2014

A possibly better way to implement this generically would be to add an
sidetable containing all NodeId => Vec, and then we don't need
to pollute the entire AST with 4 words everywhere for the attributes it can
have. This does mean giving more things Ids I think. Possibly not.

On Tue, Jun 3, 2014 at 2:09 PM, Eric Reed notifications@github.com wrote:

+1 Attributes on all the things!


Reply to this email directly or view it on GitHub
#16 (comment).

http://octayn.net/

@pnkfelix
Copy link
Member

pnkfelix commented Jun 5, 2014

Some notes from discussion during triage meeting:

  1. The scope of the RFC needs to be precisely defined. The detailed description section includes a fuzzy optional extension to expressions. Trim it to exactly what we'll support.
  2. There are some questions that need to be resolved.
    • From one POV it seem safe to restrict the scope of the rfc to just blocks and statements, but every expression can be turned into a statement, so its not clear whether that actually restricts the scope of the RFC all that much.
    • Also it would be good to know whether inner attributes are including the scope of this RFC, and if so, what do they apply to (just blocks, presumably?)

@nikomatsakis you had some additional thoughts that seemed a little implementation oriented in terms of what it would be like to parse code that has an arbitrary number of attributes followed by some statment-or-expression, feel free to to to clarify my points above.

Anyway, @huonw , we invite you to revise the RFC and then we will try to address it formally at a team meeting.

@sfackler
Copy link
Member

sfackler commented Jun 6, 2014

As a point of reference, Java 8 significantly expanded the parts of the grammar that can be tagged with annotations: http://java.dzone.com/articles/java-8-type-annotations

@huonw
Copy link
Member Author

huonw commented Jun 27, 2014

Updated to include expressions more definitely. There are some annoying interactions with if detailed in the last section.


This can be addressed by having `#[attr] if cond { ...` be an exterior
attribute (applying to the whole `if`/`else` chain) and `if cond
#[attr] { ... ` be an interior attribute (applying to only the current
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Markdown is interpreting this as a heading :-)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Haha, whoops. Fixed.

@tbu-
Copy link
Contributor

tbu- commented Jul 4, 2014

Not related to attributes in general, but allowing #[cfg]s everywhere makes it harder to reason about the code, like in C/C++ when you have preprocessor logic all over a function instead of different functions where the platform specific behavior is isolated.

That's why I think it might be a bit dangerous to allow this as this might make people move away from separate functions for #[cfg]-dependant behavior.

huonw added 2 commits July 9, 2014 07:37
- explicitly disallowing attributes on `if`/`else` due to weirdness
- provide alternatives for this
- add a drawbacks section discussing syntactic lock-down and the
  antipattern of `#[cfg]`
- clarify that this RFC is just for parsing attributes, not requiring that
  `#[cfg]` stripping be added in all these new places.
general `unsafe` block).

Only allowing attributes on "statement expressions" that is,
expressions at the top level of a block,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@huonw oops, missed this earlier: I assume you wanted to finish this sentence with a period, or some expository text explaining why narrowing the scope of the change could be good, or more importantly, why it is not good, i.e. reiterating why it is important to support annotating (most) expressions.

sentences.
@brson
Copy link
Contributor

brson commented Jul 15, 2014

@slimsag
Copy link

slimsag commented Jun 7, 2015

shouldn't this be removed from the active RFC list if it's been accepted/merged?

@sfackler
Copy link
Member

sfackler commented Jun 7, 2015

The active list contains RFCs that have been accepted but not implemented.

@Kimundi
Copy link
Member

Kimundi commented Nov 8, 2015

I'm currently in the process of implementing this, and in accordance with the current RFC text attributes will be initially prohibited on if expressions.

However, thinking about it I wonder if the following scheme could be used:

#[covers_remaining_three] if expr #[covers_block] {
    // ...
} else #[covers_remaining_two] if expr #[covers_block] {
    // ...
} else #[covers_block] {
    // ...
}

The idea being that an attribute before an if token applies to the whole, chained if expression, while a attribute before the block only applies to that block expression.

Of course, the "disadvantage" here is that for consistency you'd want to support those block-only attributes on anything taking a block, like the loop expressions. And then there would be more than one location per block-taking expression where a attribute might apply, which while not ambiguous could become potentially confusing.

@huonw
Copy link
Member Author

huonw commented Nov 10, 2015

@Kimundi, that is in fact one of the alternatives. I think nailing down this aspect of the behaviour is probably worth an amendment RFC.

bors added a commit to rust-lang/rust that referenced this pull request Dec 4, 2015
…kfelix

See rust-lang/rfcs#16 and #15701

- Added syntax support for attributes on expressions and all syntax nodes in statement position.
- Extended `#[cfg]` folder to allow removal of statements, and
of expressions in optional positions like expression lists and trailing
block expressions.
- Extended lint checker to recognize lint levels on expressions and
locals.
- As per RFC, attributes are not yet accepted on `if` expressions.

Examples:
  ```rust
let x = y;
{
        ...
}
assert_eq!((1, #[cfg(unset)] 2, 3), (1, 3));

let FOO = 0;
```

Implementation wise, there are a few rough corners and open questions:
- The parser work ended up a bit ugly.
- The pretty printer change was based mostly on guessing.
- Similar to the `if` case, there are some places in the grammar where a new `Expr` node starts,
  but where it seemed weird to accept attributes and hence the parser doesn't. This includes:
  - const expressions in patterns
  - in the middle of an postfix operator chain (that is, after `.`, before indexing, before calls)
  - on range expressions, since `#[attr] x .. y` parses as  `(#[attr] x) .. y`, which is inconsistent with
    `#[attr] .. y` which would parse as `#[attr] (.. y)`
- Attributes are added as additional `Option<Box<Vec<Attribute>>>` fields in expressions and locals.
- Memory impact has not been measured yet.
- A cfg-away trailing expression in a block does not currently promote the previous `StmtExpr` in a block to a new trailing expr. That is to say, this won't work:
```rust
let x = {
    #[cfg(foo)]
    Foo { data: x }
    #[cfg(not(foo))]
    Foo { data: y }
};
```
- One-element tuples can have their inner expression removed to become Unit, but just Parenthesis can't. Eg, `(#[cfg(unset)] x,) == ()` but `(#[cfg(unset)] x) == error`. This seemed reasonable to me since tuples and unit are type constructors, but could probably be argued either way.
- Attributes on macro nodes are currently unconditionally dropped during macro expansion, which seemed fine since macro disappear at that point?
- Attributes on `ast::ExprParens` will be prepend-ed to the inner expression in the hir folder.
- The work on pretty printer tests for this did trigger, but not fix errors regarding macros:
  - expression `foo![]` prints as `foo!()`
  - expression `foo!{}` prints as `foo!()`
  - statement `foo![];` prints as `foo!();`
  - statement `foo!{};` prints as `foo!();`
  - statement `foo!{}` triggers a `None` unwrap ICE.
withoutboats pushed a commit to withoutboats/rfcs that referenced this pull request Jan 15, 2017
@Centril Centril added A-syntax Syntax related proposals & ideas A-attributes Proposals relating to attributes labels Nov 23, 2018
cramertj added a commit to cramertj/rfcs that referenced this pull request Jan 23, 2019
Remove LocalWaker and simplify the RawWakerVTable
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-attributes Proposals relating to attributes A-syntax Syntax related proposals & ideas
Projects
None yet
Development

Successfully merging this pull request may close these issues.