Add assert_on_const lint
#388
Conversation
integraledelebesgue
force-pushed
the
feature/assertions_on_constants
branch
5 times, most recently
from
2f1794d to
8d75341
Compare
src/lints/assert_on_const.rs
Outdated
| let asserts = get_all_inline_macro_calls(db, item) | ||
| .into_iter() | ||
| .filter(|call| { | ||
| let Some(path) = call.path(db).segments(db).elements(db).last() else { |
There was a problem hiding this comment.
Please note that last() is not best choice when new macro syntax will be available, but it would also require us to check use statements if assert was not overwritten (idk if this is what compiler would do in this case)
There was a problem hiding this comment.
Perhaps we can use the resolver to check if the path refers to builtin assert macro?
src/lints/assert_on_const.rs
Outdated
| if let Some(ResolvedConcreteItem::Constant(const_id)) = resolved_item | ||
| && let ConstValue::Enum(variant, _) = db.lookup_intern_const_value(const_id) | ||
| && variant.concrete_enum_id == core_bool_enum(db) |
There was a problem hiding this comment.
nit: In linter, in such cases, we tend to use if_chain! macro to make those kind of conditions more readable. Please use it here
There was a problem hiding this comment.
DIsagree that it makes it more readable, if_chain was used because rust didn't support patterns in other place than strictly after if. Now we can get rid of if_chain (No formatting issues - main benefit, slightly faster compilation).
| fn assert_on_bool_literal() { | ||
| test_lint_diagnostics!(r" | ||
| fn foo() { | ||
| assert!(true); | ||
| } | ||
| ", @r" | ||
| Plugin diagnostic: Unnecessary assert on a const value detected. | ||
| --> lib.cairo:3:17 | ||
| assert!(true); | ||
| ^^^^ | ||
| ") | ||
| } | ||
|
|
||
| #[test] | ||
| fn assert_on_bool_const() { | ||
| test_lint_diagnostics!(r" | ||
| const C: bool = false; | ||
| fn foo() { | ||
| assert!(C); | ||
| } | ||
| ", @r" | ||
| Plugin diagnostic: Unnecessary assert on a const value detected. | ||
| --> lib.cairo:4:17 | ||
| assert!(C); | ||
| ^ | ||
| ") | ||
| } |
There was a problem hiding this comment.
nit: check out other tests. It's better to define the code snippets as const, as in the future, the same code might be used to test the fixer for the same lint.
integraledelebesgue
force-pushed
the
feature/assertions_on_constants
branch
3 times, most recently
from
2a4a305 to
db515f3
Compare
integraledelebesgue
force-pushed
the
feature/assertions_on_constants
branch
from
db515f3 to
7cd820a
Compare
Warning
This lint is not going to work until software-mansion/cairols#647 is solved
Closes #81