-
Notifications
You must be signed in to change notification settings - Fork 161
feat(jans-cedarling): add custom linter for inefficient string concatenation #13164
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
Merged
Merged
Changes from 4 commits
Commits
Show all changes
17 commits
Select commit
Hold shift + click to select a range
f4eb2f5
feat(jans-cedarling): add custom linter for inefficient string concat…
dagregi 5fe54b8
feat(jans-cedarling): update warning messages and provide suggestions
dagregi b341ee3
ci(jans-cedarling): update CI to include custom lints
dagregi 12d7cae
chore(jans-cedarling): add missing license header
dagregi 0a8cf7f
chore(jans-cedarling): update test file for the lint
dagregi 14df962
ci(jans-cedarling): pin cargo-dylint and dylint-link version to 5.0.0
dagregi e4b22b5
fix(jans-cedarling): update lints to check for qualified paths as well
dagregi 3a8bb00
fix(jans-cedarling): improve custom lint to use actual type snippet
dagregi d713e88
chore(jans-cedarling): fix minor typo in comment
dagregi d0dc044
ci(jans-cedarling): update custom lint action to use `cargo-binstall`
dagregi f3c8b64
feat(jans-cedarling): flag format macros passed as a variable
dagregi 8d58e5d
fix(jans-cedarling): fix existing EntityUid parsing issues
dagregi af016f3
ci(jans-cedarling): fix invalid workflow file
dagregi a41c6ba
chore(jans-cedarling): pin cargo-binstall to 1.17.4 in workflow
dagregi f08fba6
chore(jans-cedarling): add --no-confirm to cargo binstall commands in CI
dagregi 7460617
Merge branch 'main' into jans-cedarling-13010
moabu baf4b1f
ci: update .github/workflows/test-cedarling.yml
moabu File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,6 @@ | ||
| [target.'cfg(all())'] | ||
| rustflags = ["-C", "linker=dylint-link"] | ||
|
|
||
| # For Rust versions 1.74.0 and onward, the following alternative can be used | ||
| # (see https://github.com/rust-lang/cargo/pull/12535): | ||
| # linker = "dylint-link" |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1 @@ | ||
| /target |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,19 @@ | ||
| [package] | ||
| name = "custom_lints" | ||
| version = "0.1.0" | ||
| description = "Custom lints for the Cedarling" | ||
| edition = "2024" | ||
dagregi marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| publish = false | ||
|
|
||
| [lib] | ||
| crate-type = ["cdylib"] | ||
|
|
||
| [dependencies] | ||
| clippy_utils = { git = "https://github.com/rust-lang/rust-clippy", rev = "20ce69b9a63bcd2756cd906fe0964d1e901e042a" } | ||
| dylint_linting = "5.0.0" | ||
|
|
||
| [dev-dependencies] | ||
| dylint_testing = "5.0.0" | ||
|
|
||
| [package.metadata.rust-analyzer] | ||
| rustc_private = true | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,3 @@ | ||
| [toolchain] | ||
| channel = "nightly-2025-09-18" | ||
| components = ["llvm-tools-preview", "rustc-dev"] |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,147 @@ | ||
| // This software is available under the Apache-2.0 license. | ||
| // See https://www.apache.org/licenses/LICENSE-2.0.txt for full text. | ||
| // | ||
| // Copyright (c) 2024, Gluu, Inc. | ||
|
|
||
| #![feature(rustc_private)] | ||
coderabbitai[bot] marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| #![warn(unused_extern_crates)] | ||
|
|
||
| extern crate rustc_ast; | ||
| extern crate rustc_errors; | ||
| extern crate rustc_hir; | ||
| extern crate rustc_span; | ||
|
|
||
| use clippy_utils::{ | ||
| diagnostics::{span_lint_and_help, span_lint_and_sugg}, | ||
| sym, | ||
| }; | ||
| use rustc_ast::LitKind::Str; | ||
| use rustc_errors::Applicability; | ||
| use rustc_hir::{Expr, ExprKind, QPath, TyKind}; | ||
| use rustc_lint::{LateContext, LateLintPass}; | ||
| use rustc_span::{Symbol, source_map::Spanned}; | ||
|
|
||
| dylint_linting::declare_late_lint! { | ||
| /// ### What it does | ||
| /// Checks for `EntityUid::from_str(&format!(...))` pattern which is explicitly | ||
| /// warned against in the Cedar documentation for performance reasons. | ||
| /// | ||
| /// ### Why is this bad? | ||
| /// The Cedar create documentation specifically warns that using `format!()` | ||
| /// with `EntityUid::from_str` is inefficient. Use string literals instead. | ||
| /// | ||
coderabbitai[bot] marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| /// ### Example | ||
| /// | ||
| /// ```rust | ||
| /// EntityUid::from_str(&format!("{}::\"{}\"", entity_type, id)) | ||
| /// ``` | ||
| /// | ||
| /// Use instead: | ||
| /// | ||
| /// ```rust | ||
| /// let entity_type_name = EntityTypeName::from_str(entity_type).unwrap(); | ||
| /// let entity_id = EntityId::from_str(id).unwrap_or_else(|e| match e {}); | ||
| /// | ||
| /// EntityUid::from_type_name_and_id(entity_type_name, entity_id) | ||
| /// ``` | ||
dagregi marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| pub BAD_STRING_CONCATENATION, | ||
| Warn, | ||
| "using EntityUid::from_str with format!() is inefficient" | ||
| } | ||
|
|
||
| impl<'tcx> LateLintPass<'tcx> for BadStringConcatenation { | ||
| fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'tcx>) { | ||
| // This checks if the expression kind is a function call extracting | ||
| // the function itself and its arguments | ||
| if let ExprKind::Call(callee, [arg]) = &expr.kind | ||
olehbozhok marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| // Checks the function's kind is a path to definition e.g., Vec::new or EntityUid::from_str | ||
| && let ExprKind::Path(QPath::TypeRelative(ty, segment)) = &callee.kind | ||
| // Checks the function's type is a path to a type e.g., Vec<T> or EntityUid | ||
| && let TyKind::Path(QPath::Resolved(_, path)) = &ty.kind | ||
| // Checks for the type path segments identifier is `EntityUid` or not | ||
| // e.g., cedar_policy::EntityUid::from_str will have 2 path segments cedar_policy and EntityUid | ||
| && path.segments[0].ident.name.as_str() == "EntityUid" | ||
| // Checks if the function name is `from_str` or not | ||
| && segment.ident.name == sym::from_str | ||
coderabbitai[bot] marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| // Checks if the argument of `from_str` is a reference and extract the expression inside | ||
| && let ExprKind::AddrOf(_, _, exp) = arg.kind | ||
| // Checks if the argument inside `from_str` is a function call then extarct the | ||
| // argument of said function | ||
| && let ExprKind::Call(_, [format_call]) = exp.kind | ||
| && let ExprKind::Block(block, _) = format_call.kind | ||
| && is_format_macro(cx, block.expr) | ||
| && let Some(bexpr) = block.expr | ||
| && let ExprKind::Call(_, [format_arg]) = &bexpr.kind | ||
| { | ||
| if let Some(arg) = extract_string_literal(format_arg) { | ||
| span_lint_and_sugg( | ||
| cx, | ||
| BAD_STRING_CONCATENATION, | ||
| expr.span, | ||
| "using `EntityUid::from_str` with format! is inefficient", | ||
| "try this instead", | ||
| format!("EntityUid::from_str({arg:?})"), | ||
| Applicability::MachineApplicable, | ||
| ); | ||
coderabbitai[bot] marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| } else { | ||
| span_lint_and_help( | ||
| cx, | ||
| BAD_STRING_CONCATENATION, | ||
| expr.span, | ||
| "using `EntityUid::from_str` with format! is inefficient", | ||
| None, | ||
| "consider using `EntityUid::from_type_name_and_id` instead", | ||
| ); | ||
| } | ||
| } | ||
| } | ||
| } | ||
coderabbitai[bot] marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
| /// Checks if the given expression is a format macro or not | ||
| fn is_format_macro<'tcx>(cx: &LateContext<'tcx>, expr: Option<&Expr<'tcx>>) -> bool { | ||
| if let Some(expr) = expr | ||
| && let ExprKind::Call(callee, _) = &expr.kind | ||
| && let ExprKind::Path(QPath::Resolved(_, path)) = &callee.kind | ||
| { | ||
| if path.segments.len() >= 2 { | ||
| let last_two: Vec<_> = path | ||
| .segments | ||
| .iter() | ||
| .rev() | ||
| .take(2) | ||
| .map(|s| s.ident.name.as_str()) | ||
| .collect(); | ||
|
|
||
| if last_two[0] == "format" && last_two[1] == "fmt" { | ||
| return true; | ||
| } | ||
| } | ||
|
|
||
| // Fallback: check def_id | ||
| if let Some(def_id) = path.res.opt_def_id() { | ||
| let path_str = cx.tcx.def_path_str(def_id); | ||
| return matches!(path_str.as_str(), "alloc::fmt::format" | "std::fmt::format"); | ||
| } | ||
| } | ||
| false | ||
| } | ||
|
|
||
| // Checks if the argument of format macro is a string literal and extract it | ||
| fn extract_string_literal(expr: &Expr<'_>) -> Option<Symbol> { | ||
| if let ExprKind::Call(_, [arg]) = expr.kind | ||
| && let ExprKind::AddrOf(_, _, expr) = arg.kind | ||
| && let ExprKind::Array([element]) = expr.kind | ||
| && let ExprKind::Lit(Spanned { | ||
| node: Str(item, _), .. | ||
| }) = element.kind | ||
| { | ||
| return Some(item); | ||
| } | ||
|
|
||
| None | ||
| } | ||
|
|
||
| #[test] | ||
| fn ui() { | ||
| dylint_testing::ui_test(env!("CARGO_PKG_NAME"), "ui"); | ||
| } | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,33 @@ | ||
| #![allow(unused)] | ||
|
|
||
| fn main() { | ||
dagregi marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| // This should trigger the lint | ||
| let eid1 = EntityUid::from_str(&format!(r#"User::"alice""#)); | ||
|
|
||
| // This should trigger the lint | ||
| let user = "alice"; | ||
| let eid2 = EntityUid::from_str(&format!(r#"User::"{}""#, user)); | ||
|
|
||
| // This should trigger the lint - (even with std::format!) | ||
| let eid3 = EntityUid::from_str(&std::format!(r#"User::"bob""#)); | ||
|
|
||
| // This should NOT trigger | ||
| let eid4 = EntityUid::from_str(r#"User::"alice""#); | ||
|
|
||
| // This should NOT trigger | ||
| let literal = String::from(r#"User::"dave""#); | ||
| let eid5 = EntityUid::from_str(&literal); | ||
dagregi marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
| let entity_type_name = "Jans::User"; | ||
| let entity_id = "user123"; | ||
| let eid7 = EntityUid::from_str(&format!(r#"{entity_type_name}::"{entity_id}""#)); | ||
| } | ||
|
|
||
| // Mock EntityUid for testing (since we don't want to pull in the actual Cedar crate) | ||
| struct EntityUid; | ||
|
|
||
| impl EntityUid { | ||
| fn from_str(s: &str) -> Result<Self, String> { | ||
| Ok(EntityUid) | ||
| } | ||
| } | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,32 @@ | ||
| warning: using `EntityUid::from_str` with format! is inefficient | ||
| --> $DIR/main.rs:3:16 | ||
| | | ||
| LL | let eid1 = EntityUid::from_str(&format!(r#"User::"alice""#)); | ||
| | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this instead: `EntityUid::from_str("User::/"alice/"")` | ||
| | | ||
| = note: `#[warn(bad_string_concatenation)]` on by default | ||
|
|
||
| warning: using `EntityUid::from_str` with format! is inefficient | ||
| --> $DIR/main.rs:7:16 | ||
| | | ||
| LL | let eid2 = EntityUid::from_str(&format!(r#"User::"{}""#, user)); | ||
| | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ | ||
| | | ||
| = help: consider using `EntityUid::from_type_name_and_id` instead | ||
|
|
||
| warning: using `EntityUid::from_str` with format! is inefficient | ||
| --> $DIR/main.rs:10:16 | ||
| | | ||
| LL | let eid3 = EntityUid::from_str(&std::format!(r#"User::"bob""#)); | ||
| | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this instead: `EntityUid::from_str("User::/"bob/"")` | ||
|
|
||
| warning: using `EntityUid::from_str` with format! is inefficient | ||
| --> $DIR/main.rs:21:16 | ||
| | | ||
| LL | let eid7 = EntityUid::from_str(&format!(r#"{entity_type_name}::"{entity_id}""#)); | ||
| | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ | ||
| | | ||
| = help: consider using `EntityUid::from_type_name_and_id` instead | ||
|
|
||
| warning: 4 warnings emitted | ||
|
|
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.