fix: incorrect suggestion for default_trait_access

[hamirmahal]

fixes #12909.

Please write a short comment explaining your change (or "none" for internal only changes)

changelog: [default_trait_access]: fix incorrect code suggestion

[rustbot]

r? @Alexendoo

rustbot has assigned @Alexendoo.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

[hamirmahal]

Is there a more robust check we can do that would keep data::Data::default() as data::Data::default(), but still replace std::string::String with String?

[hamirmahal]

I'm worried this might be too brittle.

[hamirmahal]

I tried factoring out the cx.tcx.def_path_str(def.did()) into a full_path variable, but that oddly kept "std::string::String" as "std::string::String", instead of mapping it to "String", when doing

    let full_path = cx.tcx.def_path_str(def.did());
    let replacement = if full_path.starts_with("std::") {
        with_forced_trimmed_paths!(format!("{}::default()", full_path))
    } else {
        with_no_trimmed_paths!(format!("{}::default()", full_path))
    };

[y21]

What is the std:: special casing for, or what makes us not want to suggest the full path for String specifically?
Users can shadow the String type with their own and then we still need std::string::String.
Also, not all types in std are automatically part of the prelude.

I tried factoring out the cx.tcx.def_path_str(def.did()) into a full_path variable, but that oddly kept "std::string::String" as "std::string::String", instead of mapping it to "String", when doing

This happens because the macro sets a special flag in the type pretty printer before evaluating the expression. When you factor it out like that, it evaluates the def_path_str call without having set the special flag, so this is equivalent to not using the trimmed path macros at all I believe

[hamirmahal]

What is the std:: special casing for, or what makes us not want to suggest the full path for String specifically? Users can shadow the String type with their own and then we still need std::string::String. Also, not all types in std are automatically part of the prelude.

with_forced_trimmed_paths automatically converted std::string::String to String, but that would make this lint sometimes give an incorrect suggestion.

If we use with_no_trimmed_paths for everything, the suggestion can be unnecessarily verbose, which I thought might annoy end-users. Also, if we do that, we change existing behavior for this lint, which might be fine.

Should we just use with_no_trimmed_paths in every case?

[y21]

I'm not sure what the right move here is if the plan is to get a "machine applicable" suggestion. I do know that it's a general, known issue that type pretty printing can result in invalid paths and quite a few lints are affected by this. Some lints work around it by taking a snippet of a path from the source code, but I don't think that works here

[Alexendoo]

I don't think there's a correct answer here unfortunately, pretty printing is unaware about what's currently in scope and I believe that information has been discarded at this point. Keeping the suggestion non machine applicable and using the default or force trimmed paths seems like the best choice

We could have some specific machine applicable suggestions though, if we find that the parent node is a let statement with a provided type we could use that to produce the suggestion and suggest removing the type there at the same time. Some care would need to be taken around generics though

[hamirmahal]

Hmm... how do we feel about always using the full, untrimmed paths?

The suggestions could be longer, but will they always be correct?

If so, I think the longer paths are worth it, since they'll let us automatically fix this issue whenever it comes up.

[Alexendoo]

It would let the lint be machine applicable but it would still (usually) need manual intervention to clean the path up, I don't think that should be automatically applied as this is a relatively stylistic lint. Certainly not clear cut though

[hamirmahal]

I'm spread a bit thin at the moment; I probably won't be able to dig into this anytime soon.

Should I just leave this open here for anyone who wants to take over?

[Alexendoo]

No worries. You can close the PR if you can't find the time to work on it, or if you plan to return to it at a later date it's fine to leave it open

[xFrednet]

Hey this is triage, I'm closing this due to inactivity. If you want to continue this implementation, you're welcome to create a new PR. Thank you for the time, you already put into this!

Interested parties are welcome to pick this implementation up as well :)

@rustbot label +S-inactive-closed -S-waiting-on-author -S-waiting-on-review