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.

Sign up for GitHub

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

Interpolation prohibited outside @safe #79

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Interpolation prohibited outside @safe #79

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
97ed200
Interpolation prohibited outside @safe
bergel Sep 24, 2024
ad50f6c
Merge branch 'main' into axb-safe-logging
bergel Dec 3, 2024
bdd8c5d
Working...
bergel Dec 3, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 18 additions & 1 deletion src/linting/extended_checks.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@
!is_there_any_star_marker && return x == y

contains(x, "QQQ") && contains(y, "QQQ") &&
throw(BothCannotHaveStarException("Cannot both $x and $y have a star marker"))

Check failure on line 58 in src/linting/extended_checks.jl

View workflow job for this annotation

GitHub Actions / run_lint

if contains(x, "QQQ")
reg_exp = Regex(replace(x, "QQQ" => ".*"))
return !isnothing(match(reg_exp, y))
Expand Down Expand Up @@ -218,8 +218,8 @@
struct InterpolationInSafeLogRule <: RecommendationLintRule end
struct UseOfStaticThreads <: ViolationLintRule end
struct LogStatementsMustBeSafe <: FatalLintRule end

struct ShowErrorReporting <: RecommendationLintRule end
struct InterpolationOnlyInSafe <: ViolationLintRule end

const all_extended_rule_types = Ref{Any}(
vcat(
Expand Down Expand Up @@ -263,7 +263,7 @@

function generic_check(T::DataType, x::EXPR, template_code::String)
keyword = first(split(template_code, ['(', '{', ' ']))
return generic_check(T, x, template_code, "`$(keyword)` should be used with extreme caution.")

Check failure on line 266 in src/linting/extended_checks.jl

View workflow job for this annotation

GitHub Actions / run_lint 

Log messages must always be constructed via @safe("..") strings. If this interpolation is used in a log message, it should be a @safe-string. Please try this instead: @safe("...$(x)..."). If this is not being used for logging, you can lint-ignore this line.
end

function check_with_process(T::DataType, x::EXPR, markers::Dict{Symbol,String})
Expand Down Expand Up @@ -292,7 +292,7 @@

function check(t::InitializingWithFunctionRule, x::EXPR, markers::Dict{Symbol,String})
# If we are not in a const statement, then we exit this function.
haskey(markers, :const) || return

Check failure on line 295 in src/linting/extended_checks.jl

View workflow job for this annotation

GitHub Actions / run_lint 

Use `thaskey(dict,key)` instead of the Julia's `haskey`.
generic_check(t, x, "Threads.nthreads()", "`Threads.nthreads()` should not be used in a constant variable.")
generic_check(t, x, "is_local_deployment()", "`is_local_deployment()` should not be used in a constant variable.")
generic_check(t, x, "Deployment.is_local_deployment()", "`Deployment.is_local_deployment()` should not be used in a constant variable.")
Expand Down Expand Up @@ -348,11 +348,11 @@
check(t::PtrRule, x::EXPR) = generic_check(t, x, "Ptr{hole_variable}(hole_variable)")

function check(t::ArrayWithNoTypeRule, x::EXPR, markers::Dict{Symbol,String})
haskey(markers, :filename) || return

Check failure on line 351 in src/linting/extended_checks.jl

View workflow job for this annotation

GitHub Actions / run_lint 

Use `thaskey(dict,key)` instead of the Julia's `haskey`.
contains(markers[:filename], "src/Compiler") || return

haskey(markers, :macrocall) && markers[:macrocall] == "@match" && return

Check failure on line 354 in src/linting/extended_checks.jl

View workflow job for this annotation

GitHub Actions / run_lint 

Use `thaskey(dict,key)` instead of the Julia's `haskey`.
haskey(markers, :macrocall) && markers[:macrocall] == "@matchrule" && return

Check failure on line 355 in src/linting/extended_checks.jl

View workflow job for this annotation

GitHub Actions / run_lint 

Use `thaskey(dict,key)` instead of the Julia's `haskey`.

generic_check(t, x, "[]", "Need a specific Array type to be provided.")
end
Expand Down Expand Up @@ -392,7 +392,7 @@
end

function check(t::UnsafeRule, x::EXPR, markers::Dict{Symbol,String})
haskey(markers, :function) || return

Check failure on line 395 in src/linting/extended_checks.jl

View workflow job for this annotation

GitHub Actions / run_lint 

Use `thaskey(dict,key)` instead of the Julia's `haskey`.
isnothing(match(r"_unsafe_.*", markers[:function])) || return
isnothing(match(r"unsafe_.*", markers[:function])) || return

Expand Down Expand Up @@ -488,7 +488,7 @@
end

function check(t::RelPathAPIUsageRule, x::EXPR, markers::Dict{Symbol,String})
haskey(markers, :filename) || return

Check failure on line 491 in src/linting/extended_checks.jl

View workflow job for this annotation

GitHub Actions / run_lint 

Use `thaskey(dict,key)` instead of the Julia's `haskey`.
contains(markers[:filename], "src/Compiler/Front") || return

generic_check(t, x, "hole_variable::RelPath", "Usage of type `RelPath` is not allowed in this context.")
Expand All @@ -500,7 +500,7 @@
end

function check(t::NonFrontShapeAPIUsageRule, x::EXPR, markers::Dict{Symbol,String})
haskey(markers, :filename) || return

Check failure on line 503 in src/linting/extended_checks.jl

View workflow job for this annotation

GitHub Actions / run_lint 

Use `thaskey(dict,key)` instead of the Julia's `haskey`.
# In the front-end and in FFI, we are allowed to refer to `Shape`
contains(markers[:filename], "src/FrontCompiler") && return
contains(markers[:filename], "src/FFI") && return
Expand Down Expand Up @@ -535,7 +535,7 @@
is_safe_macro_call(y) =
y.head == :macrocall && y.args[1].head == :IDENTIFIER && y.args[1].val == "@safe"

is_safe_literal(x) = x.head in [:NOTHING,

Check failure on line 538 in src/linting/extended_checks.jl

View workflow job for this annotation

GitHub Actions / run_lint 

Use `tin(item,collection)` instead of the Julia's `in` or `∈`.
:INTEGER,
:FLOAT,
:TRUE,
Expand Down Expand Up @@ -592,3 +592,20 @@
# generic_check(t, x, "showerror(hole_variable_star)", msg)
generic_check(t, x, "showerror", msg)
end

function check(t::InterpolationOnlyInSafe, x::EXPR, markers::Dict{Symbol,String})
# Allow usage in Front benchmarks
contains(markers[:filename], "bench") && return
# Allow usages in tests
contains(markers[:filename], "test") && return

# We are in a macro call and the macro is @safe, we merely exit
haskey(markers, :macrocall) && markers[:macrocall] == "@safe" && return

# We are not in a @safe macro call, so we need to check for interpolation
msg = raw"""
Log messages must always be constructed via @safe("..") strings. If this interpolation is used in a log message, it should be a @safe-string. Please try this instead: @safe("...$(x)..."). If this is not being used for logging, you can lint-ignore this line.
"""

generic_check(t, x, "\"LINT_STRING_WITH_INTERPOLATION\"", msg)
end
151 changes: 83 additions & 68 deletions test/rai_rules_tests.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -697,6 +697,7 @@ end
# LINT_STRING_WITH_INTERPOLATION
@test !t("\"1 + 2\"", "\"LINT_STRING_WITH_INTERPOLATION\"")
@test t(raw"\"foo $x\"", "\"LINT_STRING_WITH_INTERPOLATION\"")
@test t(raw"\"foo $(x)\"", "\"LINT_STRING_WITH_INTERPOLATION\"")
@test t(raw"@warnv_safe_to_log 1 \"logged! $(a_variable)\"", "@warnv_safe_to_log hole_variable \"LINT_STRING_WITH_INTERPOLATION\"")
@test !t(raw"@warnv_safe_to_log 1 \"logged! (a_variable)\"", "@warnv_safe_to_log hole_variable \"LINT_STRING_WITH_INTERPOLATION\"")

Expand Down Expand Up @@ -1714,86 +1715,86 @@ end
@test !isnothing(match(expected, result))
end

@testset "Checking string interpolation" begin
# @testset "Checking string interpolation" begin

# ERRORS
source_with_error = raw"""
function f(conf)
@INFO "($conf.container.baseurl)"
end
"""
# # ERRORS
# source_with_error = raw"""
# function f(conf)
# @INFO "($conf.container.baseurl)"
# end
# """

source_with_error2 = raw"""
function f(conf)
@INFO "$conf.container.baseurl"
end
"""
# source_with_error2 = raw"""
# function f(conf)
# @INFO "$conf.container.baseurl"
# end
# """

source_with_error3 = raw"""
function f(conf)
@INFO "this string contains an error $conf.container.baseurl indeed!"
end
"""
# source_with_error3 = raw"""
# function f(conf)
# @INFO "this string contains an error $conf.container.baseurl indeed!"
# end
# """

source_with_error4 = raw"""
function f(conf)
@INFO "this string contains an error $conf .container.baseurl indeed!"
end
"""
# source_with_error4 = raw"""
# function f(conf)
# @INFO "this string contains an error $conf .container.baseurl indeed!"
# end
# """

source_with_error5 = raw"""
function f(engine_name)
@INFO "Issuing delete request for engine $engine_name..."
end
"""
# source_with_error5 = raw"""
# function f(engine_name)
# @INFO "Issuing delete request for engine $engine_name..."
# end
# """

source_with_error6 = raw"""
function f()
Source("model/$name", "model/$name", read(joinpath(@__DIR__, "models", "$name.rel"), String))
end
"""
# source_with_error6 = raw"""
# function f()
# Source("model/$name", "model/$name", read(joinpath(@__DIR__, "models", "$name.rel"), String))
# end
# """

source_with_error7 = raw"""
function f()
path = "$dir/$name.csv"
end
"""
# source_with_error7 = raw"""
# function f()
# path = "$dir/$name.csv"
# end
# """

@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_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: 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_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: 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"""
function f(conf)
@INFO "$(conf.container.baseurl)"
end
"""
# # NO ERROR
# source_without_error = raw"""
# function f(conf)
# @INFO "$(conf.container.baseurl)"
# end
# """

source_without_error2 = raw"""
function f(conf)
@INFO "this string contains an error $(conf.container.baseurl) indeed!"
end
"""
# source_without_error2 = raw"""
# function f(conf)
# @INFO "this string contains an error $(conf.container.baseurl) indeed!"
# end
# """

source_without_error3 = raw"""
function f()
_profile_filename = "profile-$(timestamp).pb.gz"
end
"""
# source_without_error3 = raw"""
# function f()
# _profile_filename = "profile-$(timestamp).pb.gz"
# end
# """

@test count_lint_errors(source_without_error) == 0
@test count_lint_errors(source_without_error2) == 0
@test count_lint_errors(source_without_error3) == 0
end
# @test count_lint_errors(source_without_error) == 0
# @test count_lint_errors(source_without_error2) == 0
# @test count_lint_errors(source_without_error3) == 0
# end

@testset "Arithmetic LintResult" begin
l1 = LintResult()
Expand Down Expand Up @@ -1922,6 +1923,20 @@ end
@test lint_test(source, "Line 6, column 5: Use `Threads.@threads :dynamic` instead of `Threads.@threads :static`.")
end

@testset "Interpolation prohibited outside @safe" begin
source = raw"""
function f()
print("...$(x)...")
@info @safe("...$(x)...")
@safe("...$(y)...")
end
"""
@test lint_test(source, raw"""Line 2, column 11: Log messages must always be constructed via @safe("..") strings. If this interpolation is used in a log message, it should be a @safe-string. Please try this instead: @safe("...$(x)..."). If this is not being used for logging, you can lint-ignore this line.""")

# There is only one lint error in source, which is in Line 2 as tested just above
@test count_lint_errors(source) == 1
end

@testset "Unsafe logging" begin
source = raw"""
function f()
Expand Down Expand Up @@ -1964,8 +1979,8 @@ end
)
end
"""
@test count_lint_errors(source) == 12
for line in 2:count_lint_errors(source) + 1
@test count_lint_errors(source) == 17
for line in 2:12 + 1
@test lint_test(source, "Line $(line), column 5: Unsafe logging statement. You must enclose variables and strings with `@safe(...)`.")
end
end
Expand Down
Loading