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.

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

[refurb] Implement hashlib-digest-hex (FURB181) #9077

Merged
merged 3 commits into from
Dec 10, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
57 changes: 57 additions & 0 deletions crates/ruff_linter/resources/test/fixtures/refurb/FURB181.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,57 @@
import hashlib
from hashlib import (
blake2b,
blake2s,
md5,
sha1,
sha3_224,
sha3_256,
sha3_384,
sha3_512,
sha224,
)
from hashlib import sha256
from hashlib import sha256 as hash_algo
from hashlib import sha384, sha512, shake_128, shake_256

# these will match

blake2b().digest().hex()
blake2s().digest().hex()
md5().digest().hex()
sha1().digest().hex()
sha224().digest().hex()
sha256().digest().hex()
sha384().digest().hex()
sha3_224().digest().hex()
sha3_256().digest().hex()
sha3_384().digest().hex()
sha3_512().digest().hex()
sha512().digest().hex()
shake_128().digest(10).hex()
shake_256().digest(10).hex()

hashlib.sha256().digest().hex()

sha256(b"text").digest().hex()

hash_algo().digest().hex()

# not yet supported
h = sha256()
h.digest().hex()
Copy link
Member

Choose a reason for hiding this comment

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

It's not-impossible for us to detect this, you could look at what we do in TRIO115 where we map from name to value. There's also a general utility in draft here (#8583), but not merged. Either way, not a requirement for merging, just making a mental note for myself if anything.

Copy link
Contributor Author

@sbrugman sbrugman Dec 9, 2023

Choose a reason for hiding this comment

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

Interesting, nice utility. Would be good to revisit once that PR is merged!

For myself, I took this easy rule to get back at ruff developing after a while. I'm keen to go the more challenging implementation of using the fluid interface (dosisod/refurb#286). This impacts a lot of data engineering (e.g. spark) and data science (pytorch etc.) code I see come by.



# these will not

sha256().digest()
sha256().digest().hex("_")
sha256().digest().hex(bytes_per_sep=4)
sha256().hexdigest()

class Hash:
def digest(self) -> bytes:
return b""


Hash().digest().hex()
5 changes: 5 additions & 0 deletions crates/ruff_linter/src/checkers/ast/analyze/expression.rs
Original file line number Diff line number Diff line change
Expand Up @@ -356,6 +356,8 @@ pub(crate) fn expression(expr: &Expr, checker: &mut Checker) {
Rule::FString,
// flynt
Rule::StaticJoinToFString,
// refurb
Rule::HashlibDigestHex,
]) {
if let Expr::Attribute(ast::ExprAttribute { value, attr, .. }) = func.as_ref() {
let attr = attr.as_str();
Expand Down Expand Up @@ -581,6 +583,9 @@ pub(crate) fn expression(expr: &Expr, checker: &mut Checker) {
if checker.enabled(Rule::HashlibInsecureHashFunction) {
flake8_bandit::rules::hashlib_insecure_hash_functions(checker, call);
}
if checker.enabled(Rule::HashlibDigestHex) {
refurb::rules::hashlib_digest_hex(checker, call);
}
if checker.enabled(Rule::RequestWithoutTimeout) {
flake8_bandit::rules::request_without_timeout(checker, call);
}
Expand Down
1 change: 1 addition & 0 deletions crates/ruff_linter/src/codes.rs
Original file line number Diff line number Diff line change
Expand Up @@ -965,6 +965,7 @@ pub fn code_to_rule(linter: Linter, code: &str) -> Option<(RuleGroup, Rule)> {
(Refurb, "169") => (RuleGroup::Preview, rules::refurb::rules::TypeNoneComparison),
(Refurb, "171") => (RuleGroup::Preview, rules::refurb::rules::SingleItemMembershipTest),
(Refurb, "177") => (RuleGroup::Preview, rules::refurb::rules::ImplicitCwd),
(Refurb, "181") => (RuleGroup::Preview, rules::refurb::rules::HashlibDigestHex),

// flake8-logging
(Flake8Logging, "001") => (RuleGroup::Preview, rules::flake8_logging::rules::DirectLoggerInstantiation),
Expand Down
1 change: 1 addition & 0 deletions crates/ruff_linter/src/rules/refurb/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ mod tests {
#[test_case(Rule::IsinstanceTypeNone, Path::new("FURB168.py"))]
#[test_case(Rule::TypeNoneComparison, Path::new("FURB169.py"))]
#[test_case(Rule::RedundantLogBase, Path::new("FURB163.py"))]
#[test_case(Rule::HashlibDigestHex, Path::new("FURB181.py"))]
fn rules(rule_code: Rule, path: &Path) -> Result<()> {
let snapshot = format!("{}_{}", rule_code.noqa_code(), path.to_string_lossy());
let diagnostics = test_path(
Expand Down
120 changes: 120 additions & 0 deletions crates/ruff_linter/src/rules/refurb/rules/hashlib_digest_hex.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,120 @@
use ruff_diagnostics::{Diagnostic, Edit, Fix, FixAvailability, Violation};
use ruff_macros::{derive_message_formats, violation};
use ruff_python_ast::{Expr, ExprAttribute, ExprCall};
use ruff_text_size::{Ranged, TextRange};

use crate::checkers::ast::Checker;

/// ## What it does
/// Checks for the use of `.digest().hex()` on a hashlib hash, like `sha512`.
///
/// ## Why is this bad?
/// When generating a hex digest from a hash, it's preferable to use the
/// `.hexdigest()` method, rather than calling `.digest()` and then `.hex()`,
/// as the former is more concise and readable.
///
/// ## Example
/// ```python
/// from hashlib import sha512
///
/// hashed = sha512(b"some data").digest().hex()
/// ```
///
/// Use instead:
/// ```python
/// from hashlib import sha512
///
/// hashed = sha512(b"some data").hexdigest()
/// ```
///
/// ## Fix safety
/// This rule's fix is marked as unsafe, as the target of the `.digest()` call
/// could be a user-defined class that implements a `.hex()` method, rather
/// than a hashlib hash object.
///
/// ## References
/// - [Python documentation: `hashlib`](https://docs.python.org/3/library/hashlib.html)
#[violation]
pub struct HashlibDigestHex;

impl Violation for HashlibDigestHex {
const FIX_AVAILABILITY: FixAvailability = FixAvailability::Sometimes;

#[derive_message_formats]
fn message(&self) -> String {
format!("Use of hashlib's `.digest().hex()`")
}

fn fix_title(&self) -> Option<String> {
Some("Replace with `.hexdigest()`".to_string())
}
}

/// FURB181
pub(crate) fn hashlib_digest_hex(checker: &mut Checker, call: &ExprCall) {
if !call.arguments.is_empty() {
return;
}

let Expr::Attribute(ExprAttribute { attr, value, .. }) = call.func.as_ref() else {
return;
};

if attr.as_str() != "hex" {
return;
}

let Expr::Call(ExprCall {
func, arguments, ..
}) = value.as_ref()
else {
return;
};

let Expr::Attribute(ExprAttribute { attr, value, .. }) = func.as_ref() else {
return;
};

if attr.as_str() != "digest" {
return;
}

let Expr::Call(ExprCall { func, .. }) = value.as_ref() else {
return;
};

if checker.semantic().resolve_call_path(func).is_some_and(
|call_path: smallvec::SmallVec<[&str; 8]>| {
matches!(
call_path.as_slice(),
[
"hashlib",
"md5"
| "sha1"
| "sha224"
| "sha256"
| "sha384"
| "sha512"
| "blake2b"
| "blake2s"
| "sha3_224"
| "sha3_256"
| "sha3_384"
| "sha3_512"
| "shake_128"
| "shake_256"
| "_Hash"
]
)
},
) {
let mut diagnostic = Diagnostic::new(HashlibDigestHex, call.range());
if arguments.is_empty() {
diagnostic.set_fix(Fix::unsafe_edit(Edit::range_replacement(
".hexdigest".to_string(),
TextRange::new(value.end(), call.func.end()),
)));
}
checker.diagnostics.push(diagnostic);
}
}
2 changes: 2 additions & 0 deletions crates/ruff_linter/src/rules/refurb/rules/mod.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
pub(crate) use check_and_remove_from_set::*;
pub(crate) use delete_full_slice::*;
pub(crate) use hashlib_digest_hex::*;
pub(crate) use if_expr_min_max::*;
pub(crate) use implicit_cwd::*;
pub(crate) use isinstance_type_none::*;
Expand All @@ -16,6 +17,7 @@ pub(crate) use unnecessary_enumerate::*;

mod check_and_remove_from_set;
mod delete_full_slice;
mod hashlib_digest_hex;
mod if_expr_min_max;
mod implicit_cwd;
mod isinstance_type_none;
Expand Down