Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

RFC: Hide size/layout information by default #16825

Closed
wants to merge 1 commit into from

Conversation

Wilfred
Copy link
Contributor

@Wilfred Wilfred commented Mar 12, 2024

Most of the time, I'm not interested in the exact layout of types. However, rust-analyzer shows the information very prominently, even before the doc comment.

Screenshot 2024-03-12 at 12 38 03

I've fixed the whitespace in #16824, but I feel like opt-in is a better default for layout information. I know that this has been the behaviour for a while, but the layout information is much more visible after cf905cf.

What do you think?

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 12, 2024
@HKalbasi
Copy link
Member

I think it should be enabled by default due discoverability reasons. People can disable features they found annoying and noisy, but they can't enable features that they don't know exist.

What about placing it in its old place? Or some other place, like at the end of doc comments, or even making its place configurable, with its default being something less invasive.

@Wilfred
Copy link
Contributor Author

Wilfred commented Mar 13, 2024

FWIW there is some precedent for off-by-default features: some of the inlay hint settings and the niches option are opt-in. I suppose it depends what proportion of users want to see size and alignment info at all times.

That said, I didn't find this information obtrusive previously, so putting it back on its own line would also suit me :)

@Veykril
Copy link
Member

Veykril commented Mar 13, 2024

The placement I personally think is good where it is now (as I made that change :^), putting after docs is a plain bad choice because docs can be long and I don't want to scroll down all the way when I actually care about the info. Putting it where it was before (behind the closing brace) is imo also no ideal, we render more info than we used to for on hovers (like where clauses and fields now) which makes it less visually appealing if it comes after, and usually you put comments above the thing they describe, not below.

Enabling by default due to discoverability is always a point to have, and its pretty subjective by the team whether we deem that sufficient or not (as was raised, some inlay hints are off by default due to them being rather noisy in general even though that means people won't necessarily discover them). This problem would be solved entirely if we had a landing page on installation in VSCode #13351 which could introduce all the features. I think it would be fine for us to change the default here, in most cases this info is not relevant to people and we've had this on for a while that the ones that do make use of it will wonder where the info went and presumably look into the config to check if they can re-enable it.

Another question this raises to me is if we want to maybe change it so they are enabled by default for types with actual stable layouts (aka repr(C) etc), but disabled otherwise?

@lnicola
Copy link
Member

lnicola commented Mar 13, 2024

This doesn't seem too noisy to me, and it's probably easy enough to disable. I think we'd get more angry users (like with needless_return) if it was a problem.

This problem would be solved entirely if we had a landing page on installation in VSCode

We already have a lot of settings, and a lot of users would probably space out after going through 20 or so and just accept the defaults. I know I would.

@davidbarsky
Copy link
Contributor

I think my biggest complaint with the size/layout information is that they look like comments, which throws me for a loop. If they rendered with the system UI font—and I'm not sure how feasible that is—then I think I'd be perfectly fine with them as the default.

Another question this raises to me is if we want to maybe change it so they are enabled by default for types with actual stable layouts (aka repr(C) etc), but disabled otherwise?

I really like that approach and would strongly advocate for it. If people are using non-#[repr(Rust)] for their structs/enums, then they probably do care about size/layout information, but people who are using Arc<Mutex<SomeStruct>> probably don't—it's just noise.

(If people care about size/layout information and they're not using annotating things with-#[repr(Rust)], then I think they're the sort of power user who would be comfortable changing a setting.)

@Wilfred
Copy link
Contributor Author

Wilfred commented Mar 13, 2024

Note that some editors, at least Emacs, will prioritise the first line of the hover information in the UI. For example:

Screenshot from 2024-03-13 09-47-14

Note the footer is only showing alignment when my cursor is on self.

@HKalbasi
Copy link
Member

but people who are using Arc<Mutex> probably don't—it's just noise

One usage of those annotations is to optimize the memory usage, so the size of SomeStruct might be important even when it is not #[repr(C)] and is used in a Arc<Mutex>.

@lnicola
Copy link
Member

lnicola commented Mar 13, 2024

Right. If a struct is #[repr(C)] because you're calling into C, you don't care about its layout because you can't do anything about it.

@Veykril
Copy link
Member

Veykril commented Mar 13, 2024

If they rendered with the system UI font—and I'm not sure how feasible that is—then I think I'd be perfectly fine with them as the default.

The hover box is markdown, so we have a lot of options for that. Comments were just the simplest to od back when it was implemented I reckon.

Note that some editors, at least Emacs, will prioritise the first line of the hover information in the UI. For example:

This does not really matter here, the first line for most hovers is usually the module path, self params and other locals are the exception.

@davidbarsky
Copy link
Contributor

but people who are using Arc probably don't—it's just noise

One usage of those annotations is to optimize the memory usage, so the size of SomeStruct might be important even when it is not #[repr(C)] and is used in a Arc<Mutex>.

I mean, I see the utility there, but to be entirely honest, my eyes glaze over those annotations. I buy the utility side of things, but it just feels a bit like... noise? At least, when formatted as is.

@HKalbasi HKalbasi added S-waiting-on-decision and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 15, 2024
@Veykril
Copy link
Member

Veykril commented Apr 15, 2024

At least, when formatted as is.

Happy to take alternative formatting ideas if you have any fwiw

@HKalbasi
Copy link
Member

For the record, clangd render it in this way:
image

It is enabled by default, but only appears in definition hover, not in usage hover.

@bors
Copy link
Collaborator

bors commented Apr 16, 2024

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

@Wilfred
Copy link
Contributor Author

Wilfred commented Apr 17, 2024

@HKalbasi ooh, that seems like a really nice heuristic. Showing the layout information on definition sites means the information is available, but not too prominent.

It also looks like they're taking advantage of the fact that textDocument/hover can return multiple items, so the size can be rendered as markdown rather than source code.

@Veykril
Copy link
Member

Veykril commented Apr 17, 2024

It also looks like they're taking advantage of the fact that textDocument/hover can return multiple items, so the size can be rendered as markdown rather than source code.

We are already returning "multiple results" as well (its still just one hover result, but concatenated markdown delimited via ---), the main reason for us using comment syntax is that it was simple to come up with.

@Veykril
Copy link
Member

Veykril commented Apr 18, 2024

I'll close this in favor of two new issues #17097 and #17096 that arose from the discussion here.

@Veykril Veykril closed this Apr 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants