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

impl FromStr for proc_macro::Literal #84717

Merged
merged 5 commits into from
May 20, 2021
Merged

Conversation

dtolnay
Copy link
Member

@dtolnay dtolnay commented Apr 29, 2021

Note that unlike impl FromStr for proc_macro::TokenStream, this impl does not permit whitespace or comments. The input string must consist of nothing but your literal.

  • "1".parse::<Literal>() ⟶ ok

  • "1.0".parse::<Literal>() ⟶ ok

  • "'a'".parse::<Literal>() ⟶ ok

  • "\"\n\"".parse::<Literal>() ⟶ ok

  • "0 1".parse::<Literal>() ⟶ LexError

  • " 0".parse::<Literal>() ⟶ LexError

  • "0 ".parse::<Literal>() ⟶ LexError

  • "/* comment */0".parse::<Literal>() ⟶ LexError

  • "0/* comment */".parse::<Literal>() ⟶ LexError

  • "0// comment".parse::<Literal>() ⟶ LexError


Use case

let hex_int: Literal = format!("0x{:x}", int).parse().unwrap();

The only way this is expressible in the current API is significantly worse.

let hex_int = match format!("0x{:x}", int)
    .parse::<TokenStream>()
    .unwrap()
    .into_iter()
    .next()
    .unwrap()
{
    TokenTree::Literal(literal) => literal,
    _ => unreachable!(),
};

@rust-highfive
Copy link
Collaborator

r? @petrochenkov

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Apr 29, 2021
@petrochenkov
Copy link
Contributor

Aren't APIs like this provided by higher-level crates like proc_macro2?
But looks like this case cannot be done precisely using the existing public API due to possible whitespace and comments around the literal.

The main alternative that I can see here is implementing FromStr for TokenTree instead.
(Which is the intermediate step from more general TokenStream to more specific Literal.)
In that case the literal would be obtainable as

let hex_int = match format!("0x{:x}", int).parse::<TokenTree>().unwrap() {
    TokenTree::Literal(literal) => literal,
    _ => unreachable!(),
};

and impl FromStr for Literal would became purely a convenience function.

@petrochenkov petrochenkov added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 3, 2021
@dtolnay
Copy link
Member Author

dtolnay commented May 3, 2021

Aren't APIs like this provided by higher-level crates like proc_macro2?

Nope. The proc-macro2 API is intended to be an exact clone of the proc-macro API and is not meant to expose anything beyond that.

It backports new proc-macro APIs to older versions of Rust where possible, though. Still it's not for something that isn't in proc-macro. New APIs land in proc-macro first (such as this PR) and then go into proc-macro2.


The main alternative that I can see here is implementing FromStr for TokenTree instead.

I prefer not to do this because of the ambiguity around whether whitespace/comments are permitted in the input. If not, Group parsing gets very weird. If yes, there's no longer a way to parse a literal that is only allowed to be a literal, other than writing your own whitespace/comment lexer. And if yes for groups and no for other TokenTree variants, ... doesn't seem great.

Besides, from an API design perspective FromStr for TokenTree does not strike me as useful, whereas FromStr for Literal is directly commonly useful. I feel it would be quite unusual to have input which you know is exactly one "token tree" but do not know which kind of token tree, and would be fine with it being a Group containing multiple tokens, but not fine with it being non-grouped multiple tokens. Also the choice of what Spacing to put on parsed Punct tokens is not good. Presumably it would just unconditionally put an Alone spacing, but it seems bizarre and I don't really know use cases where that would be always appropriate behavior.

I'd like to have FromStr for TokenStream to mean parsing arbitrary tokens/whitespace, and FromStr for Literal that is just effectively equivalent to a checked Symbol::intern(string) i.e. what you give it is the actual representation of the literal and nothing else.

@dtolnay dtolnay added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels May 3, 2021
@petrochenkov petrochenkov added I-nominated S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 3, 2021
@dtolnay dtolnay added needs-fcp This change is insta-stable, so needs a completed FCP to proceed. and removed I-nominated labels May 4, 2021
@dtolnay
Copy link
Member Author

dtolnay commented May 4, 2021

@rust-lang/libs:
@rfcbot fcp merge

Previous comment has some discussion of this impl in relation to impl FromStr for TokenStream (which is long stable) and impl FromStr for TokenTree (which I think I would prefer not to have).

@rfcbot
Copy link

rfcbot commented May 4, 2021

Team member @dtolnay has proposed to merge this. The next step is review by the rest of the tagged team members:

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

@rfcbot rfcbot added proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. labels May 4, 2021
library/proc_macro/src/lib.rs Outdated Show resolved Hide resolved
library/proc_macro/src/lib.rs Show resolved Hide resolved
src/test/ui/proc-macro/auxiliary/api/parse.rs Show resolved Hide resolved
@m-ou-se
Copy link
Member

m-ou-se commented May 5, 2021

There's an accepted RFC to turn prefix".." into a single token in Rust 2021, such that a later RFC could define e.g. z".." as a new type of literal. Looking at this PR, I'm now wondering what edition Literal::from_str("z\"\"") should use, since the string does not carry any span information. This is already a problem for TokenStream::from_str though, so not really a new concern for this PR.

I'll post a comment on that RFC PR about this.

@rfcbot rfcbot added final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. and removed proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. labels May 5, 2021
@rfcbot
Copy link

rfcbot commented May 5, 2021

🔔 This is now entering its final comment period, as per the review above. 🔔

@m-ou-se m-ou-se added S-waiting-on-fcp Status: PR is in FCP and is awaiting for FCP to complete. and removed S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). labels May 5, 2021
@rfcbot rfcbot added finished-final-comment-period The final comment period is finished for this PR / Issue. and removed final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. labels May 15, 2021
@rfcbot
Copy link

rfcbot commented May 15, 2021

The final comment period, with a disposition to merge, as per the review above, is now complete.

As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed.

The RFC will be merged soon.

@dtolnay
Copy link
Member Author

dtolnay commented May 19, 2021

Squashed 52dd0ffa490172640345feb5affa80524cb89712 into a8c3dd1b3046d7cfeea3ae3acbe64368eb357f71, 2acccefa1ee48b0dc36513d16dd1efce0e39b8e3 into 57fc619d0cfdd77050c52ed8a1cac8912c9f39d7, 84aa2c316910535848d6c664f3e51acb2d2ca080 into 56d9a4cca2c4f1bd846522c59358133a0c14f7fa, and rebased.

@petrochenkov
Copy link
Contributor

@bors r+

@bors
Copy link
Contributor

bors commented May 19, 2021

📌 Commit 34585cb has been approved by petrochenkov

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels May 19, 2021
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request May 19, 2021
impl FromStr for proc_macro::Literal

Note that unlike `impl FromStr for proc_macro::TokenStream`, this impl does not permit whitespace or comments. The input string must consist of nothing but your literal.

- `"1".parse::<Literal>()` ⟶ ok

- `"1.0".parse::<Literal>()` ⟶ ok

- `"'a'".parse::<Literal>()` ⟶ ok

- `"\"\n\"".parse::<Literal>()` ⟶ ok

- `"0 1".parse::<Literal>()` ⟶ LexError

- `" 0".parse::<Literal>()` ⟶ LexError

- `"0 ".parse::<Literal>()` ⟶ LexError

- `"/* comment */0".parse::<Literal>()` ⟶ LexError

- `"0/* comment */".parse::<Literal>()` ⟶ LexError

- `"0// comment".parse::<Literal>()` ⟶ LexError

---

## Use case

```rust
let hex_int: Literal = format!("0x{:x}", int).parse().unwrap();
```

The only way this is expressible in the current API is significantly worse.

```rust
let hex_int = match format!("0x{:x}", int)
    .parse::<TokenStream>()
    .unwrap()
    .into_iter()
    .next()
    .unwrap()
{
    TokenTree::Literal(literal) => literal,
    _ => unreachable!(),
};
```
bors added a commit to rust-lang-ci/rust that referenced this pull request May 20, 2021
Rollup of 8 pull requests

Successful merges:

 - rust-lang#84717 (impl FromStr for proc_macro::Literal)
 - rust-lang#85169 (Add method-toggle to <details> for methods)
 - rust-lang#85287 (Expose `Concurrent` (private type in public i'face))
 - rust-lang#85315 (adding time complexity for partition_in_place iter method)
 - rust-lang#85439 (Add diagnostic item to `CStr`)
 - rust-lang#85464 (Fix UB in documented example for `ptr::swap`)
 - rust-lang#85470 (Fix invalid CSS rules for a:hover)
 - rust-lang#85472 (CTFE Machine: do not expose Allocation)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit a1ac372 into rust-lang:master May 20, 2021
@rustbot rustbot added this to the 1.54.0 milestone May 20, 2021
@apiraino apiraino removed the to-announce Announce this issue on triage meeting label May 20, 2021
@dtolnay dtolnay added the relnotes Marks issues that should be documented in the release notes of the next release. label May 21, 2021
@dtolnay dtolnay deleted the literalfromstr branch May 21, 2021 02:11
cynecx added a commit to cynecx/rust-analyzer that referenced this pull request May 25, 2021
lnicola pushed a commit to lnicola/rust-analyzer that referenced this pull request May 29, 2021
lnicola pushed a commit to lnicola/rust-analyzer that referenced this pull request May 29, 2021
bors bot added a commit to rust-lang/rust-analyzer that referenced this pull request May 29, 2021
9047: fix: proc_macro_srv: temporary abi fix (rust-lang/rust#84717) r=lnicola a=lnicola

bors r+

9048: Add some lint completion tests r=Veykril a=Veykril

bors r+

Co-authored-by: cynecx <[email protected]>
Co-authored-by: Lukas Wirth <[email protected]>
bors bot added a commit to rust-lang/rust-analyzer that referenced this pull request May 29, 2021
9047: fix: proc_macro_srv: temporary abi fix (rust-lang/rust#84717) r=lnicola a=lnicola

bors r+

Co-authored-by: cynecx <[email protected]>
wip-sync pushed a commit to NetBSD/pkgsrc-wip that referenced this pull request Aug 12, 2021
Pkgsrc changes:
 * Bump bootstrap requirements to 1.53.0.
 * Adjust patches, adapt to upstream changes, adjust cargo checksums
 * If using an external llvm, require >= 10.0

Upsteream changes:

Version 1.54.0 (2021-07-29)
============================

Language
-----------------------

- [You can now use macros for values in built-in attribute macros.][83366]
  While a seemingly minor addition on its own, this enables a lot of
  powerful functionality when combined correctly. Most notably you can
  now include external documentation in your crate by writing the following.
  ```rust
  #![doc = include_str!("README.md")]
  ```
  You can also use this to include auto-generated modules:
  ```rust
  #[path = concat!(env!("OUT_DIR"), "/generated.rs")]
  mod generated;
  ```

- [You can now cast between unsized slice types (and types which contain
  unsized slices) in `const fn`.][85078]
- [You can now use multiple generic lifetimes with `impl Trait` where the
   lifetimes don't explicitly outlive another.][84701] In code this means
   that you can now have `impl Trait<'a, 'b>` where as before you could
   only have `impl Trait<'a, 'b> where 'b: 'a`.

Compiler
-----------------------

- [Rustc will now search for custom JSON targets in
  `/lib/rustlib/<target-triple>/target.json` where `/` is the "sysroot"
  directory.][83800] You can find your sysroot directory by running
  `rustc --print sysroot`.
- [Added `wasm` as a `target_family` for WebAssembly platforms.][84072]
- [You can now use `#[target_feature]` on safe functions when targeting
  WebAssembly platforms.][84988]
- [Improved debugger output for enums on Windows MSVC platforms.][85292]
- [Added tier 3\* support for `bpfel-unknown-none`
   and `bpfeb-unknown-none`.][79608]

\* Refer to Rust's [platform support page][platform-support-doc] for more
   information on Rust's tiered platform support.

Libraries
-----------------------

- [`panic::panic_any` will now `#[track_caller]`.][85745]
- [Added `OutOfMemory` as a variant of `io::ErrorKind`.][84744]
- [ `proc_macro::Literal` now implements `FromStr`.][84717]
- [The implementations of vendor intrinsics in core::arch have been
   significantly refactored.][83278] The main user-visible changes are
   a 50% reduction in the size of libcore.rlib and stricter validation
   of constant operands passed to intrinsics. The latter is technically
   a breaking change, but allows Rust to more closely match the C vendor
   intrinsics API.

Stabilized APIs
---------------

- [`BTreeMap::into_keys`]
- [`BTreeMap::into_values`]
- [`HashMap::into_keys`]
- [`HashMap::into_values`]
- [`arch::wasm32`]
- [`VecDeque::binary_search`]
- [`VecDeque::binary_search_by`]
- [`VecDeque::binary_search_by_key`]
- [`VecDeque::partition_point`]

Cargo
-----

- [Added the `--prune <spec>` option to `cargo-tree` to remove a package from
  the dependency graph.][cargo/9520]
- [Added the `--depth` option to `cargo-tree` to print only to a certain depth
  in the tree ][cargo/9499]
- [Added the `no-proc-macro` value to `cargo-tree --edges` to hide procedural
  macro dependencies.][cargo/9488]
- [A new environment variable named `CARGO_TARGET_TMPDIR` is
  available.][cargo/9375]
  This variable points to a directory that integration tests and
  benches can use as a "scratchpad" for testing filesystem operations.

Compatibility Notes
-------------------
- [Mixing Option and Result via `?` is no longer permitted in
  closures for inferred types.][86831]
- [Previously unsound code is no longer permitted where different
  constructors in branches could require different lifetimes.][85574]
- As previously mentioned the [`std::arch` instrinsics now uses
  stricter const checking][83278] than before and may reject some
  previously accepted code.
- [`i128` multiplication on Cortex M0+ platforms currently
  unconditionally causes overflow when compiled with `codegen-units
  = 1`.][86063]

[85574]: rust-lang/rust#85574
[86831]: rust-lang/rust#86831
[86063]: rust-lang/rust#86063
[86831]: rust-lang/rust#86831
[79608]: rust-lang/rust#79608
[84988]: rust-lang/rust#84988
[84701]: rust-lang/rust#84701
[84072]: rust-lang/rust#84072
[85745]: rust-lang/rust#85745
[84744]: rust-lang/rust#84744
[85078]: rust-lang/rust#85078
[84717]: rust-lang/rust#84717
[83800]: rust-lang/rust#83800
[83366]: rust-lang/rust#83366
[83278]: rust-lang/rust#83278
[85292]: rust-lang/rust#85292
[cargo/9520]: rust-lang/cargo#9520
[cargo/9499]: rust-lang/cargo#9499
[cargo/9488]: rust-lang/cargo#9488
[cargo/9375]: rust-lang/cargo#9375
[`BTreeMap::into_keys`]: https://doc.rust-lang.org/std/collections/struct.BTreeMap.html#method.into_keys
[`BTreeMap::into_values`]: https://doc.rust-lang.org/std/collections/struct.BTreeMap.html#method.into_values
[`HashMap::into_keys`]: https://doc.rust-lang.org/std/collections/struct.HashMap.html#method.into_keys
[`HashMap::into_values`]: https://doc.rust-lang.org/std/collections/struct.HashMap.html#method.into_values
[`arch::wasm32`]: https://doc.rust-lang.org/core/arch/wasm32/index.html
[`VecDeque::binary_search`]: https://doc.rust-lang.org/std/collections/struct.VecDeque.html#method.binary_search
[`VecDeque::binary_search_by`]: https://doc.rust-lang.org/std/collections/struct.VecDeque.html#method.binary_search_by
[`VecDeque::binary_search_by_key`]: https://doc.rust-lang.org/std/collections/struct.VecDeque.html#method.binary_search_by_key
[`VecDeque::partition_point`]: https://doc.rust-lang.org/std/collections/struct.VecDeque.html#method.partition_point
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. finished-final-comment-period The final comment period is finished for this PR / Issue. relnotes Marks issues that should be documented in the release notes of the next release. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants