Skip to content

Conversation

yijunyu
Copy link

@yijunyu yijunyu commented Nov 9, 2021

These patches are made by Fanchangchun (F001) at Huawei to implement the basic features of AOP for Rust, as described here: https://trusted-programming.github.io/articles/work-in-progress/Language/AOP.html.

Please review it and see if any further changes are needed to include it for Rust compiler.

Best regards,
Yijun

@rust-highfive
Copy link
Contributor

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @michaelwoerister (or someone else) soon.

Please see the contribution instructions for more information.

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Nov 9, 2021
@notriddle
Copy link
Contributor

notriddle commented Nov 9, 2021

Wow, this seems pretty useful!

However, because Rust can’t really remove language features once they’re shipped in the stable language, it’s important that all the details are perfected before then. The first step of this is to open an RFC. Most of your RFC can probably be copied from your blog post.

Once that’s done, it can be tried out as a nightly-only feature, then stabilized.

This is described in more detail near the end of The Book.

@halohsu
Copy link

halohsu commented Nov 11, 2021

Cool feature :)

@michaelwoerister
Copy link
Member

Thanks for the PR, @yijunyu!

Since this arguably is a user facing change to the language, I think we'll need to discuss the feature before merging an implementation. I'm nominating this for discussion in the compiler team triage meeting so we can figure out if this needs a full RFC or if a major change proposal is sufficient.

@michaelwoerister michaelwoerister added the I-compiler-nominated Nominated for discussion during a compiler team meeting. label Nov 11, 2021
@apiraino apiraino added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Nov 11, 2021
pub fn search_pointcuts<'tcx>(tcx: TyCtxt<'tcx>, desc: String, src_map: &SourceMap) -> Vec<Found> {
let pc = Pointcut::parse(&desc);
pc.validate();
println!("[AOP] pointcut accepted: {}, {:?}", desc, pc);
Copy link
Member

@bjorn3 bjorn3 Nov 12, 2021

Choose a reason for hiding this comment

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

This println will need to be removed before merging. Same for all other println.

Copy link
Member

Choose a reason for hiding this comment

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

You could use debug!() instead.

Copy link
Member

@bjorn3 bjorn3 left a comment

Choose a reason for hiding this comment

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

In the article you say the following

cargo aspect does the following:

  • read the contents of Aspect.toml file;
  • make a full copy of src folder into src-saved folder;
  • call cargo +AOP rustc -- -Z aop-inspect="call _.unwrap()" to find the code location where the unwrap() method was called. The results are stored as a RUST_ASPECT_OUTPUT.txt file in the output directory;
  • read the search results, modify the source code, insert this line println!("function unwrap is called"); at the end of the concern;
  • compile the project again using the default toolchain.

which doesn't seem like a very clean solution to me. To me it seems that rustc should be able to handle it on it's own. That is also the only way to make it work together with macros.

Ideally it also wouldn't run compilation twice. Would it be possible to insert the extra function calls while translating to MIR, or as transformation pass over MIR? That would also work for function calls that are due to operator overloading, which I think your current implementation skips.

}
if self.pc.is_enter_or_exit(fn_name) {
//
//
Copy link
Member

Choose a reason for hiding this comment

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

This needs to be implemented, right?

impl Pointcut {
fn parse(s: &str) -> Self {
let mut p = PointcutParser { tokens: lex(s), cur: 0};
//println!("[DEBUG] tokens: {:?}", p.tokens);
Copy link
Member

Choose a reason for hiding this comment

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

Leftover commented out code.

full_name.push_str("RUST_ASPECT_OUTPUT.txt");
out_file.push(full_name);
}
let mut f = std::fs::File::create(&out_file).expect(&format!("Create output file {:?} failed", &out_file));
Copy link
Member

Choose a reason for hiding this comment

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

This should probably be behind a -Zunpretty option.

@@ -0,0 +1,625 @@
use rustc_data_structures::fx::FxHashMap;
Copy link
Member

Choose a reason for hiding this comment

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

I don't think this logic should be implemented in rustc_driver. At the very least it should be pushed back to rustc_interface (otherwise rustdoc can't support it), but preferably it should be elsewhere.

@davidtwco
Copy link
Member

davidtwco commented Nov 22, 2021

A note from the author (who can't comment himself):

"bjorn3" left an important comment in the PR:

which doesn't seem like a very clean solution to me. To me it seems that rustc should be able to handle it on it's own. That is also the only way to make it work together with macros.

This is a really great and important question. And below is my reply:

IMHO, the AOP feature contains two parts: a) find out where is my concerns, b) modify the concerned code or IR provided at another place.

The reason I chose two phase compilation is as below:

  1. I want to search the concerns by type information. ( a hypothetical example: I want to find all the Option::unwrap function > calls but not Result::unwrap calls )
  2. The new code is rewritten elsewhere, when it is inserted into existing code, it may introduce invalid tokens, AST nodes or HIR.

It is really difficult to me to modify the HIR or MIR after type check.

I totally agree that it is enjoyable if we can avoid twice compilation. However, it looks like another kind of meta programming mechanism to me. I don't know how it is feasible.

So, there is an important design here: should we implement AOP only based on macro which lacks of type information, or introduce new mechanisms in compiler. If we want to add some kind of support in the compiler, how far we could go? My original idea is that, the compiler only provide a readonly interface, from which we can query the typecheck result.

I'm curious about which option would be better as below? I appreciate if you experts can provide me some guidance.

  1. No special support in compiler at all. We'd better find other ways to implement this feature.
    • base on macros
    • write a standalone tool as a pre-processor
  2. Compiler provide a readonly interface to query type information
  3. Compiler provide a complex mechanism for meta programming (not only query type information, but also modification of IR)

Thanks very much!

@davidtwco
Copy link
Member

Would it be possible to insert the extra function calls while translating to MIR, or as transformation pass over MIR? That would also work for function calls that are due to operator overloading, which I think your current implementation skips.

I think this could be a good way to approach implementing this. We could write a MIR pass which inserts calls at the pointcut locations. These would call compiler-generated functions with appropriate arguments (not familiar enough with AOP to know what these would need to be). None of these functions would do anything, and ideally we'd generate them as weak functions (or some other mechanism that someone more familiar with this suggests) so that the user can provide their own implementation (in the current crate, or through some cargo tool and build.rs or something like that).

@pnkfelix pnkfelix assigned pnkfelix and unassigned michaelwoerister Dec 2, 2021
@pnkfelix
Copy link
Contributor

pnkfelix commented Dec 6, 2021

Hi @yijunyu and @davidtwco !

First off: Really neat idea, in terms of an unexplored feature (or set of features) for Rust!

While its great to have a concrete prototype to look at, a change of this magnitude can't really be dropped in via a Pull Request to Rust without some amount of up-front agreement about the overall design.

The natural next question is: "up-front agreement with whom ?" Or at least, that was the natural question for me. Certainly the compiler team is a stake-holder here. But I think the language team may also want to weigh in on the design.

I think the best next step would be for you to draft an initiative "proposal", as described on the "language initiative process" here (process) and here (proposal). Your proposal should be written in a way such that it stands on its own; i.e. the reader should not need to have to look at this PR to understand the proposal (though you can feel free to link to Pull Request as a useful reference for people who want to dig into the details of one proposed implementation).

(It is possible that the language team will discuss the matter and decide that this proposal is more well-suited as something that is entirely under the domain of the compiler itself, in which case we would take your proposal and turn it into a compiler team major change proposal (MCP). But, I think it will be easier for everyone if you first start off assuming that this should be a language initiative, rather than merely a compiler MCP.)

In any case, I'm going to close this PR, as its premature for us to attempt to land this change without first going through the initiative process.

@pnkfelix pnkfelix closed this Dec 6, 2021
@yijunyu
Copy link
Author

yijunyu commented Dec 6, 2021

Hi, @pnkfelix , yes I learnt from @davidtwco that we shall go through the MCP process before considering the PR and that will be our next step. Yes, do close the PR, thank you !

@apiraino apiraino removed I-compiler-nominated Nominated for discussion during a compiler team meeting. S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants