Add number separator related assists #3683

vbfox · 2020-03-22T17:22:09Z

This PR proposes 2 new assists, inspired by the identical ones in jetbrains C# IDEs

Remove separators that remove all '_' from a number literal
Separate number by thousands/bytes/nibbles with separator

I also did a few technical changes around the assist code:

Add add_assists that has the same syntax as adding a group but without assigning a group to the returned assists.
Add find_covering_node_at_offset to avoid cases where the selection start inside a number literal but ends further (like at the end of the file) triggering the assist.
Test helpers to allow testing AssistHandler implementations that can return multiple assists

Remarks/Questions:

I didn't set the cursor position explicitly as doing it right inside the code would complexify it greatly. At least on VSCode the final position is very good (And the LSP doesn't have the cursor position in TextEdit AFAIK), I suspect there is a diff algorithm applied somewhere to do a best guess of the cursor position. If specifying the cursor position is needed I'd prefer to PR an a similar feature in rust-analyzer than implementing it manually in analyzers.
After implementing it I remarked that jetbrains version add a '_' character between the prefix and the digits for cases where there is a prefix (binary, hex) like 0x_FF00_FF00 I don't have an opinion about it but if anyone has a preference...
There is no separator insertion for octal numbers

Note: I plan to add also PR the conversion between bases at some point.

This add 2 assists * Remove separators that remove all '_' from a number literal * Separate number by thousands/bytes/nibbles with separator

bjorn3 · 2020-03-22T17:23:56Z

crates/ra_assists/src/handlers/number_representation.rs

+            r#"fn f() { let x = <|>0b001_0101_000_101_010; }"#,
+            r#"fn f() { let x = <|>0b00101010_00101010; }"#,
+        )
+    }


That's a lot of tests :)

I'm not sure all the combinations are useful in the end they are essentially the same tests applied to all supported representations, can remove some if needed :)

yeah, I think it might make sense to remove some of the boilerplate here with a table driven test:

for (before, after) in [("0b001_0101_000_101_010", "0b00101010_00101010") ... ]

Veetaha

Looks very nice!

crates/ra_assists/src/handlers/number_representation.rs

From PR suggestion Co-Authored-By: Veetaha <[email protected]>

matklad

I didn't set the cursor position explicitly

Yeah, I think most of the assists shouldn't actually specify cursor position. We currently over-specify it, because our bulit-in diffing is imprecise and because (I think) VS Code used to not be so good at determining the cursor position afterwards.

To be clear, there are absolutelly assists where we want to place cursor smarter than the default (add derive, for example), but most of the boring transformations should be fine as is.

matklad · 2020-03-23T11:04:19Z

crates/ra_assists/src/handlers/number_representation.rs

+            r#"fn f() { let x = <|>0b001_0101_000_101_010; }"#,
+            r#"fn f() { let x = <|>0b00101010_00101010; }"#,
+        )
+    }


yeah, I think it might make sense to remove some of the boilerplate here with a table driven test:

for (before, after) in [("0b001_0101_000_101_010", "0b00101010_00101010") ... ]

matklad · 2020-03-23T11:14:52Z

crates/ra_assists/src/handlers/number_representation.rs

+}
+
+#[derive(Clone, Debug)]
+struct NumberLiteral {


This is a pretty and general-purpose utility, so I think it's should live in ra_synax. I think the right API would be as follows:

add pub struct IntNumber(SyntaxToken); to tokens.rs

suffix/preffix methods could return Option<&str> then. I think ideally we might want even fn suffix_range() -> Option<TextRange>, fn suffix_text() -> Option<&str>, but its ok to not add anything beyond this PR.

ideally, duplicaiton with LiteralKind should be removed. Perhaps it could return an enum of tokens, instead of a lowered enum? But that, again, is probably out of scope of this change.

The main theme of the API we want is that, as long as possible, we should just give "views" into existing syntax nodes and tokens, rather than converting them into a POD rust types. Sort of the opposite of what you'd do in a traditional compiler.

matklad · 2020-03-23T11:20:58Z

crates/ra_assists/src/assist_ctx.rs

    }
 }

+pub(crate) struct AssistVec<'a> {


yup, this API looks good to me!

matklad · 2020-03-23T11:22:30Z

crates/ra_assists/src/handlers/number_representation.rs

+    str.replace("_", "")
+}
+
+pub(crate) fn remove_digit_separators(ctx: AssistCtx) -> Option<Assist> {


Let's put pub(crate) functions on top, and add "doc comments" to them (see other assists). These comments then go into auto-generated docs. No need to document every assist, but at least a single example would be useful.

matklad · 2020-04-21T10:25:56Z

ping @vbfox , in case this just accidently fell of from your radar :)

matklad · 2020-05-05T13:09:23Z

closing due to inactivity, but feel free to reopen!

10998: Add number representation assists r=Veykril a=errx Reimplemented assists from this PR #3683 with current APIs. ![image](https://user-images.githubusercontent.com/462486/145726792-47700215-26f2-4fdc-9520-63d1487901e5.png) ![image](https://user-images.githubusercontent.com/462486/145726802-f528a2f7-9159-41d3-b459-fc3fae033e60.png) I've decided not to add options about size of the groups so behaviour is similar to clippy's. Minimal number length is also taken from clippy. Co-authored-by: Oleg Matrokhin <[email protected]>

tell people how to set miri flags fixes rust-lang/miri#3677

Add number separator related assists

6576d2a

This add 2 assists * Remove separators that remove all '_' from a number literal * Separate number by thousands/bytes/nibbles with separator

bjorn3 reviewed Mar 22, 2020

View reviewed changes

Veetaha reviewed Mar 22, 2020

View reviewed changes

crates/ra_assists/src/handlers/number_representation.rs Show resolved Hide resolved

crates/ra_assists/src/handlers/number_representation.rs Outdated Show resolved Hide resolved

Use filter instead of manual code

08a082e

From PR suggestion Co-Authored-By: Veetaha <[email protected]>

matklad reviewed Mar 23, 2020

View reviewed changes

matklad closed this May 5, 2020

lnicola mentioned this pull request Oct 6, 2020

Add assist for converting the base of integer literals. #6111

Merged

errx mentioned this pull request Dec 12, 2021

Add number representation assists #10998

Merged

lnicola pushed a commit to lnicola/rust-analyzer that referenced this pull request Jul 16, 2024

Auto merge of rust-lang#3683 - RalfJung:MIRIFLAGS, r=RalfJung

f862aa8

tell people how to set miri flags fixes rust-lang/miri#3677

Add number separator related assists #3683

Add number separator related assists #3683

Uh oh!

Conversation

vbfox commented Mar 22, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Veetaha left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

matklad left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

matklad commented Apr 21, 2020

Uh oh!

matklad commented May 5, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

vbfox commented Mar 22, 2020 •

edited

Loading