Serialize additional data for procedural macros #63269

Aaron1011 · 2019-08-04T20:43:59Z

Split off from #62855

This PR serializes the declaration Span and attributes for all
procedural macros. This allows Rustdoc to properly render doc comments
and source links when performing inlinig procedural macros across crates

rust-highfive · 2019-08-04T20:44:02Z

r? @zackmdavis

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

Aaron1011 · 2019-08-04T20:44:08Z

r? @eddyb

src/librustc/hir/lowering.rs

eddyb · 2019-08-09T17:06:26Z

r? @petrochenkov

petrochenkov · 2019-08-09T18:26:33Z

Oh no.
@eddyb, I specifically wanted you to review the metadata part (#62855 (comment)).

eddyb · 2019-08-10T06:43:58Z

src/librustc_metadata/cstore.rs


    /// Whether or not this crate should be consider a private dependency
    /// for purposes of the 'exported_private_dependencies' lint
    pub private_dep: bool
 }

+pub struct FullProcMacro {


Maybe call this ProcMacroDef?

There's already a struct called ProcMacroDef in src/libsyntax_ext/proc_macro_harness.rs. I didn't want to have two different structs with the same name.

eddyb · 2019-08-10T06:45:28Z

src/librustc_metadata/schema.rs

+    /// This needs to come before 'def_path_table',
+    /// as we need use data from it when decoding `def_path_table.
+    /// This also needs to come at the very end of the 'Lazy/LazySeq' data,
+    /// as we need all of the other data in order to deserialize it


The last two lines here don't make sense: a Lazy/LazySeq is just an offset into the file (hence "lazy"), so the order doesn't matter at all.

I meant to refer to the encoding order, which does matter. However, now that I lazily deserialize the proc macros, it doesn't matter.

eddyb · 2019-08-10T06:45:54Z

src/librustc_metadata/schema.rs

+    /// This also needs to come at the very end of the 'Lazy/LazySeq' data,
+    /// as we need all of the other data in order to deserialize it
+    pub proc_macro_data: Option<LazySeq<ProcMacroData>>,
+    pub def_path_table: Lazy<hir::map::definitions::DefPathTable>,


There's no need to move this.

src/librustc_metadata/schema.rs

eddyb · 2019-08-10T06:48:24Z

src/librustc_metadata/creader.rs

+        // that's *almost* complete - it's just missing 'proc_macros' and 'def_path_table'.
+        // We then deserialize 'proc_macros' and 'def_path_table', using (cmeta, self.sess)
+        // as our deserializer. Then, we replace the dummy 'proc_macros' and 'def_path_table'
+        // with the data we just deserialized.


The correct solution is to make loading the proc macros lazy.

eddyb · 2019-08-10T06:48:57Z

src/librustc_metadata/creader.rs

@@ -581,8 +597,10 @@ impl<'a> CrateLoader<'a> {
    /// implemented as dynamic libraries, but we have a possible future where
    /// custom derive (and other macro-1.1 style features) are implemented via
    /// executables and custom IPC.
-    fn load_derive_macros(&mut self, root: &CrateRoot<'_>, dylib: Option<PathBuf>, span: Span)
-                          -> Vec<(ast::Name, Lrc<SyntaxExtension>)> {
+    fn load_derive_macros(&mut self, root: &CrateRoot<'_>, dylib: Option<PathBuf>, span: Span,


Heh, this should be load_proc_macros nowadays.

eddyb · 2019-08-10T06:51:00Z

src/librustc_metadata/cstore_impl.rs

@@ -203,11 +203,6 @@ provide! { <'tcx> tcx, def_id, other, cdata,
            DefId { krate: def_id.krate, index }
        })
    }
-    proc_macro_decls_static => {
-        cdata.root.proc_macro_decls_static.map(|index| {


I don't understand, this was supposed to be used to compute the symbol name, what happened to that?

eddyb · 2019-08-10T06:56:51Z

src/libsyntax/ast.rs

+#[derive(Clone, RustcEncodable, RustcDecodable, Debug)]
+pub struct ProcMacroInfo {
+    pub span: Span,
+    pub attrs: Vec<Attribute>


@petrochenkov This is the part I was r?-ing you for but it's simpler than I initially thought.

eddyb · 2019-08-10T06:57:25Z

src/libsyntax_ext/proc_macro_harness.rs

+            proc_macro_infos.borrow_mut().push(ProcMacroInfo {
+                span: cd.span,
+                attrs: cd.raw_attrs.clone()
+            });


@petrochenkov I guess this is what I want to make sure you have nothing against.

This is something I have a lot against, actually.

Aren't fn items encoded into the metadata already (including spans and attributes)?
As I understand it, a compiled proc macro crate has two views - the "regular" one with the original fn items and everything, and the "proc-macro facade" one masking all the original items and making it look like the crate only exposes macros.

So we just need to link from the proc-macro view (CrateMetadata::proc_macros) to the real items somehow instead of cloning some data from those items, keeping them in AST and encoding them for the second time.

Ah yeah, we'd just need to map the DefId's.

Would it make sense to unconditionally decode and store the 'real' def_path_table, but continue to use a 'fake' table for most operations on proc macro crates?

I suppose, yeah. At this point I'm not sure why we don't use the original DefIds directly, and just change what def_kind returns?

eddyb · 2019-08-10T07:03:28Z

src/librustc_metadata/cstore_impl.rs

@@ -427,7 +422,7 @@ impl cstore::CStore {
    pub fn load_macro_untracked(&self, id: DefId, sess: &Session) -> LoadedMacro {
        let data = self.get_crate_data(id.krate);
        if let Some(ref proc_macros) = data.proc_macros {
-            return LoadedMacro::ProcMacro(proc_macros[id.index.to_proc_macro_index()].1.clone());
+            return LoadedMacro::ProcMacro(proc_macros[id.index.to_proc_macro_index()].ext.clone());


@petrochenkov Does this need to be cached, or could the ext be loaded here?

Resolver already has a cache for this (macro_map in fn get_macro_by_def_id), so load_macro_untracked can just load the extension from metadata.

eddyb · 2019-08-10T07:06:16Z

src/librustc_metadata/decoder.rs

-            return Lrc::new([]);
+            return Lrc::from(
+                self.proc_macros.as_ref().unwrap()[node_id.to_proc_macro_index()].attrs
+                    .clone()


You could have attributes be lazy, like a regular Entry.

eddyb · 2019-08-10T07:08:54Z

src/librustc_metadata/creader.rs

+                      macros.len(), decls.len());
+        }
+
+        let extensions = macros.zip(decls.iter()).map(|(proc_macro, &decl) | {


I think CrateMetadata should hold onto decls (as a &'static [ProcMacro]) rather than the FullProcMacros.

bors · 2019-08-12T06:21:06Z

☔ The latest upstream changes (presumably #63469) made this pull request unmergeable. Please resolve the merge conflicts.

rust-highfive · 2019-08-14T00:09:12Z

The job x86_64-gnu-llvm-6.0 of your PR failed (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.

2019-08-13T23:49:48.9863276Z ##[command]git remote add origin https://github.com/rust-lang/rust
2019-08-13T23:49:49.0042814Z ##[command]git config gc.auto 0
2019-08-13T23:49:49.0090370Z ##[command]git config --get-all http.https://github.com/rust-lang/rust.extraheader
2019-08-13T23:49:49.0137032Z ##[command]git config --get-all http.proxy
2019-08-13T23:49:49.0275049Z ##[command]git -c http.extraheader="AUTHORIZATION: basic ***" fetch --force --tags --prune --progress --no-recurse-submodules --depth=2 origin +refs/heads/*:refs/remotes/origin/* +refs/pull/63269/merge:refs/remotes/pull/63269/merge
---
2019-08-13T23:50:23.8100287Z do so (now or later) by using -b with the checkout command again. Example:
2019-08-13T23:50:23.8100320Z 
2019-08-13T23:50:23.8100551Z   git checkout -b <new-branch-name>
2019-08-13T23:50:23.8100605Z 
2019-08-13T23:50:23.8100661Z HEAD is now at 48edf7fda Merge 6ff269308d88711ac3efbd4b0e73700605159ca9 into 60960a260f7b5c695fd0717311d72ce62dd4eb43
2019-08-13T23:50:23.8277050Z ##[section]Starting: Collect CPU-usage statistics in the background
2019-08-13T23:50:23.8280226Z ==============================================================================
2019-08-13T23:50:23.8280290Z Task         : Bash
2019-08-13T23:50:23.8280355Z Description  : Run a Bash script on macOS, Linux, or Windows
---
2019-08-13T23:56:02.1266583Z    Compiling serde_json v1.0.40
2019-08-13T23:56:06.2489768Z    Compiling tidy v0.1.0 (/checkout/src/tools/tidy)
2019-08-13T23:56:14.4673315Z     Finished release [optimized] target(s) in 1m 25s
2019-08-13T23:56:14.4747165Z tidy check
2019-08-13T23:56:14.5868535Z tidy error: /checkout/src/librustc_metadata/cstore_impl.rs:442: line longer than 100 chars
2019-08-13T23:56:16.3325619Z some tidy checks failed
2019-08-13T23:56:16.3326563Z 
2019-08-13T23:56:16.3326563Z 
2019-08-13T23:56:16.3327763Z command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-tools-bin/tidy" "/checkout/src" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo" "--no-vendor"
2019-08-13T23:56:16.3328461Z 
2019-08-13T23:56:16.3328673Z 
2019-08-13T23:56:16.3332873Z failed to run: /checkout/obj/build/bootstrap/debug/bootstrap test src/tools/tidy
2019-08-13T23:56:16.3333223Z Build completed unsuccessfully in 0:01:28
2019-08-13T23:56:16.3333223Z Build completed unsuccessfully in 0:01:28
2019-08-13T23:56:17.7762566Z ##[error]Bash exited with code '1'.
2019-08-13T23:56:17.7794105Z ##[section]Starting: Checkout
2019-08-13T23:56:17.7795846Z ==============================================================================
2019-08-13T23:56:17.7795920Z Task         : Get sources
2019-08-13T23:56:17.7795962Z Description  : Get sources from a repository. Supports Git, TfsVC, and SVN repositories.

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

Aaron1011 · 2019-08-14T01:26:12Z

@eddyb: I've made the changes you requested.

Aaron1011 · 2019-08-16T23:53:16Z

@petrochenkov: Squashed

petrochenkov · 2019-08-17T00:01:57Z

@bors r=eddyb,petrochenkov

bors · 2019-08-17T00:01:59Z

📌 Commit 73d7719 has been approved by eddyb,petrochenkov

…r=eddyb,petrochenkov Serialize additional data for procedural macros Split off from rust-lang#62855 This PR serializes the declaration `Span` and attributes for all procedural macros. This allows Rustdoc to properly render doc comments and source links when performing inlinig procedural macros across crates

Centril · 2019-08-17T12:05:29Z

Failed in #63653 (comment), @bors r-

(Known by process of elimination as #63655 excluded that PR and it is working.)

Split off from rust-lang#62855 This PR deerializes the declaration `Span` and attributes for all procedural macros from their underlying function definitions. This allows Rustdoc to properly render doc comments and source links when inlining procedural macros across crates

Aaron1011 · 2019-08-17T17:15:20Z

@petrochenkov: I had accidentally removed this check. The build should now pass.

petrochenkov · 2019-08-17T19:50:07Z

@bors r=eddyb,petrochenkov

bors · 2019-08-17T19:50:08Z

📌 Commit 64f867a has been approved by eddyb,petrochenkov

bors · 2019-08-18T08:15:46Z

⌛ Testing commit 64f867a with merge 71e2882...

…rochenkov Serialize additional data for procedural macros Split off from #62855 This PR serializes the declaration `Span` and attributes for all procedural macros. This allows Rustdoc to properly render doc comments and source links when performing inlinig procedural macros across crates

eddyb · 2019-08-18T09:05:02Z

@petrochenkov in #63269 (comment):

One big remaining question: why do we need the static _DECLS at all?

I've ranted about this before but I'm not sure if in this thread or on Discord.
To summarize, what we can do is maybe have one static proc_macro::bridge::client::Client per function (or even one static per crate plus the functions themselves), but not get rid of them completely.

If you look at what that Client contains, it's not just the proc macro fn pointer (which, yes, we'd be able to get via gensym), which would go into the f: F field, but also two other fn pointers, which need to come from the proc_macro library that the proc macro crate was linked against - so in a sense, the Client type encapsulates the ABI of a proc macro expander function.

rust/src/libproc_macro/bridge/client.rs

Lines 325 to 448 in ef1ecbe

    
           /// A client-side "global object" (usually a function pointer), 
        
           /// which may be using a different `proc_macro` from the one 
        
           /// used by the server, but can be interacted with compatibly. 
        
           /// 
        
           /// N.B., `F` must have FFI-friendly memory layout (e.g., a pointer). 
        
           /// The call ABI of function pointers used for `F` doesn't 
        
           /// need to match between server and client, since it's only 
        
           /// passed between them and (eventually) called by the client. 
        
           #[repr(C)] 
        
           #[derive(Copy, Clone)] 
        
           pub struct Client<F> { 
        
               pub(super) get_handle_counters: extern "C" fn() -> &'static HandleCounters, 
        
               pub(super) run: extern "C" fn(Bridge<'_>, F) -> Buffer<u8>, 
        
               pub(super) f: F, 
        
           } 
        
           // FIXME(#53451) public to work around `Cannot create local mono-item` ICE, 
        
           // affecting not only the function itself, but also the `BridgeState` `thread_local!`. 
        
           pub extern "C" fn __run_expand1( 
        
               mut bridge: Bridge<'_>, 
        
               f: fn(crate::TokenStream) -> crate::TokenStream, 
        
           ) -> Buffer<u8> { 
        
               // The initial `cached_buffer` contains the input. 
        
               let mut b = bridge.cached_buffer.take(); 
        
               panic::catch_unwind(panic::AssertUnwindSafe(|| { 
        
                   bridge.enter(|| { 
        
                       let reader = &mut &b[..]; 
        
                       let input = TokenStream::decode(reader, &mut ()); 
        
                       // Put the `cached_buffer` back in the `Bridge`, for requests. 
        
                       Bridge::with(|bridge| bridge.cached_buffer = b.take()); 
        
                       let output = f(crate::TokenStream(input)).0; 
        
                       // Take the `cached_buffer` back out, for the output value. 
        
                       b = Bridge::with(|bridge| bridge.cached_buffer.take()); 
        
                       // HACK(eddyb) Separate encoding a success value (`Ok(output)`) 
        
                       // from encoding a panic (`Err(e: PanicMessage)`) to avoid 
        
                       // having handles outside the `bridge.enter(|| ...)` scope, and 
        
                       // to catch panics that could happen while encoding the success. 
        
                       // 
        
                       // Note that panics should be impossible beyond this point, but 
        
                       // this is defensively trying to avoid any accidental panicking 
        
                       // reaching the `extern "C"` (which should `abort` but may not 
        
                       // at the moment, so this is also potentially preventing UB). 
        
                       b.clear(); 
        
                       Ok::<_, ()>(output).encode(&mut b, &mut ()); 
        
                   }) 
        
               })) 
        
               .map_err(PanicMessage::from) 
        
               .unwrap_or_else(|e| { 
        
                   b.clear(); 
        
                   Err::<(), _>(e).encode(&mut b, &mut ()); 
        
               }); 
        
               b 
        
           } 
        
           impl Client<fn(crate::TokenStream) -> crate::TokenStream> { 
        
               pub const fn expand1(f: fn(crate::TokenStream) -> crate::TokenStream) -> Self { 
        
                   Client { 
        
                       get_handle_counters: HandleCounters::get, 
        
                       run: __run_expand1, 
        
                       f, 
        
                   } 
        
               } 
        
           } 
        
           // FIXME(#53451) public to work around `Cannot create local mono-item` ICE, 
        
           // affecting not only the function itself, but also the `BridgeState` `thread_local!`. 
        
           pub extern "C" fn __run_expand2( 
        
               mut bridge: Bridge<'_>, 
        
               f: fn(crate::TokenStream, crate::TokenStream) -> crate::TokenStream, 
        
           ) -> Buffer<u8> { 
        
               // The initial `cached_buffer` contains the input. 
        
               let mut b = bridge.cached_buffer.take(); 
        
               panic::catch_unwind(panic::AssertUnwindSafe(|| { 
        
                   bridge.enter(|| { 
        
                       let reader = &mut &b[..]; 
        
                       let input = TokenStream::decode(reader, &mut ()); 
        
                       let input2 = TokenStream::decode(reader, &mut ()); 
        
                       // Put the `cached_buffer` back in the `Bridge`, for requests. 
        
                       Bridge::with(|bridge| bridge.cached_buffer = b.take()); 
        
                       let output = f(crate::TokenStream(input), crate::TokenStream(input2)).0; 
        
                       // Take the `cached_buffer` back out, for the output value. 
        
                       b = Bridge::with(|bridge| bridge.cached_buffer.take()); 
        
                       // HACK(eddyb) Separate encoding a success value (`Ok(output)`) 
        
                       // from encoding a panic (`Err(e: PanicMessage)`) to avoid 
        
                       // having handles outside the `bridge.enter(|| ...)` scope, and 
        
                       // to catch panics that could happen while encoding the success. 
        
                       // 
        
                       // Note that panics should be impossible beyond this point, but 
        
                       // this is defensively trying to avoid any accidental panicking 
        
                       // reaching the `extern "C"` (which should `abort` but may not 
        
                       // at the moment, so this is also potentially preventing UB). 
        
                       b.clear(); 
        
                       Ok::<_, ()>(output).encode(&mut b, &mut ()); 
        
                   }) 
        
               })) 
        
               .map_err(PanicMessage::from) 
        
               .unwrap_or_else(|e| { 
        
                   b.clear(); 
        
                   Err::<(), _>(e).encode(&mut b, &mut ()); 
        
               }); 
        
               b 
        
           } 
        
           impl Client<fn(crate::TokenStream, crate::TokenStream) -> crate::TokenStream> { 
        
               pub const fn expand2( 
        
                   f: fn(crate::TokenStream, crate::TokenStream) -> crate::TokenStream 
        
               ) -> Self { 
        
                   Client { 
        
                       get_handle_counters: HandleCounters::get, 
        
                       run: __run_expand2, 
        
                       f, 
        
                   } 
        
               } 
        
           }

bors · 2019-08-18T11:53:32Z

☀️ Test successful - checks-azure
Approved by: eddyb,petrochenkov
Pushing 71e2882 to master...

Propagate spans and attributes from proc macro definitions Thanks to rust-lang#63269 we now have spans and attributes from proc macro definitions available in metadata. However, that PR didn't actually put them into use! This PR finishes that work. Attributes `rustc_macro_transparency`, `allow_internal_unstable`, `allow_internal_unsafe`, `local_inner_macros`, `rustc_builtin_macro`, `stable`, `unstable`, `rustc_deprecated`, `deprecated` now have effect when applied to proc macro definition functions. From those attributes only `deprecated` is both stable and supposed to be used in new code. (`#![staged_api]` still cannot be used in proc macro crates for unrelated reasons though.) `Span::def_site` from the proc macro API now returns the correct location of the proc macro definition. Also, I made a mistake in rust-lang#63269 (comment), loaded proc macros didn't actually use the resolver cache. This PR fixes the caching issue, now proc macros go through the `Resolver::macro_map` cache as well. (Also, the first commit turns `proc_macro::quote` into a regular built-in macro to reduce the number of places where `SyntaxExtension`s need to be manually created.)

rust-highfive assigned zackmdavis Aug 4, 2019

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 4, 2019

rust-highfive assigned eddyb and unassigned zackmdavis Aug 4, 2019

eddyb reviewed Aug 9, 2019

View reviewed changes

src/librustc/hir/lowering.rs Outdated Show resolved Hide resolved

rust-highfive assigned petrochenkov and unassigned eddyb Aug 9, 2019

petrochenkov assigned eddyb and unassigned petrochenkov Aug 9, 2019

eddyb reviewed Aug 10, 2019

View reviewed changes

src/librustc_metadata/schema.rs Outdated Show resolved Hide resolved

eddyb reviewed Aug 10, 2019

View reviewed changes

Aaron1011 force-pushed the feature/proc-macro-data branch from ab06344 to 1aed667 Compare August 13, 2019 00:38

totsteps mentioned this pull request Aug 13, 2019

Improve Rustdoc's handling of procedural macros #62855

Merged

petrochenkov added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Aug 16, 2019

Aaron1011 force-pushed the feature/proc-macro-data branch from 463147c to 73d7719 Compare August 16, 2019 23:53

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 Aug 17, 2019

Centril mentioned this pull request Aug 17, 2019

Rollup of 5 pull requests #63653

Closed

bors 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-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Aug 17, 2019

Aaron1011 force-pushed the feature/proc-macro-data branch from 73d7719 to 64f867a Compare August 17, 2019 17:14

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 Aug 17, 2019

bors added the merged-by-bors This PR was explicitly merged by bors. label Aug 18, 2019

bors merged commit 64f867a into rust-lang:master Aug 18, 2019

petrochenkov mentioned this pull request Aug 20, 2019

Propagate spans and attributes from proc macro definitions #63761

Merged

Serialize additional data for procedural macros #63269

Serialize additional data for procedural macros #63269

Conversation

Aaron1011 commented Aug 4, 2019

rust-highfive commented Aug 4, 2019

Aaron1011 commented Aug 4, 2019

eddyb commented Aug 9, 2019

petrochenkov commented Aug 9, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bors commented Aug 12, 2019

rust-highfive commented Aug 14, 2019

Aaron1011 commented Aug 14, 2019

Aaron1011 commented Aug 16, 2019

petrochenkov commented Aug 17, 2019

bors commented Aug 17, 2019

Centril commented Aug 17, 2019 • edited Loading

Aaron1011 commented Aug 17, 2019

petrochenkov commented Aug 17, 2019

bors commented Aug 17, 2019

bors commented Aug 18, 2019

eddyb commented Aug 18, 2019 • edited Loading

bors commented Aug 18, 2019

Centril commented Aug 17, 2019 •

edited

Loading

eddyb commented Aug 18, 2019 •

edited

Loading