Support hovering limits for adts #17021

roife · 2024-04-06T06:03:35Z

Fix #17009

Currently, r-a supports limiting the number of struct fields displayed when hovering. This PR extends it to support enum variants and union fields. Since the display of these three (ADTs) is similar, this PR extends 'hover_show_structFields' to 'hover_show_adtFieldsOrVariants'.
This PR also resolved the problem that the layout of ADT was not restricted by display limitations when hovering on the Self type.
Additionally, this PR changes the default value of display limitations to 10 (instead of the original null), which helps users discover this feature.

Young-Flash

Thanks!

small nit: IMO default "10" is a little big, 10 fields exceeding the hover windows

roife · 2024-04-07T03:30:54Z

How about changing the default limitation to 5?

l1nxy · 2024-04-08T06:47:40Z

I'm wondering if the change in configuration should be documented on #4604.

lnicola · 2024-04-08T06:53:18Z

I'm wondering if the change in configuration should be documented on #4604.

That's mainly for protocol changes (extensions), but we do have a compatibility mechanism for renamed settings.

bors · 2024-04-16T08:07:00Z

☔ The latest upstream changes (presumably #16639) made this pull request unmergeable. Please resolve the merge conflicts.

…on for ADTs

…when hovering on Self type

Veykril · 2024-04-18T09:38:16Z

crates/hir/src/display.rs

-                f.write_str(",\n")?;
-            }
-            f.write_str("}")?;
+fn display_fields_or_variants<T: HirDisplay>(


I think we should split this into two, one for fields one for variants. Reason being for an enum like

enum Enum { Variant {}, Variant2 { field: Field }, Variant3(), Variant4(Field), Variant5, }

hovering the enum should yield

enum Enum { Variant {}, Variant2 { .. }, // or the field if the limit is enabled and non-zero Variant3(), Variant4(,.), , // or the field if the limit is enabled and non-zero Variant5, }

That is hovering an enum should only show its shape, not its vairants fields. To see those the person can hover the variant instead.

Variant2 { .. }, // or the field if the limit is enabled and non-zero

Here I use a single config to limit the num of fields or variants of ADTs, should we separate it into two different ones?

Ah wait I made a mistake, hovering enums should not render the variants fields (well I said that already but my example has incorrect comments I guess).

Splitting the two into two configs sounds reasonable nevertheless.

So, for hovering on enums, we can display it as 🤔

enum Enum { Variant {}, Variant2 { /* .. */ }, Variant3(), Variant4( /* .. */ ), Variant5, }

Yes, basically for the variants we want to use the same format that is used for when we set the config to 0 for fields on structs / unions. (which fits your example here)

crates/ide/src/hover/tests.rs

roife · 2024-04-19T14:03:35Z

@rustbot ready

Veykril

Last review round

Veykril · 2024-04-19T16:16:49Z

crates/ide/src/hover/render.rs

@@ -410,8 +410,20 @@ pub(super) fn definition(
        Definition::Trait(trait_) => {
            trait_.display_limited(db, config.max_trait_assoc_items_count).to_string()
        }
-        Definition::Adt(Adt::Struct(struct_)) => {
-            struct_.display_limited(db, config.max_struct_field_count).to_string()
+        Definition::Adt(adt @ (Adt::Struct(_) | Adt::Union(_))) => {


We should do this branch for Definition::Variant as well

Veykril · 2024-04-19T16:18:15Z

crates/rust-analyzer/src/config.rs

+        /// How many variants of an enum to display when hovering on. Show none if empty.
+        hover_show_enumVariants: Option<usize> = Some(5),
+        /// How many fields of a struct or union to display when hovering on. Show none if empty.
+        hover_show_structOrUnionFields: Option<usize> = Some(5),


Suggested change

hover_show_structOrUnionFields: Option<usize> = Some(5),

hover_show_fields: Option<usize> = Some(5),

since this should also work for enum variants, so just fields should be good enough

Veykril · 2024-04-25T07:09:50Z

@bors r+

bors · 2024-04-25T07:09:53Z

📌 Commit aa1f134 has been approved by Veykril

It is now in the queue for this repository.

bors · 2024-04-25T07:23:30Z

⌛ Testing commit aa1f134 with merge 65eda41...

bors · 2024-04-25T07:35:44Z

☀️ Test successful - checks-actions
Approved by: Veykril
Pushing 65eda41 to master...

rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Apr 6, 2024

roife force-pushed the add-hover-limits-for-adts branch 2 times, most recently from f405b4a to 0ec41cd Compare April 6, 2024 06:53

Young-Flash approved these changes Apr 7, 2024

View reviewed changes

roife added 4 commits April 16, 2024 16:26

Add config hover_show_adtFieldsOrVariants to handle hovering limitati…

21da6c6

…on for ADTs

Update tests and docs for hover_show_adtFieldsOrVariants

01c3559

fix: the fields or variants of ADT was not restricted by limitations …

c06d670

…when hovering on Self type

fix: adjust the limitation for ADTs' fields to 5

6bb8598

roife force-pushed the add-hover-limits-for-adts branch from edac1b5 to 6bb8598 Compare April 16, 2024 08:29

Veykril reviewed Apr 18, 2024

View reviewed changes

Veykril added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 18, 2024

fix: add a separate setting for enum variants

e0e28ec

roife force-pushed the add-hover-limits-for-adts branch from adbefb8 to e0e28ec Compare April 19, 2024 13:46

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Apr 19, 2024

Veykril reviewed Apr 19, 2024

View reviewed changes

Add hovering limitations support for variants

4357698

roife force-pushed the add-hover-limits-for-adts branch from 99c3794 to 4357698 Compare April 20, 2024 01:14

fix: remove space within {} when no fields in struct

aa1f134

Veykril approved these changes Apr 21, 2024

View reviewed changes

Veykril approved these changes Apr 25, 2024

View reviewed changes

bors merged commit 65eda41 into rust-lang:master Apr 25, 2024
11 checks passed

philips-software-forest-releaser bot mentioned this pull request Jun 4, 2024

chore(deps,rust): update rust-lang.rust-analyzer philips-software/amp-devcontainer#436

Merged

This was referenced Jun 11, 2024

chore(deps,rust): update rust-lang.rust-analyzer philips-software/amp-devcontainer#445

Merged

chore(deps,rust): update rust-lang.rust-analyzer philips-software/amp-devcontainer#447

Merged

philips-software-forest-releaser bot mentioned this pull request Jun 18, 2024

chore(deps,rust): update rust-lang.rust-analyzer philips-software/amp-devcontainer#456

Merged

philips-software-forest-releaser bot mentioned this pull request Jun 25, 2024

chore(deps,rust): update rust-lang.rust-analyzer philips-software/amp-devcontainer#469

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support hovering limits for adts #17021

Support hovering limits for adts #17021

roife commented Apr 6, 2024 •

edited

Loading

Young-Flash left a comment

roife commented Apr 7, 2024 •

edited

Loading

l1nxy commented Apr 8, 2024

lnicola commented Apr 8, 2024

bors commented Apr 16, 2024

Veykril Apr 18, 2024

roife Apr 18, 2024

Veykril Apr 18, 2024 •

edited

Loading

roife Apr 19, 2024

Veykril Apr 19, 2024

roife commented Apr 19, 2024

Veykril left a comment

Veykril Apr 19, 2024

Veykril Apr 19, 2024

Veykril commented Apr 25, 2024

bors commented Apr 25, 2024

bors commented Apr 25, 2024

bors commented Apr 25, 2024

	hover_show_structOrUnionFields: Option<usize> = Some(5),
	hover_show_fields: Option<usize> = Some(5),

Support hovering limits for adts #17021

Support hovering limits for adts #17021

Conversation

roife commented Apr 6, 2024 • edited Loading

Young-Flash left a comment

Choose a reason for hiding this comment

roife commented Apr 7, 2024 • edited Loading

l1nxy commented Apr 8, 2024

lnicola commented Apr 8, 2024

bors commented Apr 16, 2024

Veykril Apr 18, 2024

Choose a reason for hiding this comment

roife Apr 18, 2024

Choose a reason for hiding this comment

Veykril Apr 18, 2024 • edited Loading

Choose a reason for hiding this comment

roife Apr 19, 2024

Choose a reason for hiding this comment

Veykril Apr 19, 2024

Choose a reason for hiding this comment

roife commented Apr 19, 2024

Veykril left a comment

Choose a reason for hiding this comment

Veykril Apr 19, 2024

Choose a reason for hiding this comment

Veykril Apr 19, 2024

Choose a reason for hiding this comment

Veykril commented Apr 25, 2024

bors commented Apr 25, 2024

bors commented Apr 25, 2024

bors commented Apr 25, 2024

roife commented Apr 6, 2024 •

edited

Loading

roife commented Apr 7, 2024 •

edited

Loading

Veykril Apr 18, 2024 •

edited

Loading