Deprecate `ItemModifier` and `ItemDecorator` traits #21265

aochagavia · 2015-01-16T22:17:44Z

Breaking changes:

Deprecate ItemModifier trait (use MultiItemModifier instead)
Deprecate ItemDecorator trait (use MultiItemDecorator instead)
Deprecate SyntaxExtension::Decorator and SyntaxExtension::Modifier (use the MultiDecorator and MultiModifier variants instead)

[breaking-change]

Fixes #21155 and #21154

rust-highfive · 2015-01-16T22:18:03Z

r? @brson

(rust_highfive has picked a reviewer for you, use r? to override)

aochagavia · 2015-01-16T22:19:02Z

@nick29581 Is this ok? Do you have any suggestions?

brson · 2015-01-16T22:26:32Z

r? @nick29581

nrc · 2015-01-19T20:33:48Z

LGTM. It needs a test - I think there is probably already a test for ItemDecorator somewhere, you could change that to use MultiItemDecorator, or you could create a new test for the Multi- version.

aochagavia · 2015-01-20T08:32:01Z

What kind of test should it be? run-pass?

I was unable to find a test for ItemDecorator. Could you point me to it?

nrc · 2015-01-20T09:14:50Z

probably run-pass-fulldeps. It doesn't look like there is a test for ItemDecorator, you should copy the one for Modifier/MultiModifier. Start with auxiliary/macro_crate_test.rs and see where the macros get used (not sure off the top of my head).

aochagavia · 2015-01-21T12:27:51Z

@nick29581 I have added a test

nrc · 2015-01-21T19:14:38Z

The test looks great, a few comments on the other commit though

aochagavia · 2015-01-21T22:05:11Z

@nick29581 I have addressed your comments. Thanks for reviewing!

I wasn't sure what to do to with cfg_attr. Right now it can be applied on any annotatable. Is that OK?

aochagavia · 2015-01-22T14:56:50Z

src/libsyntax/ext/cfg_attr.rs

    if attr::cfg_matches(&cx.parse_sess.span_diagnostic, cx.cfg.as_slice(), &**cfg) {
-        out.attrs.push(cx.attribute(attr.span, attr.clone()));
+        let attr = cx.attribute(attr.span, attr.clone());
+        fold_annotatable!(ann, item => item.attrs.push(attr))


@nick29581 Is this implementation of the fold_annotatable macro OK?

nrc · 2015-01-23T05:11:10Z

@bors r+ e7f010ef76ebc3fc0fc8261147236c7cbf9a1048

bors · 2015-01-23T05:27:12Z

⌛ Testing commit e7f010e with merge 31c6018...

bors · 2015-01-23T06:53:09Z

💔 Test failed - auto-mac-32-opt

aochagavia · 2015-01-23T09:17:17Z

Looks like the pretty printer didn't handle attributes on typedefs.

impl Trait for Type {
    #[something]
    type AssocType = AnotherType;
}

was printed as

impl Trait for Type {type
    AssocType
    =
    AnotherType;
}

I have just fixed it. r? @nick29581

nrc · 2015-01-27T23:18:12Z

@aochagavia r+, but needs a rebase

aochagavia · 2015-01-28T17:11:56Z

After rebasing I am getting the following error (in different places):

C:\Users\...\Desktop\rust\src\libsyntax\ext\base.rs:33:3: 33:47 error: incorrect stability attribute type
C:\Users\...\Desktop\rust\src\libsyntax\ext\base.rs:33 #[deprecated="Replaced by MultiItemDecorator"]

I tried adding #[unstable] but it doesn't fix it:

C:\Users\...\Desktop\rust\src\libsyntax\ext\base.rs:32:3: 32:11 error: incorrect stability attribute type
C:\Users\...\Desktop\rust\src\libsyntax\ext\base.rs:32 #[unstable]
                                                         ^~~~~~~~
C:\Users\...\Desktop\rust\src\libsyntax\ext\base.rs:33:3: 33:47 error: incorrect stability attribute type
C:\Users\...\Desktop\rust\src\libsyntax\ext\base.rs:33 #[deprecated="Replaced by MultiItemDecorator"]

Have you seen this before? What's going on?

nrc · 2015-01-28T19:51:21Z

@aochagavia probably something to do with http://discuss.rust-lang.org/t/psa-important-info-about-rustcs-new-feature-staging/82 (I haven't read it all myself nor have I seen these errors, so I can't be more specific)

[breaking-change]

It has been replaced by SyntaxExtension::MultiDecorator [breaking-change]

Replaced by SyntaxExtension::MultiModifier [breaking-change]

aochagavia · 2015-01-31T13:28:59Z

@nick29581 Thanks for the tip! It should be all right now.

nrc · 2015-02-02T01:23:25Z

@bors r+ 143f2db

bors · 2015-02-02T06:19:40Z

⌛ Testing commit 143f2db with merge ec03896...

bors · 2015-02-02T07:48:10Z

💔 Test failed - auto-mac-32-opt

aochagavia · 2015-02-02T20:19:58Z

@nick29581 I am unable to find the source of the failure. The output of --pretty expanded is really weird and I just can't make sense of it. Maybe this issue is too much for me.

nrc · 2015-02-02T20:26:07Z

@aochagavia A good start would be to reduce the test case to just a single expansion (it currently tests all the possible expansions, which will generate a lot of expanded code). You should then be able to tell whether there is one particular expansion which is broken or whether all of them have the same bug. If you post the output of --pretty expanded here (for the broken expansion if only one or any one of them if all of them) then I can try to help you make sense of it.

Manishearth · 2015-02-22T07:02:40Z

Needs rebase.

aochagavia · 2015-02-22T16:30:47Z

@Manishearth, @nick29581 I'm not going to finish this PR, since I would need to spend too much time to figure out how to fix it.

Manishearth · 2015-02-22T17:01:36Z

(I might take this over)

nrc · 2015-02-22T19:14:36Z

@Manishearth let me know if you decide either way, if you don't, I'll try and take it over the line

Manishearth · 2015-02-22T19:16:44Z

Already started.

https://github.com/Manishearth/rust/compare/multimodifier

Manishearth · 2015-02-23T03:40:35Z

Okay, can repro the failure

Manishearth · 2015-02-24T21:55:34Z

@nick29581 I won't have time to work on this for the next few days, feel free to take over :)

nrc · 2015-02-24T22:03:24Z

@Manishearth I'm in no rush. If you think you'll come back to it, it's probably better to keep it. I have a lot on too...

Manishearth · 2015-02-24T22:03:43Z

Not sure if I will :P

nrc · 2015-05-01T05:20:39Z

Finished in #25024

aochagavia · 2015-05-02T09:46:39Z

Awesome!

rust-highfive assigned brson Jan 16, 2015

rust-highfive assigned nrc and unassigned brson Jan 16, 2015

aochagavia mentioned this pull request Jan 20, 2015

#[derive(Copy="something")] should be rejected #21435

Closed

aochagavia changed the title ~~WIP: Deprecate ItemModifier and ItemDecorator traits~~ Deprecate ItemModifier and ItemDecorator traits Jan 21, 2015

aochagavia reviewed Jan 22, 2015
View reviewed changes

aochagavia added 7 commits January 31, 2015 11:38

Deprecate ItemModifier in favor of MultiItemModifier

53579e6

[breaking-change]

Deprecate ItemDecorator in favor of MultiItemDecorator

5769039

[breaking-change]

Add MultiItemDecorator

77d5921

Add MultiDecorator variant to SyntaxExtension enum

f97cff9

Deprecate SyntaxExtension::Decorator

6f233aa

It has been replaced by SyntaxExtension::MultiDecorator [breaking-change]

Deprecate SyntaxExtension::Modifier

2e07b0d

Replaced by SyntaxExtension::MultiModifier [breaking-change]

Add tests for MultiItemDecorator

01172ee

aochagavia added 3 commits January 31, 2015 11:38

Replace uses of Decorator and Modifier

aa3fe20

Fix pretty printer

5af4df4

Fix rebase issues

143f2db

aochagavia closed this Feb 22, 2015

huonw mentioned this pull request Jan 6, 2016

Allow decorators on methods, etc. #21154

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Deprecate `ItemModifier` and `ItemDecorator` traits #21265

Deprecate `ItemModifier` and `ItemDecorator` traits #21265

aochagavia commented Jan 16, 2015

rust-highfive commented Jan 16, 2015

aochagavia commented Jan 16, 2015

brson commented Jan 16, 2015

nrc commented Jan 19, 2015

aochagavia commented Jan 20, 2015

nrc commented Jan 20, 2015

aochagavia commented Jan 21, 2015

nrc commented Jan 21, 2015

aochagavia commented Jan 21, 2015

aochagavia Jan 22, 2015

nrc commented Jan 23, 2015

bors commented Jan 23, 2015

bors commented Jan 23, 2015

aochagavia commented Jan 23, 2015

nrc commented Jan 27, 2015

aochagavia commented Jan 28, 2015

nrc commented Jan 28, 2015

aochagavia commented Jan 31, 2015

nrc commented Feb 2, 2015

bors commented Feb 2, 2015

bors commented Feb 2, 2015

aochagavia commented Feb 2, 2015

nrc commented Feb 2, 2015

Manishearth commented Feb 22, 2015

aochagavia commented Feb 22, 2015

Manishearth commented Feb 22, 2015

nrc commented Feb 22, 2015

Manishearth commented Feb 22, 2015

Manishearth commented Feb 23, 2015

Manishearth commented Feb 24, 2015

nrc commented Feb 24, 2015

Manishearth commented Feb 24, 2015

nrc commented May 1, 2015

aochagavia commented May 2, 2015

Deprecate ItemModifier and ItemDecorator traits #21265

Deprecate ItemModifier and ItemDecorator traits #21265

Conversation

aochagavia commented Jan 16, 2015

rust-highfive commented Jan 16, 2015

aochagavia commented Jan 16, 2015

brson commented Jan 16, 2015

nrc commented Jan 19, 2015

aochagavia commented Jan 20, 2015

nrc commented Jan 20, 2015

aochagavia commented Jan 21, 2015

nrc commented Jan 21, 2015

aochagavia commented Jan 21, 2015

aochagavia Jan 22, 2015

Choose a reason for hiding this comment

nrc commented Jan 23, 2015

bors commented Jan 23, 2015

bors commented Jan 23, 2015

aochagavia commented Jan 23, 2015

nrc commented Jan 27, 2015

aochagavia commented Jan 28, 2015

nrc commented Jan 28, 2015

aochagavia commented Jan 31, 2015

nrc commented Feb 2, 2015

bors commented Feb 2, 2015

bors commented Feb 2, 2015

aochagavia commented Feb 2, 2015

nrc commented Feb 2, 2015

Manishearth commented Feb 22, 2015

aochagavia commented Feb 22, 2015

Manishearth commented Feb 22, 2015

nrc commented Feb 22, 2015

Manishearth commented Feb 22, 2015

Manishearth commented Feb 23, 2015

Manishearth commented Feb 24, 2015

nrc commented Feb 24, 2015

Manishearth commented Feb 24, 2015

nrc commented May 1, 2015

aochagavia commented May 2, 2015

Deprecate `ItemModifier` and `ItemDecorator` traits #21265

Deprecate `ItemModifier` and `ItemDecorator` traits #21265