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

Introduce ra_proc_macro #3727

Merged
merged 4 commits into from
Mar 26, 2020
Merged

Conversation

edwin0cheng
Copy link
Member

This PR implemented:

  1. Reading dylib path of proc-macro crate from cargo check , similar to how OUTDIR is obtained.
  2. Added a new crate ra_proc_macro and implement the foot-work for reading result from external proc-macro expander.
  3. Added a struct ProcMacroClient , which will be responsible to the client side communication to the External process.

@edwin0cheng
Copy link
Member Author

cc @kiljacken for the dylib path reading part :)

crates/ra_proc_macro/src/lib.rs Outdated Show resolved Hide resolved
crates/ra_proc_macro/src/lib.rs Outdated Show resolved Hide resolved
Copy link
Member

@matklad matklad left a comment

Choose a reason for hiding this comment

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

I'd like to pay some attention to minimizing the crate deps here.

Ideally, the ra_db should not depend only any proc-macro specfic, and just export a trait. It should operate on tt::TokenStream, so a dep on ra_tt is required.

Additionally, Ideally proc_macros and mbe do not depend on each other, and use solely tt (which does not depen on ra_syntax). I think we should also make use that proc_macros dont (transitivellly) depend on ra_syntax.

Not sure if the above plan is feasible, but i'd love to try

crates/ra_db/src/input.rs Outdated Show resolved Hide resolved
crates/ra_proc_macro/Cargo.toml Outdated Show resolved Hide resolved
@@ -15,4 +15,5 @@ rustc-hash = "1.1.0"
ra_syntax = { path = "../ra_syntax" }
ra_cfg = { path = "../ra_cfg" }
ra_prof = { path = "../ra_prof" }
ra_proc_macro = { path = "../ra_proc_macro" }
Copy link
Member

Choose a reason for hiding this comment

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

Hm, I wonder if we can minimize the interface here...

Can we use something like this instead?

struct CrateData {
   proc_macros: FxHashMap<SmolStr, Arc<dyn Fn(tt::TokenTree) -> tt::TokenTree>>
}

Ie, define a really minimal proc macro interface directly in the ra_db crate, where the minimal interface is ideally an dyn Fn(), or maybe a named trait.

Copy link
Member Author

Choose a reason for hiding this comment

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

I changed to :

#[derive(Debug, Clone)]
pub struct ProcMacro {
    pub name: SmolStr,
    pub expander: Arc<dyn TokenExpander>,
}

impl Eq for ProcMacro {}
impl PartialEq for ProcMacro { ... }

#[derive(Debug, Clone, PartialEq, Eq)]
pub struct CrateData {
   ...
    pub proc_macro: Vec<ProcMacro>,
}

@matklad
Copy link
Member

matklad commented Mar 26, 2020

Also, thanks @edwin0cheng for splitting changes which are small diff-wise, but are large architecturally, into separate PRs! This is massively helpful!

@edwin0cheng
Copy link
Member Author

edwin0cheng commented Mar 26, 2020

Ideally, the ra_db should not depend only any proc-macro specfic, and just export a trait. It should operate on tt::TokenStream, so a dep on ra_tt is required.

In other words, if we export a trait in ra_db, Could ra_proc_macrodepend on ra_db ? Or do we define a wrapper in ra_project_model to bridge them ?

@matklad
Copy link
Member

matklad commented Mar 26, 2020

Good question! Yeah, I think it would also be good if ra_proc_macro didn't depned on ra_db...

I think bridging them in project-model might work, but, if the expansion trait is really minimal and only depends on tt, we can just put it in the ra_tt?

@edwin0cheng
Copy link
Member Author

edwin0cheng commented Mar 26, 2020

I think it is plausible, I could imagine it looks like following:

enum ExpandError {
...
}

trait TokenExpander: Debug + Send + Sync + RefUnwindSafe {
    fn exander(&self, subtree: &Subtree, attrs: Option<&Subtree>) -> Result<Subtree, ExpandError>;
}

The trait bound here is needed in CrateData, as It will be stored in salsa.

@kiljacken
Copy link
Contributor

Cargo interacting code looks reasonable to me 👍

Good job, this is some really cool work!

@edwin0cheng edwin0cheng force-pushed the introduce-ra_proc_macro branch from e9ca593 to db162df Compare March 26, 2020 16:46
@edwin0cheng
Copy link
Member Author

@matklad, I removed the dependency for mbe and refactoring a bit for salsa trait constraints. You could review it again. 👍

@matklad
Copy link
Member

matklad commented Mar 26, 2020

bors r+

@bors
Copy link
Contributor

bors bot commented Mar 26, 2020

@bors bors bot merged commit b1594f1 into rust-lang:master Mar 26, 2020
@edwin0cheng edwin0cheng deleted the introduce-ra_proc_macro branch March 26, 2020 17:25
lnicola pushed a commit to lnicola/rust-analyzer that referenced this pull request Jul 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants