Skip to content

text_dir_codepoint lint update #16452

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

Open
enthropy7 wants to merge 2 commits into
base: master
Choose a base branch
from
+743 −0
Open

text_dir_codepoint lint update #16452

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d1bcb39
Add tests for text_direction_codepoint lint
enthropy7 Dec 31, 2025
c2c3e3e
Implement text_direction_codepoint lint
enthropy7 Jan 7, 2026
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
2 changes: 2 additions & 0 deletions Cargo.lock
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 4 additions & 0 deletions Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -109,6 +109,8 @@ thiserror = "2.0.17"
time = { version = "0.3.44", features = ["parsing", "formatting", "serde"] }
toml = { version = "0.9.10", default-features = false }
toml_edit = { version = "0.24.0", features = ["serde"] }
toml_parser = "1.0.6"
toml_writer = "1.0.0"
tracing = { version = "0.1.44", default-features = false, features = ["std"] } # be compatible with rustc_log: https://github.com/rust-lang/rust/blob/e51e98dde6a/compiler/rustc_log/Cargo.toml#L9
tracing-chrome = "0.7.2"
tracing-subscriber = { version = "0.3.22", features = ["env-filter"] }
Expand Down Expand Up @@ -214,6 +216,8 @@ thiserror.workspace = true
time.workspace = true
toml = { workspace = true, features = ["std", "serde", "parse", "display", "preserve_order"] }
toml_edit.workspace = true
toml_parser.workspace = true
toml_writer.workspace = true
tracing = { workspace = true, features = ["attributes"] }
tracing-subscriber.workspace = true
unicase.workspace = true
Expand Down
15 changes: 15 additions & 0 deletions src/cargo/core/workspace.rs
View file
Open in desktop
Copy link
Contributor

@epage epage Dec 31, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixes #16373 and #16374

I had forgotten that we had separate issues. The question is whether we should mirror rustc or lint them together. I had overlooked this conversation in #16374, focusing on the other parts.

Copy link
Contributor

@epage epage Dec 31, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A good starting point would be to check the history of rustc as to why they made them separate lints.

Copy link
Author

@enthropy7 enthropy7 Dec 31, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i think it’s easier to keep lints together cause we parse cargo in raw format so, but i will check and come back later after new year :)

Copy link
Author

@enthropy7 enthropy7 Dec 31, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

rustc separated the lints because in Rust code, BiDi in string literals might be legitimate (RTL language support) while in comments it's always suspicious. In TOML there's no such distinction - BiDi anywhere in a manifest is suspicious (no legitimate RTL use case for dependency names/versions). Single lint makes sense for Cargo.

Copy link
Member

@weihanglo weihanglo Dec 31, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This sounds reasonable to me. Thanks for the investigation!

Copy link
Contributor

@epage epage Dec 31, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

package.description, package.metadata, and workspace.metadata do up some possibilities.

Copy link
Author

@enthropy7 enthropy7 Dec 31, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For those cases users can set text_direction_codepoint = "warn" or "allow" in [lints.cargo]. Deny-by-default is still appropriate since legitimate RTL in manifests is rare

Copy link
Contributor

@epage epage Jan 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are likely valid reasons for package.description to have text direction code points, e.g. if they are written in a non-english language that uses a different direction. This becomes even bigger of a deal for *.metadata.

My main concern is actually around performance. I was going to suggest we start this as one lint and if there is enough of a motivating case, we split this into two lints with the current lint becoming a lint group. However, in thinking that through, I realized we can easily implement this lint without much of a performance impact.

The way I see this working is we have one function for checking both lints. This function would do a string search for the different code points in question in the Cargo.toml, collecting their spans. If they are not present, we bail out. If they are present, we use toml_parser to create Events. We then iterate through all of the Events to see if they include the span in question and based on EventKind, we choose which lint to fire.

Copy link
Author

@enthropy7 enthropy7 Jan 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i already started to work on your suggestion, forgot to answer. thanks, i like your idea, will push new method soon

Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ use crate::lints::analyze_cargo_lints_table;
use crate::lints::rules::blanket_hint_mostly_unused;
use crate::lints::rules::check_im_a_teapot;
use crate::lints::rules::implicit_minimum_version_req;
use crate::lints::rules::text_direction_codepoint;
use crate::ops;
use crate::sources::{CRATES_IO_INDEX, CRATES_IO_REGISTRY, PathSource, SourceConfigMap};
use crate::util::context::{FeatureUnification, Value};
Expand Down Expand Up @@ -1313,6 +1314,13 @@ impl<'gctx> Workspace<'gctx> {
&mut run_error_count,
self.gctx,
)?;
text_direction_codepoint(
pkg.into(),
&path,
&cargo_lints,
&mut run_error_count,
self.gctx,
)?;

if run_error_count > 0 {
let plural = if run_error_count == 1 { "" } else { "s" };
Expand Down Expand Up @@ -1370,6 +1378,13 @@ impl<'gctx> Workspace<'gctx> {
&mut run_error_count,
self.gctx,
)?;
text_direction_codepoint(
self.root_maybe().into(),
self.root_manifest(),
&cargo_lints,
&mut run_error_count,
self.gctx,
)?;
}

// This is a short term hack to allow `blanket_hint_mostly_unused`
Expand Down
3 changes: 3 additions & 0 deletions src/cargo/lints/rules/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,16 +1,19 @@
mod blanket_hint_mostly_unused;
mod im_a_teapot;
mod implicit_minimum_version_req;
mod text_direction_codepoint;
mod unknown_lints;

pub use blanket_hint_mostly_unused::blanket_hint_mostly_unused;
pub use im_a_teapot::check_im_a_teapot;
pub use implicit_minimum_version_req::implicit_minimum_version_req;
pub use text_direction_codepoint::text_direction_codepoint;
pub use unknown_lints::output_unknown_lints;

pub const LINTS: &[crate::lints::Lint] = &[
blanket_hint_mostly_unused::LINT,
implicit_minimum_version_req::LINT,
im_a_teapot::LINT,
text_direction_codepoint::LINT,
unknown_lints::LINT,
];
Loading