Skip to content

Commit 46d7b50

Browse files
dagregimoabucoderabbitai[bot]
authored
feat(jans-cedarling): add custom linter for inefficient string concatenation (#13164)
* feat(jans-cedarling): add custom linter for inefficient string concatenation The added custom linter checks and flags for the use of EntityUid::from_str with a format! macro. Signed-off-by: dagregi <dagmawi.m@proton.me> * feat(jans-cedarling): update warning messages and provide suggestions Signed-off-by: dagregi <dagmawi.m@proton.me> * ci(jans-cedarling): update CI to include custom lints Signed-off-by: dagregi <dagmawi.m@proton.me> * chore(jans-cedarling): add missing license header Signed-off-by: dagregi <dagmawi.m@proton.me> * chore(jans-cedarling): update test file for the lint Signed-off-by: dagregi <dagmawi.m@proton.me> * ci(jans-cedarling): pin cargo-dylint and dylint-link version to 5.0.0 Signed-off-by: dagregi <dagmawi.m@proton.me> * fix(jans-cedarling): update lints to check for qualified paths as well Signed-off-by: dagregi <dagmawi.m@proton.me> * fix(jans-cedarling): improve custom lint to use actual type snippet Signed-off-by: dagregi <dagmawi.m@proton.me> * chore(jans-cedarling): fix minor typo in comment Signed-off-by: dagregi <dagmawi.m@proton.me> * ci(jans-cedarling): update custom lint action to use `cargo-binstall` * Added a README for the custom lints Signed-off-by: dagregi <dagmawi.m@proton.me> * feat(jans-cedarling): flag format macros passed as a variable Signed-off-by: dagregi <dagmawi.m@proton.me> * fix(jans-cedarling): fix existing EntityUid parsing issues Signed-off-by: dagregi <dagmawi.m@proton.me> * ci(jans-cedarling): fix invalid workflow file Signed-off-by: dagregi <dagmawi.m@proton.me> * chore(jans-cedarling): pin cargo-binstall to 1.17.4 in workflow Signed-off-by: dagregi <dagmawi.m@proton.me> * chore(jans-cedarling): add --no-confirm to cargo binstall commands in CI Signed-off-by: dagregi <dagmawi.m@proton.me> * ci: update .github/workflows/test-cedarling.yml Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Signed-off-by: Mohammad Abudayyeh <47318409+moabu@users.noreply.github.com> --------- Signed-off-by: dagregi <dagmawi.m@proton.me> Signed-off-by: Mohammad Abudayyeh <47318409+moabu@users.noreply.github.com> Co-authored-by: Mohammad Abudayyeh <47318409+moabu@users.noreply.github.com> Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
1 parent f49625c commit 46d7b50

File tree

11 files changed

+391
-5
lines changed

11 files changed

+391
-5
lines changed

.github/workflows/test-cedarling.yml

Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -29,6 +29,33 @@ jobs:
2929
run: |
3030
rustup component add clippy
3131
cargo clippy --workspace --all-targets -- -D warnings
32+
custom_lints:
33+
runs-on: ubuntu-latest
34+
steps:
35+
- name: Harden Runner
36+
uses: step-security/harden-runner@5ef0c079ce82195b2a36a210272d6b661572d83e # v2.14.2
37+
with:
38+
egress-policy: audit
39+
- uses: actions/checkout@b4ffde65f46336ab88eb53be808477a3936bae11 # v4.1.1
40+
- name: Install Rust Nightly
41+
uses: dtolnay/rust-toolchain@1ff72ee08e3cb84d84adba594e0a297990fc1ed3
42+
with:
43+
toolchain: nightly-2025-09-18
44+
components: llvm-tools-preview, rustc-dev
45+
- uses: cargo-bins/cargo-binstall@v1.17.4
46+
- name: Install dylint
47+
run: |
48+
cargo binstall cargo-dylint --version 5.0.0 --locked --no-confirm
49+
cargo binstall dylint-link --version 5.0.0 --locked --no-confirm
50+
- name: Run Tests
51+
working-directory: jans-cedarling/custom-lints
52+
run: cargo test
53+
- name: Install Rust Stable (for workspace)
54+
uses: dtolnay/rust-toolchain@1ff72ee08e3cb84d84adba594e0a297990fc1ed3 # stable
55+
- name: Run custom lints on workspace
56+
working-directory: jans-cedarling
57+
run: |
58+
RUSTFLAGS="-Dwarnings" cargo dylint --all --workspace --path custom-lints
3259
rust_benchmarks:
3360
runs-on: ubuntu-latest
3461
steps:

jans-cedarling/Cargo.toml

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,7 @@
11
[workspace]
22
resolver = "2"
33
members = ["bindings/*", "cedarling", "http_utils", "sparkv", "test_utils"]
4-
exclude = ["bindings/cedarling-java"]
4+
exclude = ["bindings/cedarling-java", "custom-lints"]
55

66
[workspace.dependencies]
77
serde = { version = "1.0", features = ["derive"] }

jans-cedarling/cedarling/src/common/default_entities.rs

Lines changed: 7 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,8 @@
55

66
use base64::prelude::*;
77
use cedar_policy::Entity;
8+
use cedar_policy::EntityId;
9+
use cedar_policy::EntityTypeName;
810
use cedar_policy::EntityUid;
911
use cedar_policy::ExpressionConstructionError;
1012
use cedar_policy::RestrictedExpression;
@@ -401,16 +403,17 @@ fn parse_cedar_format<'a>(
401403
parent_obj.get("id").and_then(|v| v.as_str()),
402404
)
403405
{
404-
let parent_uid_str = format!("{parent_entity_type}::\"{id_v}\"");
405-
match EntityUid::from_str(&parent_uid_str) {
406-
Ok(parent_uid) => {
406+
let entity_id = EntityId::from_str(id_v).unwrap_or_else(|e| match e {});
407+
match EntityTypeName::from_str(parent_entity_type) {
408+
Ok(type_name) => {
409+
let parent_uid = EntityUid::from_type_name_and_id(type_name, entity_id);
407410
parents_set.insert(parent_uid);
408411
},
409412
Err(e) => {
410413
// log warn that we could not parse uid
411414
warns.push(DefaultEntityWarning::InvalidParentUid {
412415
entry_id: entry_id.to_string(),
413-
parent_uid_str: parent_uid_str.clone(),
416+
parent_uid_str: format!("{parent_entity_type}::\"{id_v}\""),
414417
error: e.to_string(),
415418
});
416419
},
Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,6 @@
1+
[target.'cfg(all())']
2+
rustflags = ["-C", "linker=dylint-link"]
3+
4+
# For Rust versions 1.74.0 and onward, the following alternative can be used
5+
# (see https://github.com/rust-lang/cargo/pull/12535):
6+
# linker = "dylint-link"
Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
/target
Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,19 @@
1+
[package]
2+
name = "custom_lints"
3+
version = "0.1.0"
4+
description = "Custom lints for the Cedarling"
5+
edition = "2024"
6+
publish = false
7+
8+
[lib]
9+
crate-type = ["cdylib"]
10+
11+
[dependencies]
12+
clippy_utils = { git = "https://github.com/rust-lang/rust-clippy", rev = "20ce69b9a63bcd2756cd906fe0964d1e901e042a" }
13+
dylint_linting = "5.0.0"
14+
15+
[dev-dependencies]
16+
dylint_testing = "5.0.0"
17+
18+
[package.metadata.rust-analyzer]
19+
rustc_private = true
Lines changed: 81 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,81 @@
1+
# Custom Lints for Cedarling
2+
3+
This directory contains custom Rust lints specifically designed for the Cedarling project to enforce best practices and catch common performance issues.
4+
5+
## Overview
6+
7+
This custom linter package detects inefficient usage patterns in Cedar policy code, specifically:
8+
9+
- **BAD_STRING_CONCATENATION**: Detects `EntityUid::from_str(&format!(...))` patterns which are explicitly warned against in Cedar documentation for performance reasons.
10+
11+
## Usage
12+
13+
### Running custom lints
14+
15+
```bash
16+
# Build the custom lint library
17+
cargo build
18+
19+
# Run the linter in project directory
20+
cargo dylint --all --path custom-lints
21+
```
22+
23+
## Development
24+
25+
### Creating New Custom Lints
26+
27+
1. **Add a new lint declaration** in `src/lib.rs`:
28+
29+
````rust
30+
dylint_linting::declare_late_lint! {
31+
/// ### What it does
32+
/// Brief description of what the lint checks for.
33+
///
34+
/// ### Why is this bad?
35+
/// Explain why this pattern should be avoided.
36+
///
37+
/// ### Example
38+
/// ```rust
39+
/// // Bad example
40+
/// ```
41+
///
42+
/// Use instead:
43+
///
44+
/// ```rust
45+
/// // Good example
46+
/// ```
47+
pub YOUR_LINT_NAME,
48+
Warn,
49+
"lint description"
50+
}
51+
````
52+
53+
2. **Implement the lint logic** in either `LateLintPass` or `EarlyLintPass` implementation
54+
55+
3. **Add UI tests** in the `ui/` directory:
56+
- Create test cases in `ui/<your_lint_name>.rs`
57+
- Update expected output in `ui/<your_lint_name>.stderr`
58+
59+
4. **Run tests** to verify your implementation:
60+
61+
```bash
62+
cargo test
63+
```
64+
65+
## Resources
66+
67+
Helpful resources for writing lints:
68+
69+
- [Dylint Documentation](https://github.com/trailofbits/dylint#quick-start)
70+
- [Rustc Dev Guide](https://rustc-dev-guide.rust-lang.org/)
71+
- [Adding a new lint](https://github.com/rust-lang/rust-clippy/blob/master/book/src/development/adding_lints.md)
72+
- [Common tools for writing lints](https://github.com/rust-lang/rust-clippy/blob/master/book/src/development/common_tools_writing_lints.md)
73+
- [Crate `rustc_hir`](https://doc.rust-lang.org/nightly/nightly-rustc/rustc_hir/index.html)
74+
- [Struct `rustc_lint::LateContext`](https://doc.rust-lang.org/nightly/nightly-rustc/rustc_lint/struct.LateContext.html)
75+
76+
### Key Concepts
77+
78+
- **Late Lint Pass**: Runs after type checking, allowing analysis of typed expressions
79+
- **AST vs HIR**: Abstract Syntax Tree vs High-Level Intermediate Representation
80+
- **Span**: Location information in source code
81+
- **Diagnostic**: Error/warning messages with suggestions
Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,3 @@
1+
[toolchain]
2+
channel = "nightly-2025-09-18"
3+
components = ["llvm-tools-preview", "rustc-dev"]
Lines changed: 153 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,153 @@
1+
// This software is available under the Apache-2.0 license.
2+
// See https://www.apache.org/licenses/LICENSE-2.0.txt for full text.
3+
//
4+
// Copyright (c) 2024, Gluu, Inc.
5+
6+
#![feature(rustc_private)]
7+
#![warn(unused_extern_crates)]
8+
9+
extern crate rustc_ast;
10+
extern crate rustc_errors;
11+
extern crate rustc_hir;
12+
extern crate rustc_span;
13+
14+
use clippy_utils::{
15+
diagnostics::{span_lint_and_help, span_lint_and_sugg, span_lint_and_then},
16+
source::snippet,
17+
sym,
18+
};
19+
use rustc_ast::LitKind::Str;
20+
use rustc_errors::Applicability;
21+
use rustc_hir::{Expr, ExprKind, Node, QPath, TyKind, def::Res::Local};
22+
use rustc_lint::{LateContext, LateLintPass};
23+
use rustc_span::{Symbol, source_map::Spanned};
24+
25+
dylint_linting::declare_late_lint! {
26+
/// ### What it does
27+
/// Checks for `EntityUid::from_str(&format!(...))` pattern which is explicitly
28+
/// warned against in the Cedar documentation for performance reasons.
29+
///
30+
/// ### Why is this bad?
31+
/// The Cedar crate documentation specifically warns that using `format!()`
32+
/// with `EntityUid::from_str` is inefficient. Use string literals instead.
33+
///
34+
/// ### Example
35+
///
36+
/// ```rust
37+
/// EntityUid::from_str(&format!("{}::\"{}\"", entity_type, id))
38+
/// ```
39+
///
40+
/// Use instead:
41+
///
42+
/// ```rust
43+
/// let entity_type_name = EntityTypeName::from_str(entity_type).unwrap();
44+
/// let entity_id = EntityId::from_str(id).unwrap_or_else(|e| match e {});
45+
///
46+
/// EntityUid::from_type_name_and_id(entity_type_name, entity_id)
47+
/// ```
48+
pub BAD_STRING_CONCATENATION,
49+
Warn,
50+
"using EntityUid::from_str with format!() is inefficient"
51+
}
52+
53+
impl<'tcx> LateLintPass<'tcx> for BadStringConcatenation {
54+
fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'tcx>) {
55+
// This checks if the expression kind is a function call extracting
56+
// the function itself and its arguments
57+
if let ExprKind::Call(callee, [arg]) = &expr.kind
58+
// Checks the function's kind is a path to definition e.g., Vec::new or EntityUid::from_str
59+
&& let ExprKind::Path(QPath::TypeRelative(ty, segment)) = &callee.kind
60+
// Checks the function's type is a path to a type e.g., Vec<T> or EntityUid
61+
&& let TyKind::Path(QPath::Resolved(_, path)) = &ty.kind
62+
// Checks for the type path segments identifier is `EntityUid` or not
63+
// e.g., cedar_policy::EntityUid::from_str will have 2 path segments cedar_policy and EntityUid
64+
&& path.segments.last().is_some_and(|s| s.ident.name.as_str().eq("EntityUid"))
65+
// Checks if the function name is `from_str` or not
66+
&& segment.ident.name == sym::from_str
67+
// Checks if the argument of `from_str` is a reference and extract the expression inside
68+
&& let ExprKind::AddrOf(_, _, exp) = arg.kind
69+
{
70+
// Checks if the argument inside `from_str` is a function call or a definition
71+
match exp.kind {
72+
ExprKind::Path(QPath::Resolved(_, path)) => {
73+
if let Local(id) = path.res
74+
&& let Node::Pat(pattern) = cx.tcx.hir_node(id)
75+
&& let Node::LetStmt(parent) = cx.tcx.parent_hir_node(pattern.hir_id)
76+
&& let Some(init) = parent.init
77+
&& let ExprKind::Call(_, [arg]) = &init.kind
78+
&& extract_format_args(arg).is_some()
79+
{
80+
span_lint_and_then(
81+
cx,
82+
BAD_STRING_CONCATENATION,
83+
expr.span,
84+
"using `EntityUid::from_str` with format! is inefficient",
85+
|diag| {
86+
diag.span_note(parent.span, "variable defined here");
87+
},
88+
);
89+
}
90+
},
91+
ExprKind::Call(_, [arg]) => {
92+
// Checks if the given expression is a format macro
93+
if let Some([format_arg]) = extract_format_args(arg) {
94+
if let Some(arg) = extract_string_literal(format_arg) {
95+
let ty_snippet = snippet(cx, ty.span, "EntityUid");
96+
span_lint_and_sugg(
97+
cx,
98+
BAD_STRING_CONCATENATION,
99+
expr.span,
100+
"using `EntityUid::from_str` with format! is inefficient",
101+
"try this instead",
102+
format!("{ty_snippet}::from_str({arg:?})"),
103+
Applicability::MachineApplicable,
104+
);
105+
} else {
106+
span_lint_and_help(
107+
cx,
108+
BAD_STRING_CONCATENATION,
109+
expr.span,
110+
"using `EntityUid::from_str` with format! is inefficient",
111+
None,
112+
"consider using `EntityUid::from_type_name_and_id` instead",
113+
);
114+
}
115+
}
116+
},
117+
_ => {},
118+
}
119+
}
120+
}
121+
}
122+
123+
/// Checks if the given expression is a format macro or not and returns the arguments
124+
fn extract_format_args<'tcx>(expr: &Expr<'tcx>) -> Option<&'tcx [Expr<'tcx>]> {
125+
if let ExprKind::Block(block, _) = expr.kind
126+
&& let Some(bexpr) = block.expr
127+
&& let ExprKind::Call(_, args) = &bexpr.kind
128+
{
129+
return Some(args);
130+
}
131+
132+
None
133+
}
134+
135+
// Checks if the argument of format macro is a string literal and extract it
136+
fn extract_string_literal(expr: &Expr<'_>) -> Option<Symbol> {
137+
if let ExprKind::Call(_, [arg]) = expr.kind
138+
&& let ExprKind::AddrOf(_, _, expr) = arg.kind
139+
&& let ExprKind::Array([element]) = expr.kind
140+
&& let ExprKind::Lit(Spanned {
141+
node: Str(item, _), ..
142+
}) = element.kind
143+
{
144+
return Some(item);
145+
}
146+
147+
None
148+
}
149+
150+
#[test]
151+
fn ui() {
152+
dylint_testing::ui_test(env!("CARGO_PKG_NAME"), "ui");
153+
}
Lines changed: 43 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,43 @@
1+
#![allow(unused)]
2+
3+
use cedar_policy::EntityUid;
4+
5+
fn main() {
6+
// This should trigger the lint
7+
let eid1 = EntityUid::from_str(&format!(r#"User::"alice""#));
8+
9+
// This should trigger the lint
10+
let user = "alice";
11+
let eid2 = EntityUid::from_str(&format!(r#"User::"{}""#, user));
12+
13+
// This should trigger the lint - (even with std::format!)
14+
let eid3 = EntityUid::from_str(&std::format!(r#"User::"bob""#));
15+
16+
// This should NOT trigger
17+
let eid4 = EntityUid::from_str(r#"User::"alice""#);
18+
19+
// This should NOT trigger
20+
let literal = String::from(r#"User::"dave""#);
21+
let eid5 = EntityUid::from_str(&literal);
22+
23+
let entity_type_name = "Jans::User";
24+
let entity_id = "user123";
25+
let eid7 = cedar_policy::EntityUid::from_str(&format!(r#"{entity_type_name}::"{entity_id}""#));
26+
27+
let eid8 = cedar_policy::EntityUid::from_str(&format!(r#"Jans::User::"bob123""#));
28+
29+
// This should be flagged as well since it also has an impact on the performance
30+
let raw_eid = format!(r#"{entity_type_name}::"{entity_id}""#);
31+
let eid9 = EntityUid::from_str(&raw_eid);
32+
}
33+
34+
// Mock EntityUid for testing (since we don't want to pull in the actual Cedar crate)
35+
mod cedar_policy {
36+
pub(super) struct EntityUid;
37+
38+
impl EntityUid {
39+
pub(super) fn from_str(s: &str) -> Result<Self, String> {
40+
Ok(EntityUid)
41+
}
42+
}
43+
}

0 commit comments

Comments
 (0)