Name resolve proc macros #2496

P-E-P · 2023-07-31T13:21:26Z

Requires #2470

CohenArthur

I'll review the PR in details once the Early name resolver will have been merged, it's a lot of changes at the moment (my fault)

gcc/rust/expand/rust-proc-macro.cc

gcc/rust/expand/rust-proc-macro.h

CohenArthur · 2023-08-21T14:16:14Z

gcc/rust/rust-session-manager.cc

+  auto extern_crate = s.first == nullptr
+			? Imports::ExternCrate (crate_name, s.second)
+			: Imports::ExternCrate (*s.first);
+  if (s.first != nullptr)
    {
-      rust_error_at (locus, "failed to load crate metadata");
-      return UNKNOWN_NODEID;
+      bool ok = extern_crate.load (locus);
+      if (!ok)
+	{
+	  rust_error_at (locus, "failed to load crate metadata");
+	  return UNKNOWN_NODEID;
+	}


@philberty I can't exactly remember how the imports work, does that seem okay to you?

What is bothering you in this part ?

nothing, just asking for Philip's opinion since he's the one who wrote the original code :D seems perfectly fine to me!

I think the logic is ok but i dont like the variable name s. It took me a few times to read, so s is like the proc macro crate you're trying to load. So i would say something like proc_macro_import_name or something.

gcc/rust/util/rust-hir-map.cc

gcc/rust/resolve/rust-toplevel-name-resolver-2.0.cc

philberty

LGTM just change that variable name.

philberty · 2023-09-04T08:58:24Z

gcc/rust/rust-session-manager.cc

+  auto extern_crate = s.first == nullptr
+			? Imports::ExternCrate (crate_name, s.second)
+			: Imports::ExternCrate (*s.first);
+  if (s.first != nullptr)
    {
-      rust_error_at (locus, "failed to load crate metadata");
-      return UNKNOWN_NODEID;
+      bool ok = extern_crate.load (locus);
+      if (!ok)
+	{
+	  rust_error_at (locus, "failed to load crate metadata");
+	  return UNKNOWN_NODEID;
+	}


I think the logic is ok but i dont like the variable name s. It took me a few times to read, so s is like the proc macro crate you're trying to load. So i would say something like proc_macro_import_name or something.

P-E-P · 2023-09-04T10:43:26Z

LGTM just change that variable name.
I think the logic is ok but i dont like the variable name s. It took me a few times to read, so s is like the proc macro crate you're trying to load. So i would say something like proc_macro_import_name or something.

Actually it is a bit different, the package opening function knows where to look for an extern crate. For now proc macros are simple share libraries, thus without metadata.

If the function finds a .so it tries to open it and load the proc macros, if it's a .rox (?) it opens it and return a stream. That's why we have a pair with two components: some proc macros or a stream.

I think these changes highlight another underlying issue: Package opening should probably be decoupled from package search. The search function should probably return a list of path found, package loading should occur later depending on the extension and magic bytes. But this should probably be done in another PR as this refactor is not related to procedural macros.

This function will be used in the multiple other places, therefore we should make it easily usable from there. gcc/rust/ChangeLog: * ast/rust-ast.cc (Attribute::is_derive): Add member function. * ast/rust-ast.h: Likewise. * expand/rust-expand-visitor.cc (is_derive): Remove old function. (ExpandVisitor::expand_inner_stmts): Update function call. (ExpandVisitor::visit_inner_using_attrs): Likewise. Signed-off-by: Pierre-Emmanuel Patry <pierre-emmanuel.patry@embecosm.com>

This function will be used outside of the expand visitor, making it easily accessible is therefore mandatory. gcc/rust/ChangeLog: * ast/rust-ast.cc (Attribute::get_traits_to_derive): Add function as member function. * ast/rust-ast.h: Add prototype. * expand/rust-expand-visitor.cc (get_traits_to_derive): Remove function. (ExpandVisitor::expand_inner_stmts): Update function call. Signed-off-by: Pierre-Emmanuel Patry <pierre-emmanuel.patry@embecosm.com>

Having copy and any other constructor stuff might lead to a breakage in the future where the node id differs due to a newly constructed SimplePath node. This change will allow us to assert the NodeId is from the ast and not any copy made in between. gcc/rust/ChangeLog: * ast/rust-ast.cc (Attribute::get_traits_to_derive): Change return type to a vector of references. * ast/rust-ast.h: Update constructor. * expand/rust-expand-visitor.cc (ExpandVisitor::expand_inner_stmts): Update function call. Signed-off-by: Pierre-Emmanuel Patry <pierre-emmanuel.patry@embecosm.com>

Member function is_derive was overly constrained, the attribute changes when we parse it's meta items and it no longer contains a tokenstream while staying a derive. gcc/rust/ChangeLog: * ast/rust-ast.cc (Attribute::is_derive): Remove tokenstream condition. Signed-off-by: Pierre-Emmanuel Patry <pierre-emmanuel.patry@embecosm.com>

Derive attributes should be parsed before attempting to retrieve any traits. This will convert the tokenstream to a list of path if this hasn't been done previously. gcc/rust/ChangeLog: * ast/rust-ast.cc (Attribute::get_traits_to_derive): Convert tokenstream to path list. Signed-off-by: Pierre-Emmanuel Patry <pierre-emmanuel.patry@embecosm.com>

Add a simple attribute visit function and override StructStruct & Function visit functions. gcc/rust/ChangeLog: * resolve/rust-early-name-resolver-2.0.cc (Early::visit_attributes): Add function to handle attributes. (Early::visit): Override visitor functions. * resolve/rust-early-name-resolver-2.0.h: Add prototype. Signed-off-by: Pierre-Emmanuel Patry <pierre-emmanuel.patry@embecosm.com>

When resolving proc macros it is convenient to store every macro directly in the extern crate. These class in the ast module provide a better abstraction over the raw ProcMacro::{CustomDerive, Bang, Attribute} structures provided by the proc_macro library. gcc/rust/ChangeLog: * ast/rust-ast.h (class BangProcMacro): Add new proc macro abstraction. (class AttributeProcMacro): Likewise. (class CustomDeriveProcMacro): Likewise. Signed-off-by: Pierre-Emmanuel Patry <pierre-emmanuel.patry@embecosm.com>

Add multiple setters for a crate object in order to add macro abstractions previously introduced. gcc/rust/ChangeLog: * ast/rust-ast.h (struct Crate): Add proc macro members. Signed-off-by: Pierre-Emmanuel Patry <pierre-emmanuel.patry@embecosm.com>

Add some getters on the ast crate in order to be able to retrieve a reference to a crate's proc macros. gcc/rust/ChangeLog: * ast/rust-ast.h: Add getters. Signed-off-by: Pierre-Emmanuel Patry <pierre-emmanuel.patry@embecosm.com>

We do not need to copy the whole vector we can simply take a reference instead. gcc/rust/ChangeLog: * resolve/rust-early-name-resolver-2.0.cc (Early::visit_attributes): Change argument to reference. * resolve/rust-early-name-resolver-2.0.h: Update function prototype. Signed-off-by: Pierre-Emmanuel Patry <pierre-emmanuel.patry@embecosm.com>

Add mechanism to discover proc macros in loaded extern crates. In the top level resolver. gcc/rust/ChangeLog: * resolve/rust-toplevel-name-resolver-2.0.cc (TopLevel::go): Visit crate's newly stored proc macros. * rust-session-manager.cc (Session::load_extern_crate): Store proc macros in the parsed crate instead of a local variable to achieve mappings. Signed-off-by: Pierre-Emmanuel Patry <pierre-emmanuel.patry@embecosm.com>

This commit moves the procedural macros loaded definition from outside the AST to the mappings. This means most getters/setters around the mappings had to be changed. This commit also introduces the top level visit of those mappings instead of visiting the Crate ast members. gcc/rust/ChangeLog: * ast/rust-ast.h (class BangProcMacro): Move class from here to rust-proc-macro.h. Also remove related functions. (class AttributeProcMacro): Likewise. (class CustomDeriveProcMacro): Likewise. (struct Crate): Remove proc macro vector members. * expand/rust-macro-expand.h (struct MacroExpander): Change the type to the newly created classes. * expand/rust-proc-macro.cc (BangProcMacro::BangProcMacro): Add constructor implementation. (AttributeProcMacro::AttributeProcMacro): Likewise. (CustomDeriveProcMacro::CustomDeriveProcMacro): Likewise. * expand/rust-proc-macro.h (class BangProcMacro): Move class to here. (class AttributeProcMacro): Likewise. (class CustomDeriveProcMacro): Likewise. * resolve/rust-toplevel-name-resolver-2.0.cc (TopLevel::go): Change top level visitor to check mappings instead * rust-session-manager.cc (Session::load_extern_crate): Add back macro collection to mappings. * util/rust-hir-map.cc (Mappings::insert_derive_proc_macros): Update getter signature with new types. (Mappings::insert_bang_proc_macros): Likewise. (Mappings::insert_attribute_proc_macros): Likewise. (Mappings::lookup_derive_proc_macros): Likewise. (Mappings::lookup_bang_proc_macros): Likewise. (Mappings::lookup_attribute_proc_macros): Likewise. (Mappings::insert_derive_proc_macro_def): Likewise. (Mappings::insert_bang_proc_macro_def): Likewise. (Mappings::insert_attribute_proc_macro_def): Likewise. (Mappings::lookup_derive_proc_macro_def): Likewise. (Mappings::lookup_bang_proc_macro_def): Likewise. (Mappings::lookup_attribute_proc_macro_def): Likewise. (Mappings::insert_derive_proc_macro_invocation): Likewise. (Mappings::lookup_derive_proc_macro_invocation): Likewise. (Mappings::insert_bang_proc_macro_invocation): Likewise. (Mappings::lookup_bang_proc_macro_invocation): Likewise. (Mappings::insert_attribute_proc_macro_invocation): Likewise. (Mappings::lookup_attribute_proc_macro_invocation): Likewise. * util/rust-hir-map.h: Update function prototypes as well as map types. Signed-off-by: Pierre-Emmanuel Patry <pierre-emmanuel.patry@embecosm.com>

This error was emitted when a valid proc macro crate was loaded. Proc macros do not contain any import data for now. gcc/rust/ChangeLog: * metadata/rust-imports.cc (Import::try_package_in_directory): Remove error when some macro are found even if no import data is available. Signed-off-by: Pierre-Emmanuel Patry <pierre-emmanuel.patry@embecosm.com>

Move extern crate resolving under the extern crate declaration instead of doing it under the crate's root as extern crates are not resolved by the top level resolver. gcc/rust/ChangeLog: * metadata/rust-extern-crate.cc (ExternCrate::ExternCrate): Update definition to allow Extern crate with no content (pure proc macros). (ExternCrate::ok): Panic on no content. (ExternCrate::load): Likewise. * metadata/rust-extern-crate.h: Update prototypes. * resolve/rust-toplevel-name-resolver-2.0.cc (TopLevel::go): Remove macro resolution. (TopLevel::visit): Likewise. * resolve/rust-toplevel-name-resolver-2.0.h: Add visit prototype for extern crate. * rust-session-manager.cc (Session::load_extern_crate): Adapt content depending on the loaded crate's content. * util/rust-hir-map.cc (Mappings::lookup_derive_proc_macros): Change return type to optional because it is way more convenient. (Mappings::lookup_bang_proc_macros): Likewise. (Mappings::lookup_attribute_proc_macros): Likewise. * util/rust-hir-map.h: Update function prototypes. Signed-off-by: Pierre-Emmanuel Patry <pierre-emmanuel.patry@embecosm.com>

Change the condition with rust_unreachable to an assertion. This will keep the code clean and concise. Some styling issues appeared during review, this commit make the code more readable. gcc/rust/ChangeLog: * resolve/rust-toplevel-name-resolver-2.0.cc (TopLevel::visit): Change to assertion. * util/rust-hir-map.cc (Mappings::lookup_derive_proc_macros): Add empty line. (Mappings::lookup_bang_proc_macros): Likewise. (Mappings::lookup_attribute_proc_macros): Likewise. Signed-off-by: Pierre-Emmanuel Patry <pierre-emmanuel.patry@embecosm.com>

Add a templated function to insert any of the three kind of proc macro into the resolver context. gcc/rust/ChangeLog: * expand/rust-proc-macro.h: Change get_trait_name to get_name in order to be coherent with the others proc macro type name convention. * resolve/rust-toplevel-name-resolver-2.0.cc (insert_macros): Add a templated funtion that inserts a proc macro into the context and emit an error on failure. (TopLevel::visit): Change from manual insertion to a function call. Signed-off-by: Pierre-Emmanuel Patry <pierre-emmanuel.patry@embecosm.com>

This part of the code is a bit tricky as it calls multiple functions with almost the same name and slightly different behaviors. It was even more with a meaningless variable name. gcc/rust/ChangeLog: * rust-session-manager.cc (Session::load_extern_crate): Change variable name, add temporaries and comments. Signed-off-by: Pierre-Emmanuel Patry <pierre-emmanuel.patry@embecosm.com>

P-E-P added the enhancement label Jul 31, 2023

P-E-P added this to the Procedural Macros 1 milestone Jul 31, 2023

P-E-P force-pushed the name_resolve_proc_macros branch 4 times, most recently from b7bcb73 to 0d14331 Compare August 3, 2023 11:59

P-E-P requested a review from CohenArthur August 3, 2023 12:03

P-E-P mentioned this pull request Aug 3, 2023

ICE - Copy trait was implemented on Drop implementation type - [E0184] #2510

Open

CohenArthur force-pushed the name_resolve_proc_macros branch from 0d14331 to ac44e3c Compare August 3, 2023 14:58

CohenArthur reviewed Aug 3, 2023

View reviewed changes

gcc/rust/expand/rust-proc-macro.cc Show resolved Hide resolved

gcc/rust/expand/rust-proc-macro.h Outdated Show resolved Hide resolved

P-E-P force-pushed the name_resolve_proc_macros branch 2 times, most recently from 90cda22 to c3e766e Compare August 4, 2023 10:10

P-E-P marked this pull request as ready for review August 4, 2023 10:13

P-E-P force-pushed the name_resolve_proc_macros branch 2 times, most recently from ee6e860 to 638e325 Compare August 7, 2023 11:05

P-E-P requested a review from CohenArthur August 21, 2023 08:24

CohenArthur reviewed Aug 21, 2023

View reviewed changes

P-E-P mentioned this pull request Aug 29, 2023

Retrieve proc macro definition #2587

Merged

CohenArthur assigned P-E-P Aug 29, 2023

philberty requested changes Sep 4, 2023

View reviewed changes

P-E-P requested a review from philberty September 4, 2023 10:23

CohenArthur modified the milestones: Procedural Macros 1, Procedural Macros 2 Sep 4, 2023

P-E-P added 6 commits September 4, 2023 15:39

P-E-P added 11 commits September 4, 2023 15:39

Add getters for different types of proc macros

a655843

Add some getters on the ast crate in order to be able to retrieve a reference to a crate's proc macros. gcc/rust/ChangeLog: * ast/rust-ast.h: Add getters. Signed-off-by: Pierre-Emmanuel Patry <pierre-emmanuel.patry@embecosm.com>

P-E-P force-pushed the name_resolve_proc_macros branch from 93dc214 to debea7b Compare September 4, 2023 13:44

P-E-P added this pull request to the merge queue Sep 5, 2023

Merged via the queue into Rust-GCC:master with commit 1588263 Sep 5, 2023
9 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Name resolve proc macros #2496

Name resolve proc macros #2496

P-E-P commented Jul 31, 2023 •

edited

Loading

CohenArthur left a comment

CohenArthur Aug 21, 2023

P-E-P Aug 28, 2023

CohenArthur Aug 29, 2023

philberty Sep 4, 2023

philberty left a comment

philberty Sep 4, 2023

P-E-P commented Sep 4, 2023

Name resolve proc macros #2496

Name resolve proc macros #2496

Conversation

P-E-P commented Jul 31, 2023 • edited Loading

CohenArthur left a comment

Choose a reason for hiding this comment

CohenArthur Aug 21, 2023

Choose a reason for hiding this comment

P-E-P Aug 28, 2023

Choose a reason for hiding this comment

CohenArthur Aug 29, 2023

Choose a reason for hiding this comment

philberty Sep 4, 2023

Choose a reason for hiding this comment

philberty left a comment

Choose a reason for hiding this comment

philberty Sep 4, 2023

Choose a reason for hiding this comment

P-E-P commented Sep 4, 2023

P-E-P commented Jul 31, 2023 •

edited

Loading