Skip to content

Conversation

@puguo
Copy link
Contributor

@puguo puguo commented Jan 6, 2025

Added macro expansion for macro calls. Allows scanning effects from expanded macro calls, but can't adjust code span correctly.

@cdstanford
Copy link
Collaborator

cdstanford commented Jan 8, 2025

Awesome, this looks great to have! I can do a quick review - just going to clone the code and take a look. +r from @lzoghbi would be welcome!

Copy link
Collaborator

@cdstanford cdstanford left a comment

Choose a reason for hiding this comment

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

Hi @puguo

nice work, looks good in general!
This will be an awesome feature to have, really excited to see it.
I left some comments and fixes in the review below.
A few overall suggestions:

  • I think we should separate out the macro expansion functionality to avoid scanner.rs getting too unwieldy. Can you move it out into a macro_expand.rs module or similar?

  • See the GitHub workflows checks above for some things to fix - please run cargo fmt. Also run cargo clippy and address all warnings (I haven't checked these myself yet, will check on a future review!)

  • Finally, can you run make test and make top10 and post how the output differs from before on test crates? I think getting this to work correctly might require enabling the expand_macros CLI option manually (from the way you have set it up). Basically what we want to see is how your changes affect the results on existing crates, how many new results do we see and do the new results look reasonable.

Let me know when you have addressed the above and happy to do another review! Thanks again for the work on this.

}

fn db(&self) -> &RootDatabase {
pub fn db(&self) -> &RootDatabase {
Copy link
Collaborator

Choose a reason for hiding this comment

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

+r @lzoghbi ? Is it fine to make these public?

@cdstanford
Copy link
Collaborator

cdstanford commented Jan 8, 2025

Looks like clippy and fmt failed, other checks look OK.

Oh one other thing:

Allows scanning effects from expanded macro calls, but can't adjust code span correctly.

Can you clarify what you mean by "can't adjust code span correctly"? What do you show as the code span output instead? This looks like something to discuss if needed (depending on how broken it is we may want to keep macro expand disabled for now, as you have it. But would love to see macro expansion enabled by default in the near future!).

I haven't looked into how these would affect things on the VSCode extension side so will let @lzoghbi comment on that! :)

Copy link
Collaborator

@cdstanford cdstanford left a comment

Choose a reason for hiding this comment

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

I did a cursory review. Looks good to me! Just waiting for all tests to complete, once you fix clippy + fmt + address all comments, we should be good to merge.

@cdstanford cdstanford merged commit 14dee2d into PLSysSec:main Jan 24, 2025
@cdstanford cdstanford mentioned this pull request Jan 24, 2025
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.

3 participants