crytic / amarna

Amarna is a static-analyzer and linter for the Cairo programming language.
https://blog.trailofbits.com/2022/04/20/amarna-static-analysis-for-cairo-programs/
GNU Affero General Public License v3.0
149 stars 7 forks source link

Add a way how to disable a rule on granular basis #15

Open milancermak opened 2 years ago

milancermak commented 2 years ago

It would be awesome if there was a way how to disable a rule (or multiple at the same time) on a per line, per function and per file basis. Probably something like an inline comment as is common with other tools.

For example, if I'd want to disable arithmetic checks (the arithmetic-expr_add rule), I would put a # amarna-disable: arithmetic-expr_add either at the top of the file (to disable this check in the whole file), as the first comment in the function body (to disable it only in that function) or somewhere inside the function (to disable it on the next line).

Are you thinking about adding something like this?

fcasal commented 2 years ago

Hi @milancermak, thanks for the suggestion! This is certainly something we could easily add per file. Making it granular per line would work as well although it would be slightly harder to implement. To implement this, we would have to find all these comments (and their lines) and filter the potential results in the following line from the result set.

fcasal commented 2 years ago

Hi @milancermak PR https://github.com/crytic/amarna/pull/16/files adds a couple of interesting features: I've added whitelist, and exclude rule options for the commandline. I've also added per file comment rule disabling. Writing # amarna: disable=rule1,rule2 as the first line of a Cairo file and using amarna with the --disable-inline option will omit occurrences from rule1 and rule2 on the results. I also added the --show-rules option to print the names of all the rules.

milancermak commented 2 years ago

I'm guessing you mean PR #18 Looks great!

❤️ for the --show-rules option, that's very thoughtful and very handy.

So I would have to use --disable-inline for the # amarna: disable to be respected?

fcasal commented 2 years ago

Yes, correct! I decided to do it this way and not the other way around (requiring a flag to view all results) because I didn't want to change the default behavior of the tool. Let me know your opinion on this choice.

milancermak commented 2 years ago

It's confusing.

I think it mainly stems from the name. I understand "disable inline" as "do NOT take the inline comments into account" / " disregard the # amarna: disable declarations" which is the opposite of what it does.

Also, I would prefer not to have to use a flag to enable the inline comments, i.e. by default, they should be used unless requested otherwise. Comments in source file are more long-lasting, so to speak, as cmdline args - when adding the # amarna: disable declaration, my intention is for it to be primarily on, not off.

So actually, if the --disable-inline flag would do the opposite of what it currently does (that is, it would tell Amarna to ignore the # amarna: disable comments and in fact check the rules), it would solve both of my concerns.

pscott commented 1 year ago

Bumping this issue because having granularity (ideally on a file / namespace / function / line) would be very welcomed :)

I come from the Rust (🦀) world and I'm so used to #[allow(clippy::wrong_self_convention)]. I believe this is such a useful feature, and would LOVE to see it in Amarna! (and maybe more generally, in Cairo :p)