Add a quickfix for accessing a private field of a struct

[MatrixFrog]

No description provided.

[davidbarsky]

Oh, I personally wanted this feature for a bit! Do you mind fixing the clippy lints, by any chance?

[MatrixFrog]

I'm hoping to finish this up tomorrow. It's my first PR to rest analyzer so it's extremely possible that I have done something quite wrong, but hopefully someone can help sort everything out.

[MatrixFrog]

Two options here.

1. Use pub(crate) if the definition and usage are in the same crate. pub otherwise.
2. Give the user the option of which to use.

[ChayimFriedman2]

I think I prefer the first option, but having a setting is also fine.

[Veykril]

+1 for the first, we do have an assist to promote pub(crate) to pub so its an easy fix for the user, no need for a setting i think

[ChayimFriedman2]

Sorry to dismiss (part of) the work you did, but I'd be more comfortable if when we generate the diagnostics we will save the field, instead of trying to lookup it in the type's fields list (while we're at it, we could also use the existing variant instead of re-resolving it, but you don't need to do this).

Generally, what needs to be done is this: here:

rust-analyzer/crates/hir-ty/src/infer.rs

Line 206 in 7230ded

private: bool,

You will need to change private from bool into Option<LocalFieldId>, and save it when emitting the diagnostic (it is readily available). Similarly here:

rust-analyzer/crates/hir/src/diagnostics.rs

Line 227 in 7230ded

pub private: bool,

You need to change private to contain Option<hir::Field> (you will need to do some conversions from hir-ty to hir types but it's trivial).

Then in the quickfix code, you call source() on the field to get the AST.

In addition to that, I noticed that you handle macro files wrongly: when the type is inside a macro, you should use the original file not just for where to put the edit, but also to find the node, otherwise the ranges will be messed up. Use original_file_X() methods on the InFile for that (they're defined in hir-expand/src/files.rs). With my suggestion, I suggest you lookup the first token in the field declaration, call original_file_range_opt() on that, and insert pub before the token.

Feel free to ask should you need more help.

[Veykril]

Nice change! That means we can remove this part from the corresponding assist I think

rust-analyzer/crates/ide-assists/src/handlers/fix_visibility.rs

Line 91 in 6440fe2

fn add_vis_to_referenced_record_field(acc: &mut Assists, ctx: &AssistContext<'_>) -> Option<()> {

I feel like NoSuchField's private bool field should be changed to Option<FieldId> (which is Some for when it exists but is not visible). THen we have access here to the semantic field and can source that directly instead of having to iterate through the AST here.

[Veykril]

The text range here might belong to a macro file here, so we need to upmap that out of macros (and use the upmapped file id from there). Otherwise things will break

[Veykril]

Same here, this might be a range from a macro expansion, we need to upmap that

[Veykril]

Oh, looks like Chayim proposed the exact same thing (with more details), didn't see that 😅

[MatrixFrog]

No, not dismissive at all, this is very helpful! Got started on that here MatrixFrog@5a259c0 but there's one spot where I'm not quite seeing how to make the conversion: MatrixFrog@5a259c0#diff-07791b9f83c04bf09ca939688d564a7e35d082e2b916b33c53632e7d28e2e96dR651

I'm still getting used to the way a lot of rust-analyzer code passes around Idx's instead of the actual structs those indexes refer to.

[ChayimFriedman2]

A hir::Field is just a VariantDef and a LocalFieldId. The LocalFieldId you have, and the hir-ty diagnostic also contains a VariantId which is an enum of StructId, UnionId and EnumVariantId. VariantDef is an enum of Struct, Union and Variant. Struct is a wrapper around StructId, ditto for Union and Variant. So you just need to match the VariantId and call into() on each variant and wrap it in VariantDef. You can even create a impl From<VariantId> for VariantDef in hir/src/lib.rs for that.