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

Serialize additional data for procedural macros #63269

Open
wants to merge 1 commit into
base: master
from

Conversation

@Aaron1011
Copy link
Contributor

commented Aug 4, 2019

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

This comment has been minimized.

Copy link
Collaborator

commented Aug 4, 2019

r? @zackmdavis

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

@Aaron1011

This comment has been minimized.

Copy link
Contributor Author

commented Aug 4, 2019

r? @eddyb

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

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

This comment has been minimized.

Copy link
Member

commented Aug 9, 2019

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

@petrochenkov

This comment has been minimized.

Copy link
Contributor

commented Aug 9, 2019

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

@petrochenkov petrochenkov assigned eddyb and unassigned petrochenkov Aug 9, 2019


/// 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 {

This comment has been minimized.

Copy link
@eddyb

eddyb Aug 10, 2019

Member

Maybe call this ProcMacroDef?

This comment has been minimized.

Copy link
@Aaron1011

Aaron1011 Aug 10, 2019

Author Contributor

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.

/// 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

This comment has been minimized.

Copy link
@eddyb

eddyb Aug 10, 2019

Member

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.

This comment has been minimized.

Copy link
@Aaron1011

Aaron1011 Aug 14, 2019

Author Contributor

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

/// 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>,

This comment has been minimized.

Copy link
@eddyb

eddyb Aug 10, 2019

Member

There's no need to move this.

Show resolved Hide resolved src/librustc_metadata/schema.rs Outdated
// 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.

This comment has been minimized.

Copy link
@eddyb

eddyb Aug 10, 2019

Member

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

@@ -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,

This comment has been minimized.

Copy link
@eddyb

eddyb Aug 10, 2019

Member

Heh, this should be load_proc_macros nowadays.

@@ -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| {

This comment has been minimized.

Copy link
@eddyb

eddyb Aug 10, 2019

Member

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

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

This comment has been minimized.

Copy link
@eddyb

eddyb Aug 10, 2019

Member

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

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

This comment has been minimized.

Copy link
@eddyb

eddyb Aug 10, 2019

Member

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

This comment has been minimized.

Copy link
@petrochenkov

petrochenkov Aug 10, 2019

Contributor

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.

This comment has been minimized.

Copy link
@eddyb

eddyb Aug 10, 2019

Member

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

This comment has been minimized.

Copy link
@Aaron1011

Aaron1011 Aug 10, 2019

Author Contributor

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?

This comment has been minimized.

Copy link
@eddyb

eddyb Aug 10, 2019

Member

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?

@@ -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());

This comment has been minimized.

Copy link
@eddyb

eddyb Aug 10, 2019

Member

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

This comment has been minimized.

Copy link
@petrochenkov

petrochenkov Aug 10, 2019

Contributor

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.

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

This comment has been minimized.

Copy link
@eddyb

eddyb Aug 10, 2019

Member

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

macros.len(), decls.len());
}

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

This comment has been minimized.

Copy link
@eddyb

eddyb Aug 10, 2019

Member

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

@bors

This comment has been minimized.

Copy link
Contributor

commented Aug 12, 2019

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

@Aaron1011 Aaron1011 force-pushed the Aaron1011:feature/proc-macro-data branch from ab06344 to 1aed667 Aug 13, 2019

@rust-highfive

This comment has been minimized.

Copy link
Collaborator

commented Aug 14, 2019

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

This comment has been minimized.

Copy link
Contributor Author

commented Aug 14, 2019

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

@Aaron1011

This comment has been minimized.

Copy link
Contributor Author

commented Aug 16, 2019

@petrochenkov: I've squashed the commits, and removed the extraneous formatting changes.

@Aaron1011

This comment has been minimized.

Copy link
Contributor Author

commented Aug 16, 2019

@eddyb: I'm not 100% sure why this caused in this proc macro to start getting emiited. However, the relevant code has a ton of Span-related logic, so I suspect that this is due to proper spans now being available for all proc macros.

@petrochenkov

This comment has been minimized.

Copy link
Contributor

commented Aug 16, 2019

Yeah, the "in this proc macro" change is due to non-dummy def-site span becoming available.

@@ -725,6 +725,7 @@ pub struct ModuleItems {
pub struct Crate {
pub module: Mod,
pub attrs: HirVec<Attribute>,
pub proc_macros: HirVec<DefId>,

This comment has been minimized.

Copy link
@petrochenkov

petrochenkov Aug 16, 2019

Contributor

Looks like the only use of this data is in fn encode_proc_macros, so it can be HirVec<DefIndex> and avoid the re-collection.

This comment has been minimized.

Copy link
@petrochenkov

petrochenkov Aug 16, 2019

Contributor

This suggestion should be obsoleted by #63269 (comment).

} else {
self.dlsym_proc_macros(dylib.clone().map(|p| p.0), &crate_root, span)
}
});

This comment has been minimized.

Copy link
@petrochenkov

petrochenkov Aug 16, 2019

Contributor

Nit: could you move this code to its old place (it's hard to see the difference otherwise).

@@ -472,6 +465,41 @@ impl<'a> CrateLoader<'a> {
}
}

fn dlsym_proc_macros(&self,

This comment has been minimized.

Copy link
@petrochenkov

petrochenkov Aug 16, 2019

Contributor

Nit: could you move this code to its old place (again, hard to see the difference).

/// but also intentionally separate. Plugins are likely always going to be
/// 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.

This comment has been minimized.

Copy link
@petrochenkov

petrochenkov Aug 16, 2019

Contributor

The doc comment is entirely outdated now, it's probably better removed.

@petrochenkov

This comment has been minimized.

Copy link
Contributor

commented Aug 16, 2019

Thanks!
This looks much better than the original version.

@petrochenkov

This comment has been minimized.

Copy link
Contributor

commented Aug 16, 2019

So, the NodeIds of proc macro functions are needed only during metadata encoding, so it's not strictly necessary to collect the proc macro NodeIds in the "proc macro harness" pass and keep them in AST and then in HIR.

It would be simpler to collect them on the spot rather than threading them through AST and HIR.
This doesn't even need a visitor, simple iteration through the root module items with is_proc_macro_attr attributes would be enough (anything not fitting into that scheme should be already reported as an error).

@petrochenkov

This comment has been minimized.

Copy link
Contributor

commented Aug 16, 2019

One big remaining question: why do we need the static _DECLS at all? Can't all that data be a part of metadata instead? (Basically moving the whole proc_macro_harness.rs pass into metadata encoding.)

The auxiliary stuff like macro names and the list of helper attributes certainly doesn't need statics.
The main problem is the function pointers.
I wonder if the fn pointer can be obtained directly from the fn item's DefId by dlsyming its mangled name?
Can mangling be done on the spot when decoding metadata?
If it cannot, perhaps the mangled name needs to be kept in metadata?
cc @eddyb

(To clarify, I don't suggest do do all that in this PR.)

@Aaron1011

This comment has been minimized.

Copy link
Contributor Author

commented Aug 16, 2019

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

@petrochenkov

This comment has been minimized.

Copy link
Contributor

commented Aug 16, 2019

Thanks again!
r=me after squashing

@Aaron1011 Aaron1011 force-pushed the Aaron1011:feature/proc-macro-data branch from 463147c to 73d7719 Aug 16, 2019

@Aaron1011

This comment has been minimized.

Copy link
Contributor Author

commented Aug 16, 2019

@petrochenkov: Squashed

@petrochenkov

This comment has been minimized.

Copy link
Contributor

commented Aug 17, 2019

@bors r=eddyb,petrochenkov

@bors

This comment has been minimized.

Copy link
Contributor

commented Aug 17, 2019

📌 Commit 73d7719 has been approved by eddyb,petrochenkov

Centril added a commit to Centril/rust that referenced this pull request Aug 17, 2019

Rollup merge of rust-lang#63269 - Aaron1011:feature/proc-macro-data, …
…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

bors added a commit that referenced this pull request Aug 17, 2019

Auto merge of #63653 - Centril:rollup-sdu26ew, r=Centril
Rollup of 5 pull requests

Successful merges:

 - #62737 (Override Cycle::try_fold)
 - #63269 (Serialize additional data for procedural macros)
 - #63505 (Hash the remapped sysroot instead of the original.)
 - #63559 (rustc_codegen_utils: account for 1-indexed anonymous lifetimes in v0 mangling.)
 - #63621 (Modify librustc_llvm to pass -DNDEBUG while compiling.)

Failed merges:

r? @ghost
@Centril

This comment has been minimized.

Copy link
Member

commented Aug 17, 2019

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

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

Serialize additional data for procedural macros
Split off from #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 Aaron1011 force-pushed the Aaron1011:feature/proc-macro-data branch from 73d7719 to 64f867a Aug 17, 2019

@Aaron1011

This comment has been minimized.

Copy link
Contributor Author

commented Aug 17, 2019

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

@petrochenkov

This comment has been minimized.

Copy link
Contributor

commented Aug 17, 2019

@bors r=eddyb,petrochenkov

@bors

This comment has been minimized.

Copy link
Contributor

commented Aug 17, 2019

📌 Commit 64f867a has been approved by eddyb,petrochenkov

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants
You can’t perform that action at this time.