Implement RFC#1559: allow all literals in attributes #35850

SergioBenitez · 2016-08-20T07:38:31Z

Implemented rust-lang/rfcs#1559, tracked by #34981.

rust-highfive · 2016-08-20T07:38:45Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @nikomatsakis (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

eddyb · 2016-08-20T07:43:07Z

src/librustc/hir/check_attr.rs

-                        continue
+            let name = word.name();
+            let message = match name {
+                Some(word) => match &*word {


This could probably be if let Some(word) = word.name() {.

Wait, doesn't .name() cause an error for repr(5) currently, that would be silenced?

I'll use if let .... The check you're referring to happens here (libsyntax/attr.rs find_repr_attrs()).

Actually, it's slightly cleaner to use match word.name(), so I'll switch it to that.

Might be able to do match word.name().as_ref().map_or("", |s| s) (or map_or("", |s| &**s)) - assuming that it has been previously validated.

Yeah, I considered that, but we don't know if it's a word at this point (though it doesn't matter too much, yet). Plus, it seems a bit more obvious to specifically match on the None case.

Two nested match are pretty ugly 😞.

Changed to (from IRC):

let name = match word.name() { Some(word) => word, None => continue, }; let message = match &*name { ... } span_err!(self.sess, attr.span, E0517, "{}", message);

eddyb · 2016-08-20T08:11:59Z

Looks pretty good to me, great job! 🎉

cc @nrc @cgswords @Manishearth

Mark-Simulacrum · 2016-08-20T14:18:08Z

src/librustc_incremental/assert_dep_graph.rs

-                    if meta_item.is_word() && id.is_none() {
-                        id = Some(meta_item.name().clone());
-                    } else {
-                        // FIXME better-encapsulate meta_item (don't directly access `node`)


node is still directly accessed in the span_bug! so perhaps the FIXME shouldn't be removed?

This happens in a lot of places without a similar comment. Is it more important to place such a comment here than in other places?

I added those fixmes; ultimately, there should be some encapsulation to make meta-items more opaque (so that their internal representation is less obvious and more resilient to changes). If it's done other places, this fixme should be added there, too.

Okay, I'll re-add them.

cgswords · 2016-08-20T19:49:49Z

There should be some run-pass tests.

cgswords · 2016-08-20T20:01:06Z

Aside from my comments and request for tests, this is basically all awesome. 🎉

Manishearth · 2016-08-21T09:13:24Z

When this is ready to merge, let me know, we can batch up everything in #31645 (comment)

nrc · 2016-08-22T03:04:22Z

src/librustc_borrowck/borrowck/mir/mod.rs

+                if let Some(mi) = item.meta_item() {
+                    if mi.check_name(name) {
+                        return Some(mi.clone())
+                    }


Needing double ifs here is annoying - could check_name return None for non-meta items? (More generally, I kind of hate the concept of meta-item - it is arbitrary and meaningless. We should do our best not to expose the concept outside of libsyntax).

Actually, we can call check_name directly on the list item. I'll change it to that.

Edit: Actually, we need to get the meta item somehow to return it. How do you feel about:

for item in items.iter().flat_map(|l| l.iter()) { match item.meta_item() { Some(mi) if mi.check_name(name) => return Some(mi.clone()), _ => continue } }

nrc · 2016-08-22T03:11:02Z

This should be feature-gated, right?

SergioBenitez · 2016-08-22T03:12:28Z

Why should this be feature gated? The syntax is backwards compatible, and custom attributes are nightly only.

eddyb · 2016-08-22T03:40:42Z

@SergioBenitez So you're saying all stable attributes do checks that prevent new uses of literals?

SergioBenitez · 2016-08-22T03:41:03Z

@eddyb: That's the intention with this PR, yeah.

jseyfried · 2016-08-23T08:19:36Z

src/libsyntax/attr.rs

-            span: span
-        }
-    })).collect()
+    v.into_iter().map(|(_, m)| sort_meta_item(m)).collect()


This used to recursively sort MetaItemKind::Lists but now does nothing.

(now that they can contain literals, sorting MetaItemKind::Lists might not even make sense)

Same as above.

bors · 2016-08-25T01:42:51Z

☔ The latest upstream changes (presumably #35764) made this pull request unmergeable. Please resolve the merge conflicts.

SergioBenitez · 2016-08-25T03:45:38Z

Rebased on master.

nrc · 2016-08-25T04:49:16Z

r+ with MetaListItem renamed and with the commits squashed. Thanks for the PR, awesome work!

SergioBenitez · 2016-08-25T05:47:57Z

@nrc: Done.

eddyb · 2016-08-25T06:01:49Z

@Manishearth r=nrc

bors · 2016-08-25T15:12:29Z

☔ The latest upstream changes (presumably #35979) made this pull request unmergeable. Please resolve the merge conflicts.

SergioBenitez · 2016-08-25T20:38:51Z

@Manishearth: Rebased.

nikomatsakis · 2016-08-26T20:07:45Z

@bors r=nrc

bors · 2016-08-26T20:07:46Z

📌 Commit 8250a26 has been approved by nrc

nikomatsakis · 2016-08-26T20:07:54Z

r? @nrc

jseyfried · 2016-08-26T20:46:59Z

This will land in the next breaking batch.
@bors r-

SergioBenitez · 2016-08-26T22:44:11Z

@jseyfried Wasn't this supposed to be part of the previous batch? Don't see the harm in landing it now if so.

jseyfried · 2016-08-26T22:50:07Z

@SergioBenitez Which previous batch? Afaik, none of the pending syntax-[breaking-change] PRs have landed yet (c.f. #31645 (comment)).

SergioBenitez · 2016-08-26T22:51:14Z

@jseyfried: #35979

jseyfried · 2016-08-26T22:53:09Z

Those aren't syntax-[breaking-change]s though.

Implement RFC#1559: allow all literals in attributes Implemented rust-lang/rfcs#1559, tracked by rust-lang#34981.

@Manishearth

Batch up libsyntax breaking changes Batch of the following syntax-[breaking-change] changes: - #35591: Add a field `span: Span` to `ast::Generics`. - #35618: Remove variant `Mod` of `ast::PathListItemKind` and refactor the remaining variant `ast::PathListKind::Ident` to a struct `ast::PathListKind_`. - #35480: Change uses of `Constness` in the AST to `Spanned<Constness>`. - c.f. `MethodSig`, `ItemKind` - #35728: Refactor `cx.pat_enum()` into `cx.pat_tuple_struct()` and `cx.pat_path()`. - #35850: Generalize the elements of lists in attributes from `MetaItem` to a new type `NestedMetaItem` that can represent a `MetaItem` or a literal. - #35917: Remove traits `AttrMetaMethods`, `AttributeMethods`, and `AttrNestedMetaItemMethods`. - Besides removing imports of these traits, this won't cause fallout. - Add a variant `Union` to `ItemKind` to future proof for `union` (c.f. #36016). - Remove inherent methods `attrs` and `fold_attrs` of `Annotatable`. - Use methods `attrs` and `map_attrs` of `HasAttrs` instead. r? @Manishearth

rust-highfive assigned nikomatsakis Aug 20, 2016

eddyb reviewed Aug 20, 2016
View reviewed changes

Mark-Simulacrum reviewed Aug 20, 2016
View reviewed changes

Manishearth mentioned this pull request Aug 21, 2016

Tracking issue for libsyntax breaking changes #31645

Closed

nrc reviewed Aug 22, 2016
View reviewed changes

jseyfried reviewed Aug 23, 2016
View reviewed changes

Implement RFC#1559: allow all literals in attributes.

8250a26

nikomatsakis assigned nrc Aug 26, 2016

rust-highfive unassigned nikomatsakis Aug 26, 2016

jseyfried added a commit to jseyfried/rust that referenced this pull request Aug 28, 2016

Rollup merge of rust-lang#35850 - SergioBenitez:master, r=nrc

6303640

Implement RFC#1559: allow all literals in attributes Implemented rust-lang/rfcs#1559, tracked by rust-lang#34981.

jseyfried mentioned this pull request Aug 28, 2016

Batch up libsyntax breaking changes #36066

Merged

bors merged commit 8250a26 into rust-lang:master Aug 30, 2016

kevinmehall added a commit to kevinmehall/rust-peg that referenced this pull request Sep 1, 2016

rust: Fixes for rust-lang/rust#35850

31e34b7

jseyfried mentioned this pull request Feb 6, 2017

implement #[repr(align)] (tracking issue for RFC 1358) #33626

Closed

SergioBenitez mentioned this pull request Aug 3, 2018

Allow all literals in attributes (Tracking Issue for RFC #1559) #34981

Closed

Implement RFC#1559: allow all literals in attributes #35850

Implement RFC#1559: allow all literals in attributes #35850

Conversation

SergioBenitez commented Aug 20, 2016

rust-highfive commented Aug 20, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SergioBenitez Aug 20, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eddyb commented Aug 20, 2016

Mark-Simulacrum Aug 20, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cgswords commented Aug 20, 2016

cgswords commented Aug 20, 2016

Manishearth commented Aug 21, 2016

Choose a reason for hiding this comment

SergioBenitez Aug 22, 2016 • edited Loading

Choose a reason for hiding this comment

nrc commented Aug 22, 2016

SergioBenitez commented Aug 22, 2016

eddyb commented Aug 22, 2016 • edited Loading

SergioBenitez commented Aug 22, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bors commented Aug 25, 2016

SergioBenitez commented Aug 25, 2016

nrc commented Aug 25, 2016

SergioBenitez commented Aug 25, 2016

eddyb commented Aug 25, 2016

bors commented Aug 25, 2016

SergioBenitez commented Aug 25, 2016 • edited Loading

nikomatsakis commented Aug 26, 2016

bors commented Aug 26, 2016

nikomatsakis commented Aug 26, 2016

jseyfried commented Aug 26, 2016

SergioBenitez commented Aug 26, 2016

jseyfried commented Aug 26, 2016

SergioBenitez commented Aug 26, 2016

jseyfried commented Aug 26, 2016

SergioBenitez Aug 20, 2016 •

edited

Loading

Mark-Simulacrum Aug 20, 2016 •

edited

Loading

SergioBenitez Aug 22, 2016 •

edited

Loading

eddyb commented Aug 22, 2016 •

edited

Loading

SergioBenitez commented Aug 25, 2016 •

edited

Loading