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

Multiple Attributes in an Attribute Container #2600

Closed
wants to merge 2 commits into from

Conversation

Havvy
Copy link
Contributor

@Havvy Havvy commented Nov 24, 2018

Allow #[attr1, attr2] thingy.

Rendered


## Attribute Macro Input `TokenStream` Changes

Before this RFC, when an attriute macro is executed, it is passed two
Copy link
Contributor

Choose a reason for hiding this comment

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

Typo: "attriute"

`#[attr1, macro_attr] thing` | `"#[attr1, ] thing"`
`#[macro_attr, attr1] thing` | `"#[attr1] thing"`
`#[attr1, macro_attr, attr2] thing` | `"#[attr1, attr2] thing"`
`#[] #[macro_attr] thing` | `"#[] thing"` or `"thing"` (see unresolved questions)
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm wary of guaranteeing contents of input streams with such precision (at least right now).

For example, we don't specify what cfg_attr(predicate, a, b) unwraps into - #[a, b] or #[a] #[b], but now it makes observable difference (it would be reasonable to expect the former, but it's actually the latter).
Also, it may be convenient to split all multi-attribute containers into single-attribute containers in the initial implementation.

Macro authors should not rely on differences between #[a] #[b] and #[a, b] in general, and should support both, so we don't get much from specifying precise tokens.

Copy link
Member

Choose a reason for hiding this comment

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

Macro authors should not rely on differences between #[a] #[b] and #[a, b]

When a macro author highlights the span of the attribute item containing e.g. #[a, b,] from #[a, b, c] where some c has been removed, what span does that point to? a, b without the #[] or the c? Similarly, does #[a] from #[a, b, c] refer to just the a? I'd really like to see the exact semantics of whatever attribute modification and span transformation we do specified, both for the sake of this RFC and for #2539.

@Centril Centril added T-lang Relevant to the language team, which will review and decide on the RFC. A-syntax Syntax related proposals & ideas A-attributes Proposals relating to attributes labels Nov 25, 2018
--- | ---
`#[macro_attr] thing` | `"thing"`
`#[attr1] #[macro_attr] thing` | `"#[attr1] thing"`
`#[attr1, macro_attr] thing` | `"#[attr1, ] thing"`
Copy link
Contributor

Choose a reason for hiding this comment

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

What is the rationale for including the , here? Why not normalize the input before by having "#[attr1] thing" instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No hard reason. I just figured that "remove the name, ignore preceding commas and whitespace" is an easy consistent thing to do. Nobody should really care that it looks like #[attr1, ] instead of #[attr1]. Is there a value in adding complexity by normalizing?

Copy link
Member

Choose a reason for hiding this comment

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

After normalizing, what spans are provided to the procedural macro? It seems like they could wind up pointing to code that was different from what the user wrote originally, which could be confusing especially in cases where macros try to modify the users' code as an example for how to fix errors.


The changes are as follows:

* _Attribute_ becomes _AttributeContainer_ (Or removed? Is it actually used?)
Copy link
Contributor

Choose a reason for hiding this comment

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

Elaborate on the question here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

_Attribute_ in the reference is just InnerAttribute | OuterAttribute, but there's nowhere in the grammar that allows both, so it should probably just be removed from the grammar.

@nikomatsakis
Copy link
Contributor

In a prior lang team meeting, @cramertj had a lot of concerns about the interaction of this with procedural macros -- e.g., if you have #[macro] #[foo, bar], does that get "normalized" in the input to macro etc? @petrochenkov wrote some similar concerns here.

It definitely seems like the RFC should specify clearly the interaction here. I'm also of the opinion that we should do some sort of normalization so that macros behave consistently without a lot of effort, but I don't know what the details ought to be. =)

@Centril Centril self-assigned this Jan 3, 2019
@cramertj
Copy link
Member

cramertj commented Jan 8, 2019

I have a number of concerns here around the precise semantics of how providing macros with a TokenStream that doesn't look exactly like the code provided the user will work, but those concerns mostly all apply just as well to #2539, so I won't file this as an official concern on the RFC, and I'll check my box in the case of an FCP.

@graydon
Copy link

graydon commented Jan 12, 2019

Opposed, though not with vehemence, just a reasonable dose of conservatism. The drawbacks stated are clear enough:

The standard drawbacks to allowing more syntax exists. It's yet another thing to learn. It's only a small win over just having multiple attribute blocks

Those are reasons enough not to do this (as with so many of the open PRs here).

@Centril
Copy link
Contributor

Centril commented Jan 12, 2019

However, in this instance, cfg_attr(condition, attr1, attr2, ..) is already permitted so having #[attr1, attr2, ...] fits well with that.

@graydon
Copy link

graydon commented Jan 12, 2019

"It has symmetry" is not a reason to do a thing. It adds more ways to do a thing nobody especially suffers from not having more ways to do.

@Centril
Copy link
Contributor

Centril commented Jan 12, 2019

Symmetry is however not the sole motivation:

It lets us express our intent more clearly when attributes added for the same reason are grouped together.

I think this is apt; for example: #[test, should_panic] #[allow(foobar)]] fn some_random_test() {...} -- the first two are related whereas the second not so much, grouping them offers more clarity IMO.

@graydon
Copy link

graydon commented Jan 12, 2019

I'll admit this is a minor cost that is not going to break the bank, but like .. "indicating relatedness" can be accomplished today just by putting an extra newline between them, or putting the first two on a single line, or adding a comment, or putting things in a mod if the attr is shared across a bunch of items. Lots of ways to express group-y-ness, little reason to believe they're inadequate (and if they're not -- will it stop here? there are all sorts of things one might want to indicate relatedness-of in the language).

I'm only objecting here because this seems superfluous, like a part of a trend to reach for "add-or-change stuff in the language" where it's really not necessary. At this stage in a language's maturation, necessity should be the mother of invention, not plausibility.

@camden-smallwood-zz
Copy link

Was it necessary to add use/import grouping? No. There was still a gain from it despite introducing new synax to learn:

use std::ops::Add;
use std::ops::Sub;
// versus
use std::ops::{Add, Sub};

This is just as viable of an addition to the language as attribute grouping:

#[repr(C)]
#[derive(Clone, Copy)]
pub struct Point {
    pub x: i32,
    pub y: i32
}
// versus
#[repr(C), derive(Clone, Copy)]
pub struct Point {
    pub x: i32,
    pub y: i32
}

Could someone please explain why it would be a bad thing to support both of these paradigms? I don't get it.

@scottmcm
Copy link
Member

scottmcm commented Sep 1, 2020

Closing without prejudice as requested by the author.

Please feel free to re-open in future if you decide you'd like to come back to it.

@scottmcm scottmcm closed this Sep 1, 2020
@scottmcm scottmcm added the postponed RFCs that have been postponed and may be revisited at a later time. label Sep 1, 2020
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 postponed RFCs that have been postponed and may be revisited at a later time. T-lang Relevant to the language team, which will review and decide on the RFC.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants