amarna icon indicating copy to clipboard operation
amarna copied to clipboard

Add a way how to disable a rule on granular basis

Open milancermak opened this issue 2 years ago • 6 comments

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?

milancermak avatar Jun 02 '22 17:06 milancermak

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 avatar Jun 02 '22 17:06 fcasal

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.

fcasal avatar Jun 09 '22 15:06 fcasal

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?

milancermak avatar Jun 09 '22 15:06 milancermak

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.

fcasal avatar Jun 09 '22 15:06 fcasal

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.

milancermak avatar Jun 09 '22 16:06 milancermak

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)

pscott avatar Jul 26 '22 18:07 pscott