Suggest using Cow on mixture of &'static str + Strings

[llogiq]

I see a lot of owned strings in many places where a Cow<'static, str> will do. The latter will likely improve performance.

How do we detect this? It's a value of type String which are only initialized with other Strings from elsewhere or &'static str (via .to_owned()/.to_string()). Also we should probably check if the value is part of the public interface (as we did recently with wrong_pub_self_convention).

I think this should be possible with the ExprUseVisitor – so I finally have a reason to check it out. 😄

[llogiq]

Revisiting this, it is not clear to me if the ExprUseVisitor really can do the things we need.

First, we need to look at the whole crate. We start from a set of every bound value, struct/tuple field or enum variant content that is of type String. We then check all initializations and modifications of our String and remove it from the set if it contains a .to_owned(), .to_string(), .from() or .into() for a &str with any but static lifetime. Then we remove all Strings from the set that are part from the public API (e.g. returned from a function, etc.). We also need to keep track of all uses of our String – if at least one of them takes an &str, we should report the declaration, all initializations/assignments and all uses of the string, and for each suggest a code replacement that uses Cow<'static, str> instead (if there is any difference).

So if we want to use the ExprUseVisitor, I think our delegate needs to have a Map of Paths (or something different that we can use to distinguish those things) to a struct with the exprs of all initializations, assignments and uses of the String (we can use parenting to find out how it's used) plus a flag that tells us to stop...or perhaps an Option<(Vec<&'tcx Expr>, Vec<&'tcx Expr>, Vec<&tcx Expr>)>).

[llogiq]

I also need to implement Visitor somewhere, because I need a point when the whole crate is checked to report all instances of Cowifiable Strings. This allows me to None out the nodes that are public.

[llogiq]

I think this is medium to get at all, but hard to get right.

@Manishearth am I on the right track? Or is there an easier path?

[llogiq]

I started a cow branch to get my code online. I think I have the Visitor part down (there's the open question of generics, but I'm just ignoring them for now).

Now I need to implement the Delegate for the ExprUseVisitor and grab an InferCtxt to fill my map. Then I somehow need to use that map for reporting.

[llogiq]

@birkenfeld I seem to have problems getting the lifetimes right. Halp ❓ 😅

[birkenfeld]

@llogiq how can I help?

[llogiq]

I posed a question on stackoverflow which luckily got the correct answer. The reason for the problem was that I had a wrong lifetime for the Delegate.

[llogiq]

This is a lot more complicated than I'd thought. I have to find local variables (similar to shadow), struct fields/enum variant args (I don't have anything similar yet), etc. to bind the "use" to the correct identity. I need a check if a String comes from a borrowed str. I also need to care about things returned from the function (because the function may return String).