zls icon indicating copy to clipboard operation
zls copied to clipboard

Fix/improve `union(enum)` completions

Open llogick opened this issue 1 year ago • 9 comments

Resolves #1707

It'd require some rework but should this be extended to anon struct inits? Should completions include = for struct fields?

llogick avatar Jan 17 '24 01:01 llogick

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 78.27%. Comparing base (7d6a9e2) to head (06e87d2).

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1719      +/-   ##
==========================================
+ Coverage   78.24%   78.27%   +0.03%     
==========================================
  Files          35       35              
  Lines       10512    10523      +11     
==========================================
+ Hits         8225     8237      +12     
+ Misses       2287     2286       -1     

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

codecov[bot] avatar Jan 17 '24 01:01 codecov[bot]

It'd require some rework but should this be extended to anon struct inits?

Could give an explanation on how completions would look like with anon struct inits?

Should completions include = for struct fields?

I would say yes otherwise it may be a bit unclear on what to type next.

Techatrix avatar Jan 17 '24 03:01 Techatrix

It'd require some rework but should this be extended to anon struct inits?

Could give an explanation on how completions would look like with anon struct inits?

I'm thinking it'd be identical, ieb.addExecutable(. would list the fields, just as it does with b.addExecutable(.{., but insert text would be { .field_name =.. how jarring would it be for .name to be { .field_name = ?

(If . and prev tok is = or ( offer to add the {)

~~Seems CodeCov didn't catch that last push, but it's there https://app.codecov.io/gh/zigtools/zls/pull/1719?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=checks&utm_campaign=pr+comments&utm_term=zigtools~~ nvm, it took it 2 edits

llogick avatar Jan 17 '24 05:01 llogick

im messing around with this branch a bit and noticed if i tab thru the list of completions it pastes the text including the snippet placeholders image this doesnt happen with any other types of snippets afaict, is this a bug? editor issue? ideally it would just fill the name of the variant when im tabbing then do the snippet on enter

xdBronch avatar Jan 17 '24 05:01 xdBronch

also why is this only done for tagged unions? regular unions are initialized the same way

xdBronch avatar Jan 17 '24 05:01 xdBronch

im messing around with this branch a bit and noticed if i tab thru the list of completions it pastes the text including the snippet placeholders image this doesnt happen with any other types of snippets afaict, is this a bug? editor issue? ideally it would just fill the name of the variant when im tabbing then do the snippet on enter

VSCode just selects whichever is highlighted :shrug:

llogick avatar Jan 17 '24 06:01 llogick

Hmm, hmm, hmm ... still undecided if the label should be name or { .name = label

llogick avatar Feb 06 '24 23:02 llogick

name imo, and then .{ . should also complete

nektro avatar Feb 06 '24 23:02 nektro

... .{ . should also complete

it does, as long as it is after the const exe = b.addExecutable(...

label2

The amount of times I've spent back and forth debugging those situations $#^#^@^@#

The regression you made me aware is analyser.resolveOptionalChildType not working as before, eg exe.libc_file = . assigning to optional should unwrap the type.

llogick avatar Feb 07 '24 00:02 llogick

@nullptrdevs I could take care of rebasing this PR if you'd like.

Techatrix avatar Feb 28 '24 15:02 Techatrix

@nullptrdevs I could take care of rebasing this PR if you'd like.

Yes, please do take over.

PS Dropping insertText would require some work to support incomplete fields completions.

llogick avatar Feb 29 '24 15:02 llogick

I've removed the second copy of every test that tested with and without snippets and instead used the new testCompletionTextEdit function to test completion is performing the expected text edit. FYI insertText gets converted into textEdit before sending the response.

Techatrix avatar Feb 29 '24 16:02 Techatrix

FYI insertText gets converted into textEdit before sending the response.

It was worth finding out, :+1:

llogick avatar Feb 29 '24 16:02 llogick

Does your approval stand on this one, merge?

llogick avatar Feb 29 '24 16:02 llogick

I have found an issue. In the following example I am doing a comparison which should only complete to alpha but completes to { .alpha = <cursor>}.

const Birdie = enum { canary };
const U = union(enum) { alpha: []const u8 };
const u: U = undefined;
const boolean = u == .<cursor>

Can be tested with:

try testCompletionTextEdit(.{
    .source =
    \\const U = union(enum) { alpha: []const u8 };
    \\const u: U = undefined;
    \\const boolean = u == .<cursor>
    ,
    .label = "alpha",
    .expected_insert_line = "const foo: U = .alphh",
    .expected_replace_line = "const foo: U = .alphh",
});

Techatrix avatar Feb 29 '24 16:02 Techatrix