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/bridge: stop using a remote object handle for proc_macro Punct and Group #98188

Merged
merged 3 commits into from
Jun 28, 2022
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
76 changes: 16 additions & 60 deletions compiler/rustc_expand/src/proc_macro_server.rs
Original file line number Diff line number Diff line change
Expand Up @@ -14,8 +14,8 @@ use rustc_span::def_id::CrateNum;
use rustc_span::symbol::{self, kw, sym, Symbol};
use rustc_span::{BytePos, FileName, Pos, SourceFile, Span};

use pm::bridge::{server, ExpnGlobals, TokenTree};
use pm::{Delimiter, Level, LineColumn, Spacing};
use pm::bridge::{server, ExpnGlobals, Punct, TokenTree};
use pm::{Delimiter, Level, LineColumn};
use std::ops::Bound;
use std::{ascii, panic};

Expand Down Expand Up @@ -50,7 +50,7 @@ impl ToInternal<token::Delimiter> for Delimiter {
}

impl FromInternal<(TreeAndSpacing, &'_ mut Vec<Self>, &mut Rustc<'_, '_>)>
for TokenTree<Group, Punct, Ident, Literal>
for TokenTree<Span, Group, Ident, Literal>
{
fn from_internal(
((tree, spacing), stack, rustc): (TreeAndSpacing, &mut Vec<Self>, &mut Rustc<'_, '_>),
Expand Down Expand Up @@ -79,16 +79,16 @@ impl FromInternal<(TreeAndSpacing, &'_ mut Vec<Self>, &mut Rustc<'_, '_>)>
}
macro_rules! op {
($a:expr) => {
tt!(Punct::new($a, joint))
tt!(Punct { ch: $a, joint })
};
($a:expr, $b:expr) => {{
stack.push(tt!(Punct::new($b, joint)));
tt!(Punct::new($a, true))
stack.push(tt!(Punct { ch: $b, joint }));
tt!(Punct { ch: $a, joint: true })
}};
($a:expr, $b:expr, $c:expr) => {{
stack.push(tt!(Punct::new($c, joint)));
stack.push(tt!(Punct::new($b, true)));
tt!(Punct::new($a, true))
stack.push(tt!(Punct { ch: $c, joint }));
stack.push(tt!(Punct { ch: $b, joint: true }));
tt!(Punct { ch: $a, joint: true })
mystor marked this conversation as resolved.
Show resolved Hide resolved
}};
}

Expand Down Expand Up @@ -146,7 +146,7 @@ impl FromInternal<(TreeAndSpacing, &'_ mut Vec<Self>, &mut Rustc<'_, '_>)>
Lifetime(name) => {
let ident = symbol::Ident::new(name, span).without_first_quote();
stack.push(tt!(Ident::new(rustc.sess(), ident.name, false)));
tt!(Punct::new('\'', true))
tt!(Punct { ch: '\'', joint: true })
}
Literal(lit) => tt!(Literal { lit }),
DocComment(_, attr_style, data) => {
Expand All @@ -169,9 +169,9 @@ impl FromInternal<(TreeAndSpacing, &'_ mut Vec<Self>, &mut Rustc<'_, '_>)>
flatten: false,
}));
if attr_style == ast::AttrStyle::Inner {
stack.push(tt!(Punct::new('!', false)));
stack.push(tt!(Punct { ch: '!', joint: false }));
}
tt!(Punct::new('#', false))
tt!(Punct { ch: '#', joint: false })
}

Interpolated(nt) if let NtIdent(ident, is_raw) = *nt => {
Expand All @@ -192,7 +192,7 @@ impl FromInternal<(TreeAndSpacing, &'_ mut Vec<Self>, &mut Rustc<'_, '_>)>
}
}

impl ToInternal<TokenStream> for TokenTree<Group, Punct, Ident, Literal> {
impl ToInternal<TokenStream> for TokenTree<Span, Group, Ident, Literal> {
fn to_internal(self) -> TokenStream {
use rustc_ast::token::*;

Expand Down Expand Up @@ -288,27 +288,6 @@ pub struct Group {
flatten: bool,
}

#[derive(Copy, Clone, PartialEq, Eq, Hash)]
pub struct Punct {
ch: char,
// NB. not using `Spacing` here because it doesn't implement `Hash`.
joint: bool,
span: Span,
}

impl Punct {
fn new(ch: char, joint: bool, span: Span) -> Punct {
const LEGAL_CHARS: &[char] = &[
'=', '<', '>', '!', '~', '+', '-', '*', '/', '%', '^', '&', '|', '@', '.', ',', ';',
':', '#', '$', '?', '\'',
];
if !LEGAL_CHARS.contains(&ch) {
panic!("unsupported character `{:?}`", ch)
}
Punct { ch, joint, span }
}
}

#[derive(Copy, Clone, PartialEq, Eq, Hash)]
pub struct Ident {
sym: Symbol,
Expand Down Expand Up @@ -378,7 +357,6 @@ impl server::Types for Rustc<'_, '_> {
type FreeFunctions = FreeFunctions;
type TokenStream = TokenStream;
type Group = Group;
type Punct = Punct;
type Ident = Ident;
type Literal = Literal;
type SourceFile = Lrc<SourceFile>;
Expand Down Expand Up @@ -471,15 +449,15 @@ impl server::TokenStream for Rustc<'_, '_> {

fn from_token_tree(
&mut self,
tree: TokenTree<Self::Group, Self::Punct, Self::Ident, Self::Literal>,
tree: TokenTree<Self::Span, Self::Group, Self::Ident, Self::Literal>,
) -> Self::TokenStream {
tree.to_internal()
}

fn concat_trees(
&mut self,
base: Option<Self::TokenStream>,
trees: Vec<TokenTree<Self::Group, Self::Punct, Self::Ident, Self::Literal>>,
trees: Vec<TokenTree<Self::Span, Self::Group, Self::Ident, Self::Literal>>,
) -> Self::TokenStream {
let mut builder = tokenstream::TokenStreamBuilder::new();
if let Some(base) = base {
Expand Down Expand Up @@ -509,7 +487,7 @@ impl server::TokenStream for Rustc<'_, '_> {
fn into_trees(
&mut self,
stream: Self::TokenStream,
) -> Vec<TokenTree<Self::Group, Self::Punct, Self::Ident, Self::Literal>> {
) -> Vec<TokenTree<Self::Span, Self::Group, Self::Ident, Self::Literal>> {
// FIXME: This is a raw port of the previous approach (which had a
// `TokenStreamIter` server-side object with a single `next` method),
// and can probably be optimized (for bulk conversion).
Expand Down Expand Up @@ -577,28 +555,6 @@ impl server::Group for Rustc<'_, '_> {
}
}

impl server::Punct for Rustc<'_, '_> {
fn new(&mut self, ch: char, spacing: Spacing) -> Self::Punct {
Punct::new(ch, spacing == Spacing::Joint, self.call_site)
}

fn as_char(&mut self, punct: Self::Punct) -> char {
punct.ch
}

fn spacing(&mut self, punct: Self::Punct) -> Spacing {
if punct.joint { Spacing::Joint } else { Spacing::Alone }
}

fn span(&mut self, punct: Self::Punct) -> Self::Span {
punct.span
}

fn with_span(&mut self, punct: Self::Punct, span: Self::Span) -> Self::Punct {
Punct { span, ..punct }
}
}

impl server::Ident for Rustc<'_, '_> {
fn new(&mut self, string: &str, span: Self::Span, is_raw: bool) -> Self::Ident {
Ident::new(self.sess(), Symbol::intern(string), is_raw, span)
Expand Down
1 change: 0 additions & 1 deletion library/proc_macro/src/bridge/client.rs
Original file line number Diff line number Diff line change
Expand Up @@ -182,7 +182,6 @@ define_handles! {
Diagnostic,

'interned:
Punct,
Ident,
Span,
}
Expand Down
28 changes: 15 additions & 13 deletions library/proc_macro/src/bridge/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -65,19 +65,19 @@ macro_rules! with_api {
fn from_str(src: &str) -> $S::TokenStream;
fn to_string($self: &$S::TokenStream) -> String;
fn from_token_tree(
tree: TokenTree<$S::Group, $S::Punct, $S::Ident, $S::Literal>,
tree: TokenTree<$S::Span, $S::Group, $S::Ident, $S::Literal>,
) -> $S::TokenStream;
fn concat_trees(
base: Option<$S::TokenStream>,
trees: Vec<TokenTree<$S::Group, $S::Punct, $S::Ident, $S::Literal>>,
trees: Vec<TokenTree<$S::Span, $S::Group, $S::Ident, $S::Literal>>,
) -> $S::TokenStream;
fn concat_streams(
base: Option<$S::TokenStream>,
streams: Vec<$S::TokenStream>,
) -> $S::TokenStream;
fn into_trees(
$self: $S::TokenStream
) -> Vec<TokenTree<$S::Group, $S::Punct, $S::Ident, $S::Literal>>;
) -> Vec<TokenTree<$S::Span, $S::Group, $S::Ident, $S::Literal>>;
},
Group {
fn drop($self: $S::Group);
Expand All @@ -90,13 +90,6 @@ macro_rules! with_api {
fn span_close($self: &$S::Group) -> $S::Span;
fn set_span($self: &mut $S::Group, span: $S::Span);
},
Punct {
fn new(ch: char, spacing: Spacing) -> $S::Punct;
fn as_char($self: $S::Punct) -> char;
fn spacing($self: $S::Punct) -> Spacing;
fn span($self: $S::Punct) -> $S::Span;
fn with_span($self: $S::Punct, span: $S::Span) -> $S::Punct;
},
Ident {
fn new(string: &str, span: $S::Span, is_raw: bool) -> $S::Ident;
fn span($self: $S::Ident) -> $S::Span;
Expand Down Expand Up @@ -449,15 +442,24 @@ compound_traits!(
);

#[derive(Clone)]
pub enum TokenTree<G, P, I, L> {
pub struct Punct<S> {
pub ch: char,
pub joint: bool,
pub span: S,
}

compound_traits!(struct Punct<Sp> { ch, joint, span });

#[derive(Clone)]
pub enum TokenTree<S, G, I, L> {
mystor marked this conversation as resolved.
Show resolved Hide resolved
Group(G),
Punct(P),
Punct(Punct<S>),
Ident(I),
Literal(L),
}

compound_traits!(
enum TokenTree<G, P, I, L> {
enum TokenTree<Sp, G, I, L> {
Group(tt),
Punct(tt),
Ident(tt),
Expand Down
1 change: 0 additions & 1 deletion library/proc_macro/src/bridge/server.rs
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,6 @@ pub trait Types {
type FreeFunctions: 'static;
type TokenStream: 'static + Clone;
type Group: 'static + Clone;
type Punct: 'static + Copy + Eq + Hash;
type Ident: 'static + Copy + Eq + Hash;
type Literal: 'static + Clone;
type SourceFile: 'static + Clone;
Expand Down
36 changes: 17 additions & 19 deletions library/proc_macro/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -212,8 +212,8 @@ pub use quote::{quote, quote_span};
fn tree_to_bridge_tree(
tree: TokenTree,
) -> bridge::TokenTree<
bridge::client::Span,
bridge::client::Group,
bridge::client::Punct,
bridge::client::Ident,
bridge::client::Literal,
> {
Expand All @@ -238,8 +238,8 @@ impl From<TokenTree> for TokenStream {
struct ConcatTreesHelper {
trees: Vec<
bridge::TokenTree<
bridge::client::Span,
bridge::client::Group,
bridge::client::Punct,
bridge::client::Ident,
bridge::client::Literal,
>,
Expand Down Expand Up @@ -365,8 +365,8 @@ pub mod token_stream {
pub struct IntoIter(
std::vec::IntoIter<
bridge::TokenTree<
bridge::client::Span,
bridge::client::Group,
bridge::client::Punct,
bridge::client::Ident,
bridge::client::Literal,
>,
Expand Down Expand Up @@ -925,7 +925,7 @@ impl fmt::Debug for Group {
/// forms of `Spacing` returned.
#[stable(feature = "proc_macro_lib2", since = "1.29.0")]
#[derive(Clone)]
pub struct Punct(bridge::client::Punct);
pub struct Punct(bridge::Punct<bridge::client::Span>);

#[stable(feature = "proc_macro_lib2", since = "1.29.0")]
impl !Send for Punct {}
Expand Down Expand Up @@ -958,13 +958,20 @@ impl Punct {
/// which can be further configured with the `set_span` method below.
#[stable(feature = "proc_macro_lib2", since = "1.29.0")]
pub fn new(ch: char, spacing: Spacing) -> Punct {
Punct(bridge::client::Punct::new(ch, spacing))
const LEGAL_CHARS: &[char] = &[
'=', '<', '>', '!', '~', '+', '-', '*', '/', '%', '^', '&', '|', '@', '.', ',', ';',
':', '#', '$', '?', '\'',
];
if !LEGAL_CHARS.contains(&ch) {
panic!("unsupported character `{:?}`", ch);
}
Comment on lines +965 to +971
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't something like this be a match? That way, LLVM can optimize it to some arithmetic and bit twiddling?

(Oops, just checked and this is what it takes: https://godbolt.org/z/vP13eKjsW - I guess that's pretty nasty)

If you are going to use an array search, can you do an ASCII check first and make it a bytestring literal? So that it at least can hit some kind of memchr specialization or w/e.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, that's an easy change. The current code is literally verbatim cut-pasted from the code which used to be in proc_macro_server.rs:

const LEGAL_CHARS: &[char] = &[
'=', '<', '>', '!', '~', '+', '-', '*', '/', '%', '^', '&', '|', '@', '.', ',', ';',
':', '#', '$', '?', '\'',
];
if !LEGAL_CHARS.contains(&ch) {
panic!("unsupported character `{:?}`", ch)
}
.

FWIW the current code appears to actually compile to a somewhat efficient jump table (https://godbolt.org/z/P9d5366YT), and sloppily switching it to use &[u8] instead does appear to make it use memchr (https://godbolt.org/z/EfvaaMbbh)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The existing codegen seems like the nicest of the options (I wouldn't be too surprised if it performs better than the memchr variant), so I'm inclined to stick with it.

Punct(bridge::Punct { ch, joint: spacing == Spacing::Joint, span: Span::call_site().0 })
}

/// Returns the value of this punctuation character as `char`.
#[stable(feature = "proc_macro_lib2", since = "1.29.0")]
pub fn as_char(&self) -> char {
self.0.as_char()
self.0.ch
}

/// Returns the spacing of this punctuation character, indicating whether it's immediately
Expand All @@ -973,28 +980,19 @@ impl Punct {
/// (`Alone`) so the operator has certainly ended.
#[stable(feature = "proc_macro_lib2", since = "1.29.0")]
pub fn spacing(&self) -> Spacing {
self.0.spacing()
if self.0.joint { Spacing::Joint } else { Spacing::Alone }
}

/// Returns the span for this punctuation character.
#[stable(feature = "proc_macro_lib2", since = "1.29.0")]
pub fn span(&self) -> Span {
Span(self.0.span())
Span(self.0.span)
}

/// Configure the span for this punctuation character.
#[stable(feature = "proc_macro_lib2", since = "1.29.0")]
pub fn set_span(&mut self, span: Span) {
self.0 = self.0.with_span(span.0);
}
}

// N.B., the bridge only provides `to_string`, implement `fmt::Display`
// based on it (the reverse of the usual relationship between the two).
#[stable(feature = "proc_macro_lib", since = "1.15.0")]
impl ToString for Punct {
fn to_string(&self) -> String {
TokenStream::from(TokenTree::from(self.clone())).to_string()
self.0.span = span.0;
}
}

Expand All @@ -1003,7 +1001,7 @@ impl ToString for Punct {
#[stable(feature = "proc_macro_lib2", since = "1.29.0")]
impl fmt::Display for Punct {
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
f.write_str(&self.to_string())
write!(f, "{}", self.as_char())
}
}

Expand Down
2 changes: 1 addition & 1 deletion src/test/ui/proc-macro/invalid-punct-ident-1.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
// rustc-env:RUST_BACKTRACE=0

// FIXME https://github.com/rust-lang/rust/issues/59998
// normalize-stderr-test "thread.*panicked.*proc_macro_server.rs.*\n" -> ""
// normalize-stderr-test "thread.*panicked.*proc_macro.*lib.rs.*\n" -> ""
// normalize-stderr-test "note:.*RUST_BACKTRACE=1.*\n" -> ""
// normalize-stderr-test "\nerror: internal compiler error.*\n\n" -> ""
// normalize-stderr-test "note:.*unexpectedly panicked.*\n\n" -> ""
Expand Down