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

Deprecated - please migrate to the paste crate #45

Closed
gnzlbg opened this issue Nov 7, 2018 · 21 comments
Closed

Deprecated - please migrate to the paste crate #45

gnzlbg opened this issue Nov 7, 2018 · 21 comments

Comments

@gnzlbg
Copy link
Collaborator

gnzlbg commented Nov 7, 2018

I don't really know how to easily reach those using interpolate_idents, but @dtolnay recently published paste: a crate that does the same thing that interpolate_idents does but uses a procedural macro and works in stable Rust 1.30.0 and up. That is, no more breakage due to rustc changes.

The syntax to use it is slightly different: paste uses [< and >] as delimiters instead of just [ and ], which means that one does not need to write [[ ]] anymore when indexing arrays. We have been happily using it in packed_simd for a week.

While interpolate_idents has served us very well for a very long time, maybe it is time to deprecate it. We could change the readme to point to the paste crate and upload one last version to crates.io to show this there as well. The next time the crate breaks, we could just leave it broken, and tell people to migrate.

An alternative would be to maybe the next time something breaks, just re-export the paste crate from interpolate-idents or something like that. But I don't think it is worth maintaining two crates here.

@SkylerLipthay
Copy link
Owner

This is fantastic news! Based on your recent experience, are you confident that paste can fully replace interpolate_idents? I'm sure there are good reasons (probably because of procedural macro limitations?) that it was necessary to split between paste::item! and paste::expr!, but I'm wondering if this would cause problems for any users of interpolate_idents.

As far as reaching current users of this crate, it's probably easiest (even if it's not elegant) to deprecate and let users figure out the path appropriate from there. Perhaps we'll discover paste cannot act as a perfect replacement, or perhaps deprecation will go without a hitch. Either way, users of interpolate_idents are no doubt accustomed to the constant breakage caused by this dependency, so they should be relieved to see there's a stable alternative in active development.

Let me know what your thoughts are on the technical consequences of deprecation. I'm in favor of deprecation if you think the majority of current use cases can be solved with paste.

Thanks for bringing this to my attention and for your continuing work.

@gnzlbg
Copy link
Collaborator Author

gnzlbg commented Nov 7, 2018

I'm sure there are good reasons (probably because of procedural macro limitations?) that it was necessary to split between paste::item! and paste::expr!, but I'm wondering if this would cause problems for any users of interpolate_idents.

@dtolnay replaced almost all usages of interpolate_idents in packed_simd with paste::item, but had to replace one or two uses with paste::item_with_macros. packed_simd did not need paste::expr anywhere, but I don't know how representative that would be of the ecosystem using interpolate_idents!.

There are a couple of crates in crates.io currently depending on interpolate_idents - I hope I do not miss any of the maintainers here: @YeyaSwizaw , @aidanhs , @ezrosent, @joshlf , @ambaxter

Maybe it would be good if they could try migrating their own crates to paste and report back with how that went ? I will continue to help maintaining interpolate_idents until there are no stake holders left, but afterwards just adding a "DEPRECATED: use paste instead" to the readme would be enough I guess.

@SkylerLipthay
Copy link
Owner

I've updated the README with 4685d79. Feel free to modify the text if it's not entirely conveying of the situation. I agree, let's hold off on full deprecation for a bit until we hear some thoughts from interpolate_idents consumers. Let me know if I can be of any further help in the meantime.

@gnzlbg gnzlbg mentioned this issue Nov 7, 2018
@samuelsleight
Copy link

@gnzlbg Thanks for letting me know about this! Using paste was a nice drop-in replacement, though I'm also not using paste::expr anywhere.

I've published a new version of my crate, so I'm no longer depending on interpolate_idents

@joshlf
Copy link
Contributor

joshlf commented Nov 8, 2018

Unfortunately paste won't work for me and @ezrosent; if you look at the use in our object-alloc-test crate here, you'll see that we use interpolate_idents! in statement position. Switching to paste::item! results in the following error: "procedural macros cannot be expanded to statements".

@dtolnay
Copy link

dtolnay commented Nov 8, 2018

Where is it used in statement position?

Can you adjust the code to use it in item position instead?

@joshlf
Copy link
Contributor

joshlf commented Nov 8, 2018

Where is it used in statement position?

On this line.

Can you adjust the code to use it in item position instead?

I tried that by changing:

    ($fn:ident, $prefix:ident $(, $arg:tt)*) => (
        paste::item! {
            call_macro!($fn,
                        ([<$prefix _0001_byte>], $crate::types::Byte1 $(,$arg)*),
                        ([<$prefix _0002_byte>], $crate::types::Byte2 $(,$arg)*),

to this:

    ($fn:ident, $prefix:ident $(, $arg:tt)*) => (
            call_macro!($fn,
                        (paste::item! { [<$prefix _0001_byte>] }, $crate::types::Byte1 $(,$arg)*),
                        (paste::item! { [<$prefix _0002_byte>] }, $crate::types::Byte2 $(,$arg)*),

Unfortunately, that caused other errors:

error: no rules expected the token `::`
  --> src/types.rs:154:1
   |
4  | macro_rules! make_default_test {
   | ------------------------------ when calling this macro
...
12 | call_for_all_types_prefix!(make_default_test, default_test);
   | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ no rules expected the token `::`
   |

@dtolnay
Copy link

dtolnay commented Nov 8, 2018

I think you are looking at the wrong line -- the line you linked to is only evaluated in statement position if call_for_all_types_prefix is invoked in statement position. So you need to look at invocations of call_for_all_types_prefix rather than that line.

I would think all invocations of call_for_all_types_prefix would be in item position because it seems designed to generate test functions, and test functions aren't supported within a function body i.e. all of that code is only useful if invoked in item position.

@joshlf
Copy link
Contributor

joshlf commented Nov 8, 2018

Ah, looks like you're right; I was calling it from main in a documentation example.

However, now I run into this problem:

error: proc macro panicked
  --> src/types.rs:154:1
   |
12 | call_for_all_types_prefix!(make_default_test, default_test);
   | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
   |
   = help: message: `"$crate"` is not a valid identifier

@dtolnay
Copy link

dtolnay commented Nov 8, 2018

That looks like rust-lang/rust#55640. For now it should be okay to replace $crate with object_alloc_test since call_for_all_types_prefix is never called from within object_alloc_test. The point of $crate is that it is replaced by the name of the current crate if called from a different crate, and replaced by nothing if called from within the same crate. In this case all calls are from a different crate so it is always being replaced by object_alloc_test.

@joshlf
Copy link
Contributor

joshlf commented Nov 8, 2018

OK looks like, with that temporary fix in place, everything works for us!

@gnzlbg
Copy link
Collaborator Author

gnzlbg commented Nov 8, 2018

Going through the reverse dependencies on crates.io, the following still depend on interpolate idents:

There is a PR with a fix for interpolate_idents so it will soon be working for a while, but it would be really nice to be able to move to something that does not break.

@gnzlbg
Copy link
Collaborator Author

gnzlbg commented Nov 9, 2018

So packed_simd has been released without the interpolate idents dependency. I suppose that since object-alloc-test was already updated upstream a release will follow soon, and I guess that would apply to slab-alloc as well.

That means that only two crates will depend on interpolate idents for the reasonable future ayzim and expert.

@dtolnay
Copy link

dtolnay commented Nov 9, 2018

The interpolate_idents dependency was removed from expert in ambaxter/expert-rs@9ee1597 and is not used in master.

@gnzlbg
Copy link
Collaborator Author

gnzlbg commented Nov 9, 2018

The readme of ayzim states: "Master currently needs: rustup default nightly-2017-03-03"

So it will probably not need to worry about any of this since it is requiring a fixed version of rustc.

So I guess this is it ? All crates appear to have upgraded to paste at least on master. If you aren't able to do a crates.io release before interpolate-idents breaks again, let me know, and I'll patch it one last time.

@dtolnay
Copy link

dtolnay commented Nov 9, 2018

Here's another list to push on if you feel like it:
https://github.com/search?q=%22plugin+interpolate_idents%22&type=Code

@gnzlbg gnzlbg changed the title Announcement - paste crate Deprecated - please migrate to the paste crate Nov 9, 2018
@gnzlbg
Copy link
Collaborator Author

gnzlbg commented Nov 9, 2018

cc the following github-repositories use interpolate idents but are not released on crates.io:

@sdleffler/knoll-rs, @efyang/xiami-api-rs, @rust-av/flv-demuxer-test, @zolkko/pug-rust, @Oipo/nanogui-rust-sdl, @expobrain/ryaml, @majorz/parallax, @majorz/apache2-rs, @snim2/testglobsoquestion, @Gen-OS/bbb-proof-of-concept, @code-ape/rust_python_extension, @dai1975/fiatproof, @rust-lang-nursery/rustc-perf (EDIT: packed_simd is part of rustc-perf, this has been updated), @sybblow/rust-python-example, sdleffler/peony-rs, @johalun/rustkpi , @Commander-lol/rust-swerve, @borispf/pymotifs, @AdrianDanis/rlk, @traff/snakemeter, @lemonrock/rust-ucx, @miyamofigo/rust-bytebuffer , @justinmichaud/ion , @aidanhs/frametool

@Commander-lol
Copy link

Just saw this issue - my use case is generating function names for tests, so Paste is a perfectly fine replacement. Thanks for the tag 👍

@gnzlbg
Copy link
Collaborator Author

gnzlbg commented Jan 22, 2019

@SkylerLipthay so interpolate_idents broke again, this time forever. Could you disable the travis cron job for regularly testing the crate ? It's time to pull the plug!

@SkylerLipthay
Copy link
Owner

🎊 🎉 Thanks for all of your attention during this process @gnzlbg, it's been a pleasure!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

6 participants