fix(const_path_join): Handle constant expressions in path joins

[jesicaMao]

Description

This PR enhances the const_path_join lint to properly handle constant expressions in path joins, addressing issue #1553.

Problem

The lint previously only identified string literals, missing constant expressions like env!("CARGO_MANIFEST_DIR"). This led to potentially incorrect suggestions that could lose the dynamic nature of constant expressions.

Solution

The implementation now uses a more robust approach to handle path components:

1. Component Classification:

   • Introduced ComponentType enum to categorize path components:
     • Literal(String) - String literals
     • Constant(String) - Const-evaluatable expressions
     • Other - Non-constant expressions

2. Smart Detection:

   • Uses is_const_evaluatable to properly identify constant expressions
   • Maintains type safety by checking for string types
   • Avoids fragile string pattern matching

3. Contextual Suggestions:

   • All literals: Suggests combining into a single string literal
   • Mix of literals and constants: Suggests using concat!()
   • Any non-constant components: No suggestions made

Implementation Details

• Modified check_component_type to properly categorize expressions
• Enhanced check_expr to handle different component combinations
• Added comprehensive test cases covering various scenarios

Testing

Added test cases in ui/ directory covering:

• Pure string literal combinations
• Paths with env!() expressions
• Mixed constant and literal expressions
• Non-constant expressions

Fixes

Fixes #1553

[jesicaMao]

@smoelius I'll try to get the CI / lint (pull_request) test to pass, in the mean time if you're free could you please go through the changes I've made just to make sure that I'm on the right track.

[smoelius]

@smoelius I'll try to get the CI / lint (pull_request) test to pass, in the mean time if you're free could you please go through the changes I've made just to make sure that I'm on the right track.

I will reply within the next few days. I really appreciate you working on this.

[jesicaMao]

@smoelius I've managed to get the CI to pass, please take a look through the changes I've made when you're free.

[smoelius]

I like that you found is_const_evaluatable, as I can see it being part of the solution. But I'm not sure this PR's current approach is the right one. For example, matching on code snippets seems very fragile:

dylint/examples/restriction/const_path_join/src/lib.rs

Line 178 in 3debcfb

if snippet.starts_with("env!(") || snippet.starts_with("concat!(") {

Furthermore, I don't think is_string_lit should have to change.

---

The approach that seems most obvious to me is the following.

There are these two calls to is_string_lit:

• dylint/examples/restriction/const_path_join/src/lib.rs

  Line 100 in 67eddaf

  && let Some(s) = is_lit_string(cx, arg)

• dylint/examples/restriction/const_path_join/src/lib.rs

  Line 115 in 67eddaf

  && let Some(s) = is_lit_string(cx, arg)

Those calls should be replaced with calls to a function named is_string_lit_or_const_evaluatable or something similar.

Calls to that function will have three possible outcomes:

• the argument is a string literal (determined using is_string_lit)
• the argument is not a string literal, but it is const evaluatable (determined using is_const_evaluatable)
• neither of the above

The caller should record the outcome for each argument it passes.

If all arguments are string literals, the behavior should be like it is now, i.e., the string literals are combined into one string literal (currently done here).

If not all arguments are string literals, but they are all const evaluatable, then the arguments should be combined using concat!(..).

---

This is a difficult issue. To reiterate, I really appreciate you working on it.

[jesicaMao]

@smoelius I have implemented all the suggestions from your review. The key changes include:

1. Replaced the string pattern matching with proper constant expression detection using is_const_evaluatable
2. Implemented a robust component categorization system using the ComponentType enum
3. Added proper handling for different combinations of literals and constant expressions

I've also added small comments throughout the code to document the logic and decision points. As I had to make some large changes, this helped me keep track of how much I had implemented. Let me know if I should remove them, although I think these comments help to clarify the implementation choices and make future maintenance easier.

The implementation now properly preserves constant expressions while still suggesting optimizations where appropriate. All tests are passing, including the new cases that verify the handling of various constant expression scenarios.

Please let me know if you would like me to clarify any part of the implementation or if there are additional improvements you'd like to see or if I should make any more changes in general.

[jesicaMao]

@smoelius Thank you for the review feedback! I've implemented your suggestions:

1. Changed the test function to use ui_test_examples instead of ui_test_example to ensure all example tests are run properly.

2. Added a new example entry in Cargo.toml:

   [[example]]
   name = "ui_const_expr"
   path = "ui/const_expr.rs"

Thanks again for your guidance, and please let me know if I have to make any more changes.

[smoelius]

Thanks again for your guidance, and please let me know if I have to make any more changes.

Could you try to get the tests to pass?

[jesicaMao]

@smoelius I've managed to get the ci to pass, please take a look now.

[smoelius]

I really appreciate you tackling this difficult issue, but the tests show that the fix is not working.

For this case, I would expect the suggestion to be:

std::path::PathBuf::from(concat!(env!("CARGO_MANIFEST_DIR"), "/src/lib.rs"))

The point is: the fix should suggest to use concat!(..), but that doesn't seem to be happening.

[smoelius]

Apologies. My earlier comment contained errors. I've since corrected it.

[jesicaMao]

@smoelius Thanks for the review feedback! I've been thinking about your suggestion to use concat!() whenever there's a constant expression in the path. While I understand the reasoning, I've gone with a slightly different approach for a few reasons:

When we have a single constant expression like env!() with string literals, I think using the normal .join() is actually more readable and intuitive:

// Current approach for single constant + string literals
PathBuf::from(env!("CARGO_MANIFEST_DIR")).join("src/lib.rs")

// vs always using concat!
PathBuf::from(concat!(env!("CARGO_MANIFEST_DIR"), "/src/lib.rs"))

There are a couple advantages to sticking with .join() for these cases:

1. It's more consistent with how paths are usually built in Rust - .join() automatically handles platform-specific path separators

2. It's easier to modify later - if someone wants to add another component, they can just add another .join()

3. When you look at the code, the intent of "joining a path component" is clearer than with the concat version

I am using concat!() for the cases where it really makes sense - when we have multiple constant expressions mixed together:

// Example where concat! makes more sense
let dir = env!("CARGO_MANIFEST_DIR");
let suffix = "logs";
// Here we use concat! since we have multiple constant expressions
PathBuf::from(concat!(dir, "/", suffix, "/output"))

Do you think this approach is reasonable, or would you strongly prefer using concat!() for all cases with constants? Happy to adjust if needed! Although in that case i would need a bit guidance in detecting the env! macro and correctly implementing it for concat!().

[smoelius]

The point of using concat! is that it produces a single string constant and avoids the call to join.

I just checked with Compiler Explorer to ensure this wasn't wishful thinking on my part: https://godbolt.org/z/n45M1n4z6

You can see that in the compiled code without concat!, there is a call to join, but in the compiled code with concat!, there is no such call.

I don't mean to dismiss your points about readability and intuitiveness, but the whole point of the lint is to eliminate calls to join.

Although in that case i would need a bit guidance in detecting the env! macro and correctly implementing it for concat!().

I made some comments below.

[smoelius]

Suggested change

	enum ComponentType {
	Literal(String),
	Constant(String),
	Other,
	}
	enum ComponentType<'tcx> {
	Literal(String),
	Constant(&'tcx Expr<'tcx>),
	Other,
	}

I would expect ComponentType to look something like this. The way I think of it is: if an expression is constant but not a string literal, we're just going to pass it to concat!. ComponentType::Constant(..) records the expression so that we have it to pass to concat!.

Then, you'll need some logic to determine whether all of the components are string literals, etc., like I was suggesting in #1574 (comment).

And I see that you have something like that now.

For the case when all components are constant, but some are not string literals, that is when you will have to construct a concat! expression.

That will involve constructing a string of the form:

"concat!(" component_0 ", " component_1 ", " ... ", " component_n ")"

If a component is of the form ComponentType::Const, you can use the expression it contains.

I saw you were using snippet_opt earlier, but just to reinforce, you can use snippet_opt(cx, expr.span) to get the source text for expr.

There are just two more small wrinkles.

First, all of the components need to be interspersed with "/". The easiest way to do that may be to insert a ComponentType::Literal("/") between each pair of adjacent components. In this way, if you collect n components initially, you will end up with n + n - 1 (= 2n - 1) components after putting the "/" between them.

Second, adjacent string literals need to be combined.

In other words, rather than this:

concat!(env!("CARGO_MANIFEST_DIR"), "/", "src", "/", "lib.rs")

we want this:

concat!(env!("CARGO_MANIFEST_DIR"), "/src/lib.rs")

Note that when this step is done, the list of components should alternate ComponentType::Constant(..), ComponentType::Literal(..), ComponentType::Constant(..), ...

Does all that make sense?