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 a way how to disable a rule on granular basis #15

Open
milancermak opened this issue Jun 2, 2022 · 6 comments
Open

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

milancermak opened this issue Jun 2, 2022 · 6 comments

Comments

@milancermak
Copy link

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
Copy link
Collaborator

fcasal commented Jun 2, 2022

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
Copy link
Collaborator

fcasal commented Jun 9, 2022

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
Copy link
Author

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
Copy link
Collaborator

fcasal commented Jun 9, 2022

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
Copy link
Author

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
Copy link

pscott commented Jul 26, 2022

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)

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

No branches or pull requests

3 participants