Skip to content

Postfix adjustment hints #13816

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

Merged
merged 4 commits into from
Jan 9, 2023
Merged

Conversation

WaffleLapkin
Copy link
Member

Basic Description

This PR implements "postfix" adjustment hints:
2022-12-21_19-27

They are identical to normal adjustment hints, but are rendered after the expression. E.g. expr.* instead of *expr. This mirrors "postfix deref" feature that I'm planning to eventually propose to the compiler.

Motivation

The advantage of being postfix is that you need to add parentheses less often:

2022-12-21_19-38
2022-12-21_19-37

This is because a lot of "reborrow" hints are caused by field access or method calls, both of which are postfix and have higher "precedence" than prefix & and *.

Also IMHO it just looks nicer and it's more clear what is happening (order of operations).

Modes

However, there are some cases where postfix hints need parentheses but prefix don't (for example &x being turned into (&x).*.*.& or &**&x).

This PR allows users to choose which look they like more. There are 4 options (rust-analyzer.inlayHints.expressionAdjustmentHints.mode setting):

  • prefix — always use prefix hints (default, what was used before that PR)
  • postfix — always use postfix hints
  • prefer_prefix — try to minimize number of parentheses, breaking ties in favor of prefix
  • prefer_postfix — try to minimize number of parentheses, breaking ties in favor of postfix

Comparison of all modes:

2022-12-21_19-53
2022-12-21_19-49
2022-12-21_19-48
2022-12-21_19-47

Edge cases

Where are some rare cases where chain hints weirdly interact with adjustment hints, for example (note SourceAnalyzer.&):

image

This is pre-existing, you can get the same effect with prefix hints (SourceAnalyzer)).


Another weird thing is this:

2022-12-21_20-00

Here .& is a hint and ? is written in the source code. It looks like ? is part of the hint because ?. is ligature in my font. IMO this is a bug in vscode, but still worth mentioning (I'm also too lazy to report it there...).

Fixed bugs

I've used the "needs parens" API and this accidentally fixed a bug with parens around as, see the test diff:

     let _: *const u32  = &mut 0u32 as *mut u32;
                        //^^^^^^^^^^^^^^^^^^^^^<mut-ptr-to-const-ptr>
+                       //^^^^^^^^^^^^^^^^^^^^^(
+                       //^^^^^^^^^^^^^^^^^^^^^)
...
     let _: *const u32  = &mut 0u32 as *mut u32;
                        //^^^^^^^^^^^^^^^^^^^^^<mut-ptr-to-const-ptr>
+                       //^^^^^^^^^^^^^^^^^^^^^(
+                       //^^^^^^^^^^^^^^^^^^^^^)

Changelog

changelog feature Add an option to make adjustment hints (aka reborrow hints) postfix
changelog fix Fix placement of parentheses around as casts for adjustment hints

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Dec 21, 2022
@Veykril
Copy link
Member

Veykril commented Dec 21, 2022

The chain hint collision comes from the order we emit the hints in, if two hints affect the same text range their order matters, swapping

chaining::hints(hints, sema, &famous_defs, config, file_id, &expr);
adjustment::hints(hints, sema, config, &expr);
should probably fix it

The ligature one is clearly a VSCode bug, we could problem work around it by putting similar to what we did for the decorator approach in the past #6236, though that requires intercepting the inlay hint middleware in the client (patching in the server for this feels wrong) so I can do that since I know my way around the client better.

@WaffleLapkin
Copy link
Member Author

The chain hint collision comes from the order we emit the hints in, if two hints affect the same text range their order matters, swapping

The order is correct though (type is shown before adjustments), it just looks weird

@Veykril
Copy link
Member

Veykril commented Dec 21, 2022

Well yes, but I would also argue no, because the adjustment hints are mostly correct expressions (if you forget about them not existing in this postfix form), meanwhile a random type is now interspersed. (chaining hints are the one hints I consider noisy myself which is why I don't have them enabled and keep forgetting about them when touching hint code 😅). We could of course render the adjusted types for those instead when using postfix hints if you want to be completely correct here

@bors
Copy link
Contributor

bors commented Dec 22, 2022

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

@rust-cloud-vms rust-cloud-vms bot force-pushed the postfix_adjustment_hints branch from f4618c8 to be22568 Compare December 22, 2022 10:46
Comment on lines +187 to +198
// This is a very miserable pile of hacks...
//
// `Expr::needs_parens_in` requires that the expression is the child of the other expression,
// that is supposed to be its parent.
//
// But we want to check what would happen if we add `*`/`.*` to the inner expression.
// To check for inner we need `` expr.needs_parens_in(`*expr`) ``,
// to check for outer we need `` `*expr`.needs_parens_in(parent) ``,
// where "expr" is the `expr` parameter, `*expr` is the editted `expr`,
// and "parent" is the parent of the original expression...
//
// For this we utilize mutable mutable trees, which is a HACK, but it works.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ye I am not fond of this at all to be honest :) But I also don't really want to block the PR on this since I can't think of a better way right now either ...
Can you put a FIXME on this/on needs_parens_in API that this is not ideal? Or maybe open an issue about that, either works.

@Veykril
Copy link
Member

Veykril commented Jan 2, 2023

@bors delegate+

@bors
Copy link
Contributor

bors commented Jan 2, 2023

✌️ @WaffleLapkin can now approve this pull request

@bors
Copy link
Contributor

bors commented Jan 2, 2023

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

@Veykril
Copy link
Member

Veykril commented Jan 3, 2023

#13886 fixes the ligature problem

@rust-cloud-vms rust-cloud-vms bot force-pushed the postfix_adjustment_hints branch from be22568 to a9676cf Compare January 9, 2023 13:35
@WaffleLapkin
Copy link
Member Author

@bors r=Veykril

@bors
Copy link
Contributor

bors commented Jan 9, 2023

📌 Commit b6169c2 has been approved by Veykril

It is now in the queue for this repository.

@bors
Copy link
Contributor

bors commented Jan 9, 2023

⌛ Testing commit b6169c2 with merge ec96819...

@bors
Copy link
Contributor

bors commented Jan 9, 2023

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

1 similar comment
@bors
Copy link
Contributor

bors commented Jan 9, 2023

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

@bors
Copy link
Contributor

bors commented Jan 9, 2023

👀 Test was successful, but fast-forwarding failed: 422 Changes must be made through a pull request.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants