Skip to content

Commit

Permalink
Merge pull request #73 from RelationalAI/simplify-error-msg
Browse files Browse the repository at this point in the history
Simplifying messages
  • Loading branch information
bergel authored Aug 8, 2024
2 parents 2fe8eb0 + 476b83e commit faa2f93
Show file tree
Hide file tree
Showing 3 changed files with 57 additions and 56 deletions.
14 changes: 7 additions & 7 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -12,9 +12,9 @@ There are several ways to use StaticLint.jl. Here are a few usage examples:

```Julia
StaticLint.run_lint_on_text("function f() @async 1 + 2 end ");
---------- /var/folders/nz/1c4rst196ws_18tjtfl0yb980000gn/T/jl_m34buxG5sl.jl
Line 1, column 14: Macro `@spawn` should be used instead of `@async`. at offset 13 of /var/folders/nz/1c4rst196ws_18tjtfl0yb980000gn/T/jl_m34buxG5sl.jl
1 potential threat is found
---------- /var/folders/nz/1c4rst196ws_18tjtfl0yb980000gn/T/jl_1QHeJ2vm1U.jl
Line 1, column 14: Use `@spawn` instead of `@async`. /var/folders/nz/1c4rst196ws_18tjtfl0yb980000gn/T/jl_1QHeJ2vm1U.jl
1 potential threat is found: 1 violation and 0 recommendation
----------
```

Expand Down Expand Up @@ -63,7 +63,7 @@ Adding a new rule is easy. Only the file `src/linting/extended_checks.jl` has to
Here is an example of a `check`:

```Julia
check(::Async_Extention, x::EXPR) = generic_check(x, "@async hole_variable", "Macro `@spawn` should be used instead of `@async`.")
check(::Async_Extention, x::EXPR) = generic_check(x, "@async hole_variable", "Use `@spawn` instead of `@async`.")
```

The `generic_check` function takes as a second parameter the expression to be searched. The template string `"@async hole_variable"` means that the expression `x` will be matched against the template. The pseudo variable `hole_variable` matches everything. In case you want to match any arbitrary number of arguments, you can use `hole_variable_star` (look at the test for concrete examples).
Expand Down Expand Up @@ -122,20 +122,20 @@ the message that has to be ignored. Consider this example:

```Julia
function f()
# lint-disable-next-line: Macro `@spawn` should be used instead of `@async`.
# lint-disable-next-line: Use `@spawn` instead of `@async`.
@async 1 + 1
end
```

The instruction `@async 1 + 1` raises the error: Macro `@spawn` should be used instead of `@async`.
The instruction `@async 1 + 1` raises the error: Use `@spawn` instead of `@async`.
Providing this error msg to the comment `lint-disable-next-line:` disabled it.

Note that it is not necessary to have the full message. The beginning of it is enough. As
such, the code above is equivalent to:

```Julia
function f()
# lint-disable-next-line: Macro `@spawn` should be used
# lint-disable-next-line: Use `@spawn` instead
@async 1 + 1
end
```
Expand Down
15 changes: 8 additions & 7 deletions src/linting/extended_checks.jl
Original file line number Diff line number Diff line change
Expand Up @@ -208,7 +208,7 @@ const error_msgs = Dict{String, String}()
function reset_recommentation_dict!(d::Dict{String, Bool})
# Violations
d["Variable has been assigned but not used, if you want to keep this variable unused then prefix it with `_`."] = false
d[raw"Suspicious string interpolation, you may want to have $(a.b.c) instead of ($a.b.c)."] = false
d[raw"Use $(x) instead of $x"] = false
end

function initialize_recommentation_dict()
Expand Down Expand Up @@ -311,8 +311,9 @@ function check(t::Finalizer_Extention, x::EXPR)
end

function check(t::Async_Extention, x::EXPR)
generic_check(t, x, "@async hole_variable", "Macro `@spawn` should be used instead of `@async`.")
generic_check(t, x, "Threads.@async hole_variable", "Macro `@spawn` should be used instead of `@async`.")
msg = "Use `@spawn` instead of `@async`."
generic_check(t, x, "@async hole_variable", msg)
generic_check(t, x, "Threads.@async hole_variable", msg)
end

check(t::Ccall_Extention, x::EXPR) = generic_check(t, x, "ccall(hole_variable, hole_variable, hole_variable, hole_variable_star)", "`ccall` should be used with extreme caution.")
Expand Down Expand Up @@ -435,7 +436,7 @@ function check(t::Unsafe_Extension, x::EXPR, markers::Dict{Symbol,String})
end

function check(t::In_Extension, x::EXPR)
msg = "It is preferable to use `tin(item,collection)` instead of the Julia's `in` or `∈`."
msg = "Use `tin(item,collection)` instead of the Julia's `in` or `∈`."
generic_check(t, x, "in(hole_variable,hole_variable)", msg)
generic_check(t, x, "hole_variable in hole_variable", msg)

Expand All @@ -444,12 +445,12 @@ function check(t::In_Extension, x::EXPR)
end

function check(t::HasKey_Extension, x::EXPR)
msg = "It is preferable to use `thaskey(dict,key)` instead of the Julia's `haskey`."
msg = "Use `thaskey(dict,key)` instead of the Julia's `haskey`."
generic_check(t, x, "haskey(hole_variable,hole_variable)", msg)
end

function check(t::Equal_Extension, x::EXPR)
msg = "It is preferable to use `tequal(dict,key)` instead of the Julia's `equal`."
msg = "Use `tequal(dict,key)` instead of the Julia's `equal`."
generic_check(t, x, "equal(hole_variable,hole_variable)", msg)
end

Expand Down Expand Up @@ -502,7 +503,7 @@ function check(t::StringInterpolation_Extension, x::EXPR)
# We are interested only in string with interpolation, which begins with x.head==:string
x.head == :string || return

msg_error = raw"Suspicious string interpolation, use $(x) instead of $x."
msg_error = raw"Use $(x) instead of $x ([explanation](https://github.com/RelationalAI/RAIStyle?tab=readme-ov-file#string-interpolation))."
check_for_recommendation(typeof(t), msg_error)
# We iterate over the arguments of the CST String to check for STRING: (
# if we find one, this means the string was incorrectly interpolated
Expand Down
84 changes: 42 additions & 42 deletions test/rai_rules_tests.jl
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,7 @@ end
"""
@test lint_has_error_test(source)
@test lint_test(source,
"Line 2, column 5: Macro `@spawn` should be used instead of `@async`.")
"Line 2, column 5: Use `@spawn` instead of `@async`.")
end

@testset "@async 02" begin
Expand All @@ -71,7 +71,7 @@ end
"""
@test lint_has_error_test(source)
@test lint_test(source,
"Line 2, column 5: Macro `@spawn` should be used instead of `@async`.")
"Line 2, column 5: Use `@spawn` instead of `@async`.")
end

@testset "@cfunction" begin
Expand Down Expand Up @@ -521,19 +521,19 @@ end
"""
@test lint_has_error_test(source)
@test lint_test(source,
"Line 2, column 9: It is preferable to use `tin(item,collection)` instead of the Julia's `in`")
"Line 2, column 9: Use `tin(item,collection)` instead of the Julia's `in`")
@test lint_test(source,
"Line 3, column 9: It is preferable to use `tin(item,collection)` instead of the Julia's `in`")
"Line 3, column 9: Use `tin(item,collection)` instead of the Julia's `in`")
@test lint_test(source,
"Line 4, column 9: It is preferable to use `tequal(dict,key)` instead of the Julia's `equal`.")
"Line 4, column 9: Use `tequal(dict,key)` instead of the Julia's `equal`.")
@test lint_test(source,
"Line 5, column 9: It is preferable to use `thaskey(dict,key)` instead of the Julia's `haskey`.")
"Line 5, column 9: Use `thaskey(dict,key)` instead of the Julia's `haskey`.")
@test lint_test(source,
"Line 6, column 9: `uv_` functions should be used with extreme caution.")
@test lint_test(source,
"Line 7, column 9: It is preferable to use `tin(item,collection)` instead of the Julia's `in` or `∈`.")
"Line 7, column 9: Use `tin(item,collection)` instead of the Julia's `in` or `∈`.")
@test lint_test(source,
"Line 8, column 9: It is preferable to use `tin(item,collection)` instead of the Julia's `in` or `∈`.")
"Line 8, column 9: Use `tin(item,collection)` instead of the Julia's `in` or `∈`.")
end

@testset "Splatting" begin
Expand Down Expand Up @@ -952,8 +952,8 @@ end
result = String(take!(str))

expected = r"""
- \*\*Line 2, column 3:\*\* Macro `@spawn` should be used instead of `@async`\. \H+
- \*\*Line 2, column 3:\*\* Macro `@spawn` should be used instead of `@async`\. \H+
- \*\*Line 2, column 3:\*\* Use `@spawn` instead of `@async`\. \H+
- \*\*Line 2, column 3:\*\* Use `@spawn` instead of `@async`\. \H+
"""
result_matching = !isnothing(match(expected, result))
end
Expand Down Expand Up @@ -996,7 +996,7 @@ end
## Static code analyzer report
\*\*Output of the \[StaticLint\.jl code analyzer\]\(https://github\.com/RelationalAI/StaticLint\.jl\).+\*\*
Report creation time \(UTC\): \H+
- \*\*Line 2, column 3:\*\* Macro `@spawn` should be used instead of `@async`\. \H+
- \*\*Line 2, column 3:\*\* Use `@spawn` instead of `@async`\. \H+
- \*\*Line 2, column 25:\*\* Variable has been assigned but not used, if you want to keep this variable unused then prefix it with `_`. \H+
<details>
Expand Down Expand Up @@ -1051,8 +1051,8 @@ end
## Static code analyzer report
\*\*Output of the \[StaticLint\.jl code analyzer\]\(https://github\.com/RelationalAI/StaticLint\.jl\).+\*\*
Report creation time \(UTC\): \H+
- \*\*Line 2, column 3:\*\* Macro `@spawn` should be used instead of `@async`\. \H+
- \*\*Line 2, column 3:\*\* Macro `@spawn` should be used instead of `@async`\. \H+
- \*\*Line 2, column 3:\*\* Use `@spawn` instead of `@async`\. \H+
- \*\*Line 2, column 3:\*\* Use `@spawn` instead of `@async`\. \H+
<details>
<summary>For PR Reviewer \(2\)</summary>
Expand Down Expand Up @@ -1112,8 +1112,8 @@ end
## Static code analyzer report
\*\*Output of the \[StaticLint\.jl code analyzer\]\(https://github\.com/RelationalAI/StaticLint\.jl\).+\*\*
Report creation time \(UTC\): \H+
- \*\*Line 2, column 3:\*\* Macro `@spawn` should be used instead of `@async`\. \H+
- \*\*Line 2, column 3:\*\* Macro `@spawn` should be used instead of `@async`\. \H+
- \*\*Line 2, column 3:\*\* Use `@spawn` instead of `@async`\. \H+
- \*\*Line 2, column 3:\*\* Use `@spawn` instead of `@async`\. \H+
<details>
<summary>For PR Reviewer \(2\)</summary>
Expand Down Expand Up @@ -1307,7 +1307,7 @@ end

expected = " - **[Line 2, column 3:]" *
"(https://github.com/RelationalAI/raicode/blob/axb-foo-bar/$(corrected_file_name)" *
"#L2)** Macro `@spawn` should be used instead of `@async`."
"#L2)** Use `@spawn` instead of `@async`."
if !occursin(expected, result)
@info "didn't match" expected result
end
Expand Down Expand Up @@ -1512,27 +1512,27 @@ end
@testset "Locally disabling rule 01" begin
source = """
function f()
# lint-disable-next-line: Macro `@spawn` should be used instead of `@async`.
# lint-disable-next-line: Use `@spawn` instead of `@async`.
@async 1 + 1
end
"""
source_lines = split(source, "\n")
@test convert_offset_to_line_from_lines(30, source_lines) == (2, 17, nothing)
@test convert_offset_to_line_from_lines(95, source_lines) == (3, 1, "lint-disable-line: Macro `@spawn` should be used instead of `@async`.")
@test convert_offset_to_line_from_lines(91, source_lines) == (3, 14, "lint-disable-line: Use `@spawn` instead of `@async`.")

@test !lint_has_error_test(source)
end

@testset "Locally disabling rule 02" begin
source = """
function f()
# lint-disable-next-line: Macro `@spawn` should be used instead of `@async`.
# lint-disable-next-line: Use `@spawn` instead of `@async`.
@async unsafe_foo(12)
end
"""
source_lines = split(source, "\n")
@test convert_offset_to_line_from_lines(30, source_lines) == (2, 17, nothing)
@test convert_offset_to_line_from_lines(95, source_lines) == (3, 1, "lint-disable-line: Macro `@spawn` should be used instead of `@async`.")
@test convert_offset_to_line_from_lines(91, source_lines) == (3, 14, "lint-disable-line: Use `@spawn` instead of `@async`.")

@test lint_has_error_test(source)
@test lint_test(source,
Expand All @@ -1548,33 +1548,33 @@ end
"""
@test lint_has_error_test(source)
@test lint_test(source,
"Line 3, column 5: Macro `@spawn` should be used instead of `@async`.")
"Line 3, column 5: Use `@spawn` instead of `@async`.")
end

@testset "Locally disabling rule 04" begin
source = """
function f()
# lint-disable-next-line:Macro `@spawn` should be used instead of `@async`.
# lint-disable-next-line:Use `@spawn` instead of `@async`.
@async 1 + 1
end
"""
source_lines = split(source, "\n")
@test convert_offset_to_line_from_lines(30, source_lines) == (2, 17, nothing)
@test convert_offset_to_line_from_lines(95, source_lines) == (3, 2, "lint-disable-line: Macro `@spawn` should be used instead of `@async`.")
@test convert_offset_to_line_from_lines(91, source_lines) == (3, 15, "lint-disable-line: Use `@spawn` instead of `@async`.")

@test !lint_has_error_test(source)
end

@testset "Locally disabling rule 05" begin
source = """
function f()
# lint-disable-next-line: Macro `@spawn` should be used instead of `@async`.
# lint-disable-next-line: Use `@spawn` instead of `@async`.
@async 1 + 1
end
"""
source_lines = split(source, "\n")
@test convert_offset_to_line_from_lines(30, source_lines) == (2, 17, nothing)
@test convert_offset_to_line_from_lines(97, source_lines) == (3, 2, "lint-disable-line: Macro `@spawn` should be used instead of `@async`.")
@test convert_offset_to_line_from_lines(91, source_lines) == (3, 13, "lint-disable-line: Use `@spawn` instead of `@async`.")

@test !lint_has_error_test(source)
end
Expand Down Expand Up @@ -1694,10 +1694,10 @@ end
io=IOBuffer()
run_lint_on_text(source; io)

@test !rule_is_recommendation("Macro `@spawn` should be used instead of `@async`.")
@test !rule_is_recommendation("Macro `@spawn`")
@test rule_is_violation("Macro `@spawn` should be used instead of `@async`.")
@test rule_is_violation("Macro `@spawn`")
@test !rule_is_recommendation("Use `@spawn` instead of `@async`.")
@test !rule_is_recommendation("Use `@spawn`")
@test rule_is_violation("Use `@spawn` instead of `@async`.")
@test rule_is_violation("Use `@spawn`")
@test rule_is_recommendation("`@lock` should be used with extreme caution.")
@test rule_is_recommendation("`@lock` ")
@test !rule_is_violation("`@lock` should be used with extreme caution.")
Expand All @@ -1708,10 +1708,10 @@ end

@test StaticLint.retrieve_full_msg_from_prefix("`@lock` ") ==
"`@lock` should be used with extreme caution."
@test StaticLint.retrieve_full_msg_from_prefix("Macro `@spawn`") ==
"Macro `@spawn` should be used instead of `@async`."
@test StaticLint.retrieve_full_msg_from_prefix("Use `@spawn`") ==
"Use `@spawn` instead of `@async`."

msg = "Macro `@spawn` should be used instead of `@async`."
msg = "Use `@spawn` instead of `@async`."
@test StaticLint.retrieve_full_msg_from_prefix(msg) == msg
end

Expand All @@ -1738,7 +1738,7 @@ end
result = String(take!(io))
expected = r"""
---------- \H+
Line 2, column 5: Macro `@spawn` should be used instead of `@async`\. \H+
Line 2, column 5: Use `@spawn` instead of `@async`\. \H+
Line 5, column 5: `@lock` should be used with extreme caution\. \H+
2 potential threats are found: 1 violation and 1 recommendation
----------
Expand Down Expand Up @@ -1791,17 +1791,17 @@ end
end
"""

@test lint_test(source_with_error, raw"Line 2, column 11: Suspicious string interpolation, use $(x) instead of $x.")
@test lint_test(source_with_error2, raw"Line 2, column 11: Suspicious string interpolation, use $(x) instead of $x.")
@test lint_test(source_with_error3, raw"Line 2, column 11: Suspicious string interpolation, use $(x) instead of $x.")
@test lint_test(source_with_error4, raw"Line 2, column 11: Suspicious string interpolation, use $(x) instead of $x.")
@test lint_test(source_with_error5, raw"Line 2, column 11: Suspicious string interpolation, use $(x) instead of $x.")
@test lint_test(source_with_error, raw"Line 2, column 11: Use $(x) instead of $x ")
@test lint_test(source_with_error2, raw"Line 2, column 11: Use $(x) instead of $x ")
@test lint_test(source_with_error3, raw"Line 2, column 11: Use $(x) instead of $x ")
@test lint_test(source_with_error4, raw"Line 2, column 11: Use $(x) instead of $x ")
@test lint_test(source_with_error5, raw"Line 2, column 11: Use $(x) instead of $x ")

@test lint_test(source_with_error6, raw"Line 2, column 12: Suspicious string interpolation, use $(x) instead of $x.")
@test lint_test(source_with_error6, raw"Line 2, column 27: Suspicious string interpolation, use $(x) instead of $x.")
@test lint_test(source_with_error6, raw"Line 2, column 77: Suspicious string interpolation, use $(x) instead of $x.")
@test lint_test(source_with_error6, raw"Line 2, column 12: Use $(x) instead of $x ")
@test lint_test(source_with_error6, raw"Line 2, column 27: Use $(x) instead of $x ")
@test lint_test(source_with_error6, raw"Line 2, column 77: Use $(x) instead of $x ")

@test lint_test(source_with_error7, raw"Line 2, column 12: Suspicious string interpolation, use $(x) instead of $x.")
@test lint_test(source_with_error7, raw"Line 2, column 12: Use $(x) instead of $x ")

# NO ERROR
source_without_error = raw"""
Expand Down

0 comments on commit faa2f93

Please sign in to comment.