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

Add scanning for expanded macro calls #66

Merged
merged 17 commits into from
Jan 24, 2025

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.

src/bin/scan.rs Show resolved Hide resolved
data/test-packages/macro_test/src/main.rs Show resolved Hide resolved
data/test-packages/macro_test/src/main.rs Show resolved Hide resolved
src/audit_file.rs Outdated Show resolved Hide resolved
@@ -118,11 +118,11 @@ impl Resolver {
Ok(Resolver { host, vfs })
}

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?

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

src/scan_stats.rs Show resolved Hide resolved
@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