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
Merged
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
143 changes: 143 additions & 0 deletions active/0000-more-attributes.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,143 @@
- Start Date: 2014-03-20
- RFC PR #: (leave this empty)
- Rust Issue #: (leave this empty)

# Summary

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

# Motivation

One sometimes wishes to annotate things inside functions with, for
example, lint `#[allow]`s, conditional compilation `#[cfg]`s, branch
weight hints and even extra semantic (or otherwise) annotations for
external tools.

For the lints, one can currently only activate lints at the level of
the function which is possibly larger than one needs, and so may allow
other "bad" things to sneak through accidentally. E.g.

```rust
#[allow(uppercase_variable)]
let L = List::new(); // lowercase looks like one or capital i
```

For the conditional compilation, the work-around is duplicating the
whole containing function with a `#[cfg]`. A case study is
[sfackler's bindings to OpenSSL](https://github.com/sfackler/rust-openssl),
where many distributions remove SSLv2 support, and so that portion of
Rust bindings needs to be conditionally disabled. The obvious way to
support the various different SSL versions is an enum

```rust
pub enum SslMethod {
#[cfg(sslv2)]
/// Only support the SSLv2 protocol
Sslv2,
/// Only support the SSLv3 protocol
Sslv3,
/// Only support the TLSv1 protocol
Tlsv1,
/// Support the SSLv2, SSLv3 and TLSv1 protocols
Sslv23,
}
```

However, all `match`s can only mention `Sslv2` when the `cfg` is
active, i.e. the following is invalid:

```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

Sslv3 => "SSLv3",
_ => "..."
}
}
```

A valid method would be to have two definitions: `#[cfg(sslv2)] fn
name(...)` and `#[cfg(not(sslv2)] fn name(...)`. The former has the
`Sslv2` arm, the latter does not. Clearly, this explodes exponentially
for each additional `cfg`'d variant in an enum.

Branch weights would allow the careful micro-optimiser to inform the
compiler that, for example, a certain match arm is rarely taken:

```rust
match foo {
Common => {}
#[cold]
Rare => {}
}
```

The sort of things one could do with other arbitrary annotations are

```rust
#[allowed_unsafe_actions(ffi)]
#[audited="2014-04-22"]
unsafe { ... }
```

and then have an external tool that checks that that `unsafe` block's
only unsafe actions are FFI, or a tool that lists blocks that have
been changed since the last audit or haven't been audited ever.


# Detailed design

Normal attribute syntax:

```rust
fn foo() {
#[attr]
let x = 1;

#[attr]
foo();

#[attr]
match x {
#[attr]
Thing => {}
}

#[attr]
if foo {
} 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]
    // ...
}


# Alternatives

There aren't really any general alternatives; one could probably hack
around the conditional-enum-variants & matches with some macros and
helper functions to share as much code as possible; but in general
this won't work.

The other instances could be approximated with macros and helper
functions, but to an even lesser degree (e.g. how would one annotate a
general `unsafe` block).

# Unresolved questions

- Should one be able to annotate the `else` branch(es) of an `if`? e.g.
Copy link
Member

Choose a reason for hiding this comment

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

Maybe, but I don't think it's a big concern.

Copy link
Member

Choose a reason for hiding this comment

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

If we go down this road, I assume there would be a distinction between:

#[attr] if foo { ... } else { ... } // applies to whole statement, including both branches

and

if foo #[attr] { ... } else { ... } // only applies to the then-branch

I don't have a problem with this; if you cfg-disable a then-branch, you get a compilation error, presumably. (Though I do think it might motivate support for inner attributes.)

Copy link
Member

Choose a reason for hiding this comment

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

I think this is an important detail. As I see it, #[attr] if ..., the attribute would apply to the expression, but if foo { #![attr] ... } would have it apply to the block. I do not want to allow if foo #[attr] { ... }, I find it confusing.

Copy link
Member

Choose a reason for hiding this comment

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

I'm not inclined to disallow if foo #[attr] { ... } though I also would probably find inner-attributes more readable.

(I'd suggest we "just lint for it", but ... Does our lint infrastructure have enough info to try to catch the outer-attribute form here? Or are all the attributes boiled away by the time the lints run?)

Copy link
Member Author

Choose a reason for hiding this comment

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

I was thinking that if ... { ... } was a single unit, i.e. the {} are part of the if not a separate block. Maybe having both separately would be OK (I hadn't thought about it at all).

Copy link
Contributor

Choose a reason for hiding this comment

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

@huonw I have the same feeling. if expr { ... } feels like a single unit to me. If #[attr] were allowed on just the { ... } then I would expect if expr expr to be valid (e.g. if true println!("this works?");. But it's not valid.

Given that, I also find the suggestion of if expr { ... } else #[attr] if expr { ... } to be invalid. An else-statement is also a syntactical construct where the body is either an if-statement or a braced block. And if you really think this should be valid, then I question what happens if the #[cfg(foo)] evaluates to false? You then have a bare else with no associated code. I think that if you want an attribute on an else-if then you just need to nest, as in if expr { ... } else { #[attr] if expr { ... } }. Usage of this should be rare enough that this nesting is not a problem.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, and given all that I said above, I think the inner-attribute form doesn't make sense, as it appears as though it should affect the block, not the entire if-statement (especially if it occurs inside of an else-block). Similarly, putting the attribute in between the expression and the block makes no sense.

Disallowing these usages also resolves the question of how to deal with needing to select between several behaviors. if expr #[cfg(one)] { ... } #[cfg(two)] { ... } #[cfg(three)] { ... } looks pretty nonsensical, especially if two of the configs match (or if none match).

Additionally, if we did allow #[attr] on just the block, that would imply that we should allow it on all expressions, not just statements. I can see why that might be desired, but in actual practice it seems like it would be extremely confusing. Restricting it to blocks instead of arbitrary expressions would lessen the confusion, but would reintroduce confusion over why you can't put an attribute on the block of an if-statement (and allowing that brings up the problems I already mentioned).

Overall, I think the simplest thing to do is to just allow it on match arms and on statements. Not on blocks or expressions (unless those happen to be statements as well).


```rust
if foo {
} #[attr] else if bar {
} #[attr] else {
}
```

or maybe

```rust
if foo {
} else #[attr] if bar {
} else #[attr] {
}
```