case_sensitive_file_extension_comparisons hits on a to_lowercase'd string

[kesinger-idexx]

Summary

This could be user error but I would have thought that forcing the str to lowercase would mean that it's ok to test for an extension with ends_with.

Lint Name

No response

Reproducer

I tried this code:

pub fn is_jpg(filename: &str) -> bool {
    filename.to_lowercase().ends_with(".jpg")
}

I saw this happen:

warning: case-sensitive file extension comparison
  --> (filename)
   |
(line) |     filename.to_lowercase().ends_with(".jpg")
   |                             ^^^^^^^^^^^^^^^^^
   |
   = note: `-W clippy::case-sensitive-file-extension-comparisons` implied by `-W clippy::pedantic`
   = help: consider using a case-insensitive comparison instead
   = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#case_sensitive_file_extension_comparisons

I expected to see this happen:

(no output)

Version

rustc 1.62.1 (e092d0b6b 2022-07-16)
binary: rustc
commit-hash: e092d0b6b43f2de967af0887873151bb1c0b18d3
commit-date: 2022-07-16
host: x86_64-apple-darwin
release: 1.62.1
LLVM version: 14.0.5

Additional Labels

No response

[xFrednet]

@rustbot label +good-first-issue

[fabinca]

If I understand it right, the error message hints that using a comparison like this would be more effiient:

use str_utils::*;

pub fn is_jpg(filename: &str) -> bool {
    filename.ends_with_ignore_ascii_case(".jpg")
}

https://docs.rs/str-utils/latest/str_utils/

[tylerjw]

@rustbot claim

I am new here, and looking at what I think is the function of this lint I'm lost. Here is what I'm figured out so far.

• There is a file case_sensitive_file_extension_comparison.rs that contains a function called check.
• This first block of code is just detecting a string literal that it stores a variable that can ask questions of that literal in ext_str:

        if let Some(method_id) = cx.typeck_results().type_dependent_def_id(expr.hir_id);
        if let Some(impl_id) = cx.tcx.impl_of_method(method_id);
        if cx.tcx.type_of(impl_id).is_str();
        if let ExprKind::Lit(Spanned { node: LitKind::Str(ext_literal, ..), ..}) = arg.kind;
        if (2..=6).contains(&ext_literal.as_str().len());
        let ext_str = ext_literal.as_str();

• Then we test if that literal begins with a dot '.' and if it is all uppercase and ascii digits or all lowercase and ascii digits.
• Lastly there is something called recv_ty that I don't understand and given that it reaches a function called span_lint_and_help.

Let me know what sort of things I should read up on to understand how to work on and fix up this lint so it does not throw a warning in the above case. I also found a test file I think I could add code to for testing if any changes made to this lint would ignore the case where the user makes the string lowercase before using the case sensitive comparison.

[tylerjw]

Copying suggestions here from the zulip thread.

@xFrednet: Hmm, that's a good question and a bit up to us to decide. We could either ignore cases where to_lowercase() or to_uppercase() is called or create a good suggestion for it. Looking at the lint description the suggestion is quite a bit longer. I think the best thing to do, would be to convert it to a Path and then again call eq_ignore_ascii_case() on the extention. In theory that should be zero copy, while to_lowercase() and to_uppercase() could require an allocation.

@xFrednet: So the issue would basically be about creating a good suggestion and clarifying, why using a Path is better then to_lowercase() etc

@xFrednet: If you want to continue working on this issue, you can take a look at span_lint_and_then and the Diagnostic object given to you. With that you can add help messages and suggestions

I will read about these things and try to build a pull request that adds more diagnostic in the case where the user calls to_lowercase or to_uppercase to inform why Path would be a better solution.

Does anyone know if there are examples of other checks that add more to the response in special cases?

[xFrednet]

Does anyone know if there are examples of other checks that add more to the response in special cases?

You can search Clippy's code base for other usages of span_lint_and_then. I would first try to create a suggestion that looks roughly like this:

std::path::Path::new(<self-snipped>)
    .extension()
    .map_or(false, |ext| ext.eq_ignore_ascii_case(<ends_with-arg-snippet>))

And then create the message with the suggestion and a help message, "consider using a case-insensitive comparison instead". For usages of to_lowercase() we can additionally add a note saying "to_lowercase() allocates memory, which can be avoided by using a path Path"