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 semgrep #153

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Conversation

tcharding
Copy link
Member

@tcharding tcharding commented Oct 24, 2023

This is exploratory, adds a semgrep config file with a single rule. The idea is that we could use semgrep to enforce coding patterns. The downside is it has to be done with grep so it is not general enough to enforce "all error types implement std::error::Error" - well at least not trivially AFAICT.

The added rule shows how grep'able things can be enforced.

ref: https://semgrep.dev/orgs/rust_bitcoin/settings/access

@tcharding tcharding changed the title Introduce semgrep Introduce semgrep Oct 24, 2023
@tcharding
Copy link
Member Author

CC @dpc for giving me the idea.

- rust
message: Use inline attribute on `From` implementations.
patterns:
- pattern-regex: 'From<[A-Za-z]+> for [A-Za-z]+ \{$\n'
Copy link

Choose a reason for hiding this comment

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

suggestion: You don't have to use regex. semgrep supports Rust, but has some limitations around macros handling. This looks like something that should be possible to handle with normal (non-regex) syntatic matchers.

Copy link

@dpc dpc Oct 24, 2023

Choose a reason for hiding this comment

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

@tcharding https://semgrep.dev/playground/s/AdWQ (make sure to go to Advanced tab)

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh cool, that is way more powerful!

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 had a play and couldn't work out how to match on attributes (because the # is a comment in yaml). Will come back to it. Mentioning unless you know the solution off the top of your head.

Copy link

Choose a reason for hiding this comment

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

Oh. Yeah. Last time I tried the macros were the tricky one.

Copy link

@dpc dpc Oct 24, 2023

Choose a reason for hiding this comment

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

So attribute matching works https://semgrep.dev/playground/s/0z1r , but seems like not in impl blocks. :D

I've found some examples in https://github.com/returntocorp/semgrep/blob/develop/tests/patterns/rust/attribute_matching.sgrep

Copy link

Choose a reason for hiding this comment

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

I feel a bit disgusted by it, but that's the best I was able to get to work: https://semgrep.dev/playground/s/qDGR

Using focus-metavariable, #[$ARGS], $...ARGS, etc. should be possible to do it better in theory, but each seems to not work with Rust attributes for reasons unknown.

Copy link
Member Author

Choose a reason for hiding this comment

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

ok, thanks man. Will return to this.

@tcharding tcharding force-pushed the 10-24-semgrep branch 3 times, most recently from 8763df9 to a8d020d Compare January 25, 2024 05:53
@tcharding tcharding marked this pull request as ready for review January 25, 2024 05:53
@tcharding
Copy link
Member Author

The first patch can be removed to see this work in action.

patterns:
- pattern: |-
impl From<$F> for $T { fn from(...) -> $SELF { ... } }
- pattern-not-regex: "inline"
Copy link

@dpc dpc Jan 25, 2024

Choose a reason for hiding this comment

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

'\#\[inline\]' or something like that (to only match #[inline]) would be "tighter". Otherwise any inline, even in a body(?) would falsely pass.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah yes, nice. Will use, thanks.

@tcharding
Copy link
Member Author

FTR this PR is a proof of concept for introducing semgrep everywhere. Sorry @clarkmoody this repo is fast becoming the testbed for all the rust-bitcoin tooling madness we want to introduce :)

@dpc
Copy link

dpc commented Jan 27, 2024

Sorry @clarkmoody this repo is fast becoming the testbed for all the rust-bitcoin tooling madness we want to introduce :)

checks if there's flake.nix in the root dir 🤷

@tcharding
Copy link
Member Author

You haven't managed to convert me to nix yet :)

@dpc
Copy link

dpc commented Jan 29, 2024

yet! :D

As is customary we can inline this `From` implementation.
Introduce usage of semgrep by doing:

- Add a `semgrep` config file in `contrib/semgrep.yml`.
- Add a single rule to check that all `From` impls are inlined
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.

2 participants