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

proc_macro: Implementation of Extend (parameterized over Group, Ident, Punct, and Literal) for TokenStream #242

Closed
JohnScience opened this issue Jun 19, 2023 · 7 comments
Labels
ACP-accepted API Change Proposal is accepted (seconded with no objections) api-change-proposal A proposal to add or alter unstable APIs in the standard libraries T-libs-api

Comments

@JohnScience
Copy link

JohnScience commented Jun 19, 2023

Proposal

Problem statement

At the moment of writing, proc_macro::TokenStream implements parametrizations of core::iter::Extend over proc_macro::TokenStream and proc_macro::TokenTree, i.e. Extend<TokenStream> and Extend<TokenTree>. However, it is not implemented for parametrizations of Extend over

  1. proc_macro::Group
  2. proc_macro::Ident
  3. proc_macro::Punct
  4. proc_macro::Literal

which all can can be stored in respective variants of TokenTree.

I believe that this is a problem because it either

  1. causes unnecessary verbosity/boilerplate when one of Group, Ident, Punct, or Literal, is added to a TokenStream, as will be demonstrated further; or
  2. forces the developers to implement a local twin of Extend that they would implement for TokenStream, either manually or using a non-standard library.

Such inconveniences could push developers use quote, which results in higher compile times, even in situations where proc_macro with the proposed changes would suffice.

Also, it probably results in higher compile times in projects which extensively use procedural macros because these macros have to generate more Rust syntax.

Motivating examples or use cases

Before:

use proc_macro::{TokenStream, TokenTree, Ident, Span};
let mut ts = TokenStream::new();
ts.extend(iter::once(TokenTree::Ident(Ident::new("new_ident", Span::call_site()))));

After:

use proc_macro::{TokenStream, TokenTree, Ident, Span};
let mut ts = TokenStream::new();
ts.extend(iter::once(Ident::new("new_ident", Span::call_site())));

Once (or when) Extend::extend_one stabilizes, the difference will be even more noticeable.

Before:

use proc_macro::{TokenStream, TokenTree, Ident, Span};
let mut ts = TokenStream::new();
ts.extend_one(TokenTree::Ident(Ident::new("new_ident", Span::call_site())));

After:

use proc_macro::{TokenStream, TokenTree, Ident, Span};
let mut ts = TokenStream::new();
ts.extend_one(Ident::new("new_ident", Span::call_site()));

Solution sketch

impl Extend<Group> for TokenStream {
    fn extend<I: IntoIterator<Item = Group>>(&mut self, groups: I) {
        self.extend(groups.into_iter().map(TokenTree::Group))
    }
}

// And so on for the 3 remaining items, or we can use a macro_rules! for that.

Alternatives

Leave it as it is and delegate the implementation to a non-standard library via an extension trait that would be a twin of Extend. I dislike this approach because all these types belong to standard libraries and it would result in unnecessary complexity.

Links and related work

@JohnScience JohnScience added api-change-proposal A proposal to add or alter unstable APIs in the standard libraries T-libs-api labels Jun 19, 2023
@dtolnay
Copy link
Member

dtolnay commented Jun 19, 2023

Seconded. Please send PR.

@dtolnay dtolnay added the ACP-accepted API Change Proposal is accepted (seconded with no objections) label Jun 19, 2023
@eggyal
Copy link

eggyal commented Jun 19, 2023

TokenTree already implements From<Group> etc, so it's possible today to do stream.extend(iter::once(TokenTree::from(...))) which this proposal would simplify to stream.extend(iter::once(...)).

@JohnScience
Copy link
Author

JohnScience commented Jun 19, 2023

Development Progress

It's my first contribution to the standard library so I might make some silly mistakes as I make it happen. In order to make the development of the PR more transparent, I'll document what I'm doing along the way.

Steps taken

Found Standard library developers Guide.

Took a mental note of the Standard library developers Guide > 2. Building and debugging libraries,

Most of the instructions from the rustc-dev-guide also apply to the standard library since it is built with the same build system, so it is recommended to read it first.

From Rust Compiler Development Guide > Getting Started I learned about mentored issues.

Q: What are the benefits? It looks like I could use some help.

Followed Rust Compiler Development Guide > Building and debugging rustc > 1. How to build and run the compiler and shallow-cloned the repo, as suggested.

Tried ./x.py setup on Windows. Got the following error:

'.' is not recognized as an internal or external command, operable program or batch file.

Q: should the doc be updated to make it aware of Windows?

Tried python x.py setup and it worked.

Installed x, binary-wrapper around x.py, as suggested. However, x test library/proc_macro opened x.py in vscode, instead of running it.

Q: I guess I should report this bug later?

Forked rust-lang/rust, thereby getting JohnScience/rust.

Shallow-cloned and setup the fork.

Started adding a new feature gate by following Rust Compiler Development Guide > Contributing to Rust > 15. Feature Gates:

Q: I do need to add another feature gate, right?

  • Added the feature name to rustc_span/src/symbol.rs in the Symbols {...} block.

  • Created a Library Tracking Issue.

  • Added a feature gate declaration to rustc_feature/src/active.rs in the active declare_features block

  • Committed the change and tried to push it to the fork, and after waiting realized that the push failed because of the Git hook that was added with python x.py setup.

...
tidy check
Expected a gate test for the feature 'ts_extend_w_tt_item'.
Hint: create a failing test file named 'feature-gate-ts_extend_w_tt_item.rs'
      in the 'ui' test suite, with its failures due to
      missing usage of `#![feature(ts_extend_w_tt_item)]`.
Hint: If you already have such a test and don't want to rename it,
      you can also add a // gate-test-ts_extend_w_tt_item line to the test file.
tidy error: Found 1 features without a gate test.
some tidy checks failed
Build completed unsuccessfully in 0:08:43
error: failed to push some refs to 'https://github.com/JohnScience/rust'

Q: should the chapter on feature gates in Rust Compiler Development Guide be updated to add this info?

  • Found the ui test suit in the rust/tests/ui but I didn't quite make sense of the tests and respective .stderr files. At this point I foolishly didn't try to read Rust Compiler Development Guide and instead started doing something else.

  • After failing to figure out what kind of test the hook requires me to write on my own, I've engineered a prompt for ChatGPT and the tool answered my question seemingly right.


To resolve the error and pass the git hook used for the development of rustc, you need to create a test file for the feature gate you added. The error message suggests creating a failing test file named 'feature-gate-ts_extend_w_tt_item.rs' in the 'ui' test suite, which reflects the feature 'ts_extend_w_tt_item'. Here's how you can write the test file:

  1. Navigate to the 'ui' test suite directory of your Rust fork. The path would typically be <your-rust-fork>/src/test/ui/.

  2. Create a new file named 'feature-gate-ts_extend_w_tt_item.rs' in that directory.

  3. Open the 'feature-gate-ts_extend_w_tt_item.rs' file in a text editor and add the following content:

// gate-test-ts_extend_w_tt_item
// compile-flags: --error-format human --edition 2018
// check-pass
// aux-build:feature-gate-ts_extend_w_tt_item.rs

extern crate feature_gate_ts_extend_w_tt_item;

fn main() {
    // Add test code here that uses the feature gated by ts_extend_w_tt_item.
    // Make sure the code compiles and runs correctly when the feature is enabled.
}

Let's break down the contents of the file:

  • // gate-test-ts_extend_w_tt_item is a comment that specifies the name of the feature gate being tested. It should match the name of the feature gate you added in your commit.

  • // compile-flags: --error-format human --edition 2018 is a comment specifying the desired compilation flags for the test file. Modify these flags according to your requirements.

  • // check-pass is a comment indicating that the code in the test should compile successfully.

  • // aux-build:feature-gate-ts_extend_w_tt_item.rs is a comment specifying the name of the auxiliary build file if your feature gate requires one. Modify this line if your feature gate doesn't require an auxiliary build file.

  • Inside the main function, you should add the test code that uses the feature gated by ts_extend_w_tt_item. Ensure that the code compiles and runs correctly when the feature is enabled.

Save the file, commit your changes, and push them to your fork of rust-lang/rust. The git hook should now accept the push, as you have created the necessary test file for the feature gate.


  • I used rg "// gate-test-" in rust/test/ui and was happy to see multiple matches. Learn more about rg here. I quickly checked them and was reassured that Chat GPT saved the day.

  • In order to get more examples, I ran rg "// gate-test-" -l where -l flag is responsible for printing only the filenames with matches.

  • At this point I've realized that I'm silly not to check Rust Compiler Development Guide and started reading Rust Compiler Development Guide > Building and debugging rustc > 2. Testing the compiler

  • As I was going through rg "// gate-test-" -l in search of something that I could copy, I realized that proc macro crates are special. And I found that my test, just like the test tests/ui/proc-macro/allowed-signatures.rs should have #![crate_type = "proc-macro"] and extern crate proc_macro. Also, I probably will need // force-host comment because the target has to be the host.

Q: I haven't found the list of the comments in the Rust Compiler Development Guide. Is it a thing that is yet to be documented?

  • Started playing around with rg and huniq to find more comments but the terminal crashed. Due to that, I decided to create a test using only limited knowledge.

  • Faced problem related to LF vs CRLF. Fixed it in Visual Studio Code.

Q: Can't git hook either propose changes or apply them automatically?

  • Received the following error:
tidy error: following path contains more than 870 entries, you should move the test to some relevant subdirectory (current: 871): C:\Users\USER\Documents\github\rust\tests\ui

and after asking on official Discord server in #contribute channel figured out that the point of that lint is to prevent the mess from growing. I haven't figured out whether it goes to proc-macro or feature-gates, so I chose proc-macro.

  • Found out that it's currently impossible to have unstable trait implementations (Impl stability is not checked rust#55436). Therefore, I'm trying to figure out what I should do to insta-stabilize the feature.

Implementation status

Status: Postponed until the beginning of August

Possible statuses

Active - doing the work.
Coffee break - enjoying some rest but will come back soon (probably).
Lengthy break - sleep or walk in a park.

@m-ou-se
Copy link
Member

m-ou-se commented Jun 21, 2023

Started adding a new feature gate by following Rust Compiler Development Guide > Contributing to Rust > 15. Feature Gates:

That documentation is for compiler features. For library features, you don't need to touch any files in compiler/, nor do you need to add any feature gate tests. For library features, just an #[unstable] attribute is enough. (We should document this better, sorry.)

This specific feature however are trait implementations (for already stable types and trait), which unfortunately cannot be unstable. So your PR would need to add these implementations as stable right away (and requires team signoff through an FCP).

@jyn514
Copy link
Member

jyn514 commented Jun 21, 2023

It's my first contribution to the standard library so I might make some silly mistakes as I make it happen. In order to make the development of the PR more transparent, I'll document what I'm doing along the way.

@JohnScience this is extremely helpful, thank you ❤️ i don't have time right now but i plan to improve several of the issues you mentioned this weekend

@m-ou-se
Copy link
Member

m-ou-se commented Jun 21, 2023

@jyn514 and I just agreed to spend some time working together to make sure the rustc and std dev guides cover these things better and refer to each other in the proper places. Stay tuned. :)

Thanks for documenting your experience, @JohnScience.

@jyn514
Copy link
Member

jyn514 commented Jun 24, 2023

Q: should the doc be updated to make it aware of Windows?

I've opened https://github.com/rust-lang/rustc-dev-guide/pull/1701/files?short_path=cdf7c1e#diff-cdf7c1e6d2d1ec81510204b0c3250cb2c90880843f005bb942bafb84996a90b0 to improve the docs here.

and after asking on official Discord server in #contribute channel figured out that the point of that lint is to prevent the mess from growing. I haven't figured out whether it goes to proc-macro or feature-gates, so I chose proc-macro.

I made this error from tidy more helpful in https://github.com/rust-lang/rust/pull/113008/files#diff-31008ceb5198220e3ec8834cac868728b9db572697eb5a14e48b402edc9c2246.

Q: I guess I should report this bug later?

I cannot replicate this bug. It looks like you're using cmd.exe, but when I run x test library/proc_macro in cmd it does the right thing automatically. Are you sure that you've added ~/.cargo/bin to PATH?

Tried ./x.py setup on Windows. Got the following error:

We do not really support cmd.exe; it's horribly limited in what it can do. Powershell should work a lot better. I tried to document that in the rustc-dev-guide PR.

Q: I haven't found the list of the comments in the Rust Compiler Development Guide. Is it a thing that is yet to be documented?

This is documented at https://rustc-dev-guide.rust-lang.org/tests/headers.html#header-commands. I'm not sure how to make it easier to find.

  • Started playing around with rg and huniq to find more comments but the terminal crashed. Due to that, I decided to create a test using only limited knowledge.

    • Faced problem related to LF vs CRLF. Fixed it in Visual Studio Code.

Q: Can't git hook either propose changes or apply them automatically?

Can you talk more about the error you ran into here? do you mean this?

tidy check
tidy error: C:\Users\vboxuser\rust\tests\ui\feature-gates\foo.rs:1: CR character

it would be helpful if you could open an issue on rust-lang/rust asking for this to be --blessable :)

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Jun 25, 2023
…s, r=clubby789

Move some docs from the README to the dev-guide

and as a drive-by cleanup, improve the error message for `x test tidy` when a feature gate is missing.

This also improves the error message you get on Windows if python isn't installed.

cc rust-lang/libs-team#242 (comment), rust-lang/rustc-dev-guide#1701
@dtolnay dtolnay closed this as completed Jun 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ACP-accepted API Change Proposal is accepted (seconded with no objections) api-change-proposal A proposal to add or alter unstable APIs in the standard libraries T-libs-api
Projects
None yet
Development

No branches or pull requests

5 participants