feat(jans-cedarling): add custom linter for inefficient string concatenation #13164
Conversation
…enation 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>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a new dylint-style crate Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
|
How can this new rule be applied to the Cedarling code? |
We can add it to the CI that's why the pr is draft I wanted you guys opinion before continuing |
|
Looks OK to me, I don't know how to make it better |
haileyesus2433
left a comment
haileyesus2433
left a comment
There was a problem hiding this comment.
i think we should also fix the existing violations in this PR also don't forget to update the ci other than that LGTM, but the existing violations should be fixed
Signed-off-by: dagregi <dagmawi.m@proton.me>
Signed-off-by: dagregi <dagmawi.m@proton.me>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 Fix all issues with AI agents
In `@jans-cedarling/custom-lints/Cargo.toml`:
- Around line 2-6: Update the package metadata in Cargo.toml by replacing the
placeholder values for the authors and description fields with real,
project-appropriate values: set authors = ["Full Name <email@example.com>" or an
organization array] and provide a clear, concise description string describing
the crate's purpose (instead of "authors go here" and "description goes here");
keep other fields (name, version, edition) intact and ensure the metadata
reflects the repository ownership so tooling and diagnostics show accurate info.
In `@jans-cedarling/custom-lints/src/lib.rs`:
- Line 1: This file lacks the required Apache 2.0 license header; add the
standard Apache 2.0 header comment block at the very top of the file (above the
existing crate attribute #![feature(rustc_private)]) so every Rust source file
contains the license comment per guidelines; ensure the header uses the
project's canonical Apache 2.0 wording and year/owner as used across the repo.
- Around line 10-32: Update the docstring for the lint declared with
dylint_linting::declare_late_lint! to remove the "What it does" and "Why is this
bad" section headers and instead provide a single concise rationale explaining
why the pattern is problematic (e.g., using format! with EntityUid::from_str is
inefficient) and include a short example showing the bad pattern
(EntityUid::from_str(&format!(...))) and the recommended replacement (using
EntityTypeName/EntityId and EntityUid::from_type_name_and_id); keep wording in
standard Rust docstring style and trim excess narrative so the docs focus on the
rationale and example.
In `@jans-cedarling/custom-lints/ui/main.rs`:
- Line 1: This file is missing the required Apache 2.0 license header; add the
standard Apache-2.0 comment block at the very top of the Rust source (before the
fn main declaration) using the repository's canonical header text (including the
license name and copyright/years/owner placeholder as used across the project)
so every Rust file includes the Apache 2.0 header.
- Around line 3-17: The test contains unused local bindings (eid1, eid2, eid3,
eid4, eid5, user, literal) and an unused parameter s that trigger warnings;
rename these by prefixing an underscore (e.g., _eid1, _user, _literal) and
rename the function parameter s to _s (or similarly) to silence unused-variable
warnings; apply the same underscore-prefix fix to the analogous bindings
referenced in lines 23-25 as well.
In `@jans-cedarling/custom-lints/ui/main.stderr`:
- Around line 1-8: The lint help text is too vague for cases like
EntityUid::from_str(&format!(...))—update the lint message (the
bad_string_concatenation warning emission that triggers on EntityUid::from_str +
format!) to explain that using format! causes an unnecessary allocation and
subsequent parsing, and provide a concrete, efficient alternative such as using
a literal or a typed constructor (e.g., suggest
EntityUid::from_type_name_and_id("User", "alice") or passing a string literal
directly) so callers can see exactly how to avoid the allocation and parse step.
- Around line 10-16: The help text wrongly suggests "use string literals
instead" when the code uses format! with runtime data; update the lint that
detects the pattern involving EntityUid::from_str and format! so it
distinguishes literal-only format! uses from dynamic ones, and change the
guidance: for literal-only cases keep suggesting a string literal, but for
dynamic interpolation recommend using the typed constructor (e.g.,
EntityUid::from_type_name_and_id(type_name, id)) or another allocation-free API
instead of parsing; adjust the emitted message text and the detection logic that
looks for EntityUid::from_str(...) and format!(...) to produce the appropriate
message for each case.
Signed-off-by: dagregi <dagmawi.m@proton.me>
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In @.github/workflows/test-cedarling.yml:
- Around line 40-47: The GitHub Actions step "Install dylint" currently runs an
unpinned "cargo install cargo-dylint dylint-link" which can install incompatible
versions; change that run to pin both tools to 5.0.0 (e.g., "cargo install
cargo-dylint@5.0.0 dylint-link@5.0.0") so they match the workspace dependency
dylint_linting = "5.0.0" and avoid runtime compatibility errors.
In `@jans-cedarling/custom-lints/src/lib.rs`:
- Around line 25-32: The documentation comment for the lint that checks the
EntityUid::from_str(&format!(...)) pattern contains a typo: "create" should be
"crate"; update the doc string in lib.rs (the comment block describing the lint
for EntityUid::from_str(&format!(...))) to replace "create documentation" with
"crate documentation" so the wording is correct.
- Around line 56-65: The match on the type path currently uses path.segments[0]
which misses qualified paths like cedar_policy::EntityUid; change it to inspect
the last segment safely (e.g., use path.segments.last() and compare its ident to
"EntityUid" or pattern-match Some(last) before comparing) so the check works for
both plain and qualified paths and avoids panics on empty segments; keep the
surrounding guards (ExprKind::Call, ExprKind::Path with QPath::TypeRelative,
TyKind::Path with QPath::Resolved, and the function name check segment.ident ==
sym::from_str) intact.
Signed-off-by: dagregi <dagmawi.m@proton.me>
Signed-off-by: dagregi <dagmawi.m@proton.me>
Signed-off-by: dagregi <dagmawi.m@proton.me>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@jans-cedarling/custom-lints/src/lib.rs`:
- Around line 76-85: The suggestion currently emits an unqualified
"EntityUid::from_str(...)" which may not compile; modify the span_lint_and_sugg
call in the BAD_STRING_CONCATENATION branch (where
extract_string_literal(format_arg) is handled) to use the original qualified
type path snippet instead of the bare "EntityUid"—use snippet(cx, ty.span,
"EntityUid") (the ty that holds the type path) to build the replacement string
for EntityUid::from_str so the machine-applicable suggestion preserves the
original qualified path and compiles. Ensure you still pass the same
applicability and message, and update the formatted suggestion construction
where format_arg and expr.span are used.
Signed-off-by: dagregi <dagmawi.m@proton.me>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@jans-cedarling/custom-lints/src/lib.rs`:
- Around line 53-100: There is a minor typo in the inline comment inside the
LateLintPass::check_expr implementation: change "extarct" to "extract" in the
comment that describes extracting the argument of the inner function call (near
the sequence of pattern matches including ExprKind::Call(_, [format_call]) and
ExprKind::Block(block, _)); update the comment text in the check_expr block
accordingly so it reads "extract" instead of "extarct".
added the check in the CI I haven't found any existing issues though |
Signed-off-by: dagregi <dagmawi.m@proton.me>
in jans/jans-cedarling/cedarling/src/common/default_entities.rs Lines 404 to 408 in f4eb2f5 should be refactored to use |
.github/workflows/test-cedarling.yml
Outdated
| components: llvm-tools-preview, rustc-dev | ||
| - name: Install dylint | ||
| run: | | ||
| cargo install cargo-dylint --version 5.0.0 --locked |
There was a problem hiding this comment.
Maybe we can utilize cargo-binstall to download the binary instead of building it each time?
olehbozhok
left a comment
olehbozhok
left a comment
There was a problem hiding this comment.
I think it would be great to add a README file to the custom-lints folder explaining how it works, so other coworkers don’t have to spend extra time getting up to speed.
Good find I haven't thought about the format macro being assigned as a variable then passed to the method I will have to check first if it has a performance impact then add it to the linter |
* Added a README for the custom lints Signed-off-by: dagregi <dagmawi.m@proton.me>
Signed-off-by: dagregi <dagmawi.m@proton.me>
Signed-off-by: dagregi <dagmawi.m@proton.me>
Signed-off-by: dagregi <dagmawi.m@proton.me>
mo-auto
added
the
area-documentation
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @.github/workflows/test-cedarling.yml:
- Line 45: Replace the unpinned action reference "uses:
cargo-bins/cargo-binstall@main" with a pinned full 40-character commit SHA
(e.g., "cargo-bins/cargo-binstall@<commit-sha>") to match the other pinned
actions; locate the exact line containing cargo-bins/cargo-binstall@main in the
workflow and update it to use a specific commit SHA for a stable release so the
workflow is consistent with the supply-chain security posture of the other
actions.
dagregi
force-pushed
the
jans-cedarling-13010
branch
from
91162b2 to
a41c6ba
Compare
Signed-off-by: dagregi <dagmawi.m@proton.me>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @.github/workflows/test-cedarling.yml:
- Around line 53-58: The workflow installs stable then runs `cargo dylint` which
compiles the `custom-lints` crate with the wrong toolchain; change the Run
custom lints step so it invokes `cargo +nightly-2025-09-18 dylint --all
--workspace --path custom-lints` (matching the crate's rust-toolchain) instead
of installing or relying on stable, ensuring `cargo dylint` and the
`custom-lints` lint library are built with the same nightly-2025-09-18
toolchain.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @.github/workflows/test-cedarling.yml:
- Around line 46-49: Update the two cargo binstall invocations that install
cargo-dylint and dylint-link to run non-interactively in CI by adding the
--no-confirm flag to both commands (the lines invoking "cargo binstall
cargo-dylint --version 5.0.0 --locked" and "cargo binstall dylint-link --version
5.0.0 --locked"); ensure both commands include --no-confirm so the workflow
won't hang waiting for user confirmation.
Signed-off-by: dagregi <dagmawi.m@proton.me>
According to this PR https://github.com/JanssenProject/jans/pull/12988/changes it has perfomance impact. But I assume creating linter for this case will be hard. If it is, we can just fix that code line and merge the current changes. |
It's alright I already updated the linter for such case in f3c8b64 and it works fine,thanks for the thought |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @.github/workflows/test-cedarling.yml:
- Around line 35-38: The harden-runner action usage in this job pins an older
commit (`@a4aa98b93cab29d9b1101a6143fb8bce00e2eac4` # v2.7.1); update the `uses:
step-security/harden-runner@...` entry in this job to the same commit used
elsewhere (`@5ef0c079ce82195b2a36a210272d6b661572d83e` # v2.14.2) so the `Harden
Runner` step uses the consistent, up-to-date v2.14.2 release of the action.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Signed-off-by: Mohammad Abudayyeh <47318409+moabu@users.noreply.github.com>
5 participants
Prepare
Description
Target issue
closes #13010
Implementation Details
Test and Document the changes
Please check the below before submitting your PR. The PR will not be merged if there are no commits that start with
docs:to indicate documentation changes or if the below checklist is not selected.Summary by CodeRabbit
New Features
Chores
Tests
Documentation
Bug Fixes