Inlay hints for method chaining pattern #3710
Conversation
|
Thanks for your PR. I'm not the best person to review it, but I didn't see anything out of place. There's a conflict on |
There was a problem hiding this comment.
Looks really great, thank you!
I am checking for chaining on every ast::Expr in the tree; Are there performance concerns there?
IMO, no big concerns, especially after we make them calculated for the current screen only: #3394
It feels like we need another iteration on hints shortening:
But that's out of scope of this PR.
| toDecoration(hint: ra.InlayHint.ChainingHint, conv: lc.Protocol2CodeConverter): vscode.DecorationOptions { | ||
| return { | ||
| range: conv.asRange(hint.range), | ||
| renderOptions: { after: { contentText: ` ${hint.label}` } } |
There was a problem hiding this comment.
This may be controversial, but we might want to add a colon there so that it may remind the proposed type ascription syntax for rust? I'd wait on other people's opinions too though.
| renderOptions: { after: { contentText: ` ${hint.label}` } } | |
| renderOptions: { after: { contentText: `: ${hint.label}` } } |
There was a problem hiding this comment.
I did think about this, but it seemed to me that the reason for this colon is for this kind of inlay hint to exactly mimic the type syntax in a let statement. In the case of chaining there is no "real" syntax to mimic so I thought the simpler the better. Keen to hear others' opinions though.
There was a problem hiding this comment.
Type ascription is supported on nightly.
None: Option<()>;There was a problem hiding this comment.
I wonder if you can put a real type ascription on nightly between the chains of the method calls. I have a strong feeling that it will result in a syntax error, but bruh, the inlay hint must not always resemble a valid syntax.
There was a problem hiding this comment.
@matklad, @SomeoneToIgnore, @lnicola, any thoughts?
There was a problem hiding this comment.
error: casts cannot be followed by a method call
--> src/main.rs:4:13
|
4 | let a = vec![]: Vec<u8>.collect::<Vec<u8>>();
| ^^^^^^-^^^^^^^^
| |
| help: maybe write a path separator here: `::`
|There was a problem hiding this comment.
I like the way the hint is separated from the expression, but I agree that the text we display should sometimes be replaced with something different than Flatten<Chain<Chain<Once<Map<{unknown}, fn to_string<{unknown}>(&{unknown}) -> String>>, {unknown}>, Once<Option<String>>>>
There was a problem hiding this comment.
The ideal hint would be impl Iterator<Item = String> when the type is Flatten<Chain<Chain<Once<Map<{unknown}, fn to_string<{unknown}>(&{unknown}) -> String>>, {unknown}>, Once<Option<String>>>>.
| fn main() { | ||
| let c = A(B(C)) | ||
| .into_b() // This is a comment | ||
| .into_c(); |
There was a problem hiding this comment.
Hmm, I didn't have experience with chaining hints in Intelij, but shouldn't there be a hint for the last method call?
Though this may look weird since it will bias the semicolon and the type may be unnecessary duplicated as there is a variable type hint already.
There was a problem hiding this comment.
Indeed, the reason I didn't include the final method is because it would be often redundant information. Here, the type hint would show let c: C = .... IntelliJ does the same, I would guess for similar reasons:
M-J-Hooper
force-pushed
the
chaining-inlay-hints
branch
from
2621611 to
ab086e8
Compare
|
Thanks for the quick replies and feedback. I've rebased and made some amendments accordingly. |
|
CI's failing, looks like some test need adjusting |
M-J-Hooper
force-pushed
the
chaining-inlay-hints
branch
from
83c43e9 to
7b35da0
Compare
|
Excellent PR, and very easy to review, as everything is top notch! Thanks @M-J-Hooper ! bors r+ And yeah, we really should try to include this into the LSP proper... |
Build succeeded |
|
opened #3750 |
This PR adds inlay hints on method call chains:
It is not only explicit
MethodCalls where this can be helpful. The heuristic used here is that whenever any expression is followed by a new line and then a dot, it resembles a call chain and type information can be #useful.Changes:
InlayKindfor chaining.Notes:
ast::Exprin the tree; Are there performance concerns there?This is my first contribution (to RA and to Rust in general) so would appreciate any feedback.
The only issue I can find the references this feature is #2741.