Refactored runtime/ftplugin/zig.vim, removed upstream comment, and removed aucmd and auto formatting support.

[Tiseno]

https://github.com/ziglang/zig.vim added auto formatting and linting (and population of the qflist) on save as a aucmd with an opt-out configuration, and was then merged into vim runtime. This seems to me like something that should be left up to that plugin and not be a part of vim.

It should probably be:
opt-in, as it is very unexpected behavior and uncharacteristic of vim to do something like this automatically
or removed entirely, as it should be left up to the user to register that kind of behavior if they want it through a plugin.
This PR does the latter.

I could not find any concrete guidelines of what built in file type plugins should and should not do, I am personally fine with whatever, opt-in, or remove the format/astcheck functionality. But in any case I think it should not register any aucmds by default without opting in to it (i.e. the check for opt in should be for registering the aucmd, not for running the formatter or not). I have noted that the rust filetype support also has a lot of options available as opt-in, and it also always registers an aucmd.
Is this the direction vim is moving in? i.e. upstreaming oppinionated plugins for integrating languages tightly?
My expectation before going down this rabbit hole was that built-in language supports only provided: filetype, filetype detection, syntax, indentation, and a makeprg.

The zig.vim plugin wants to be an ide-like plugin, and this PR removes the upstream dependency for the ftplugin implementation. I also made some consistency changes to the code in how undo_ftplugin is handled and added the zig_recommended_style as consistent with other ftplugins. The only significant thing left is the g:zig_std_dir/path and execution of zig env, which might also be up for discussion if it should be removed.

[habamax]

I believe autocmd that does autoformat should absolutely not happen by default.

[habamax]

I would also avoid all g:zig_std_dir set up:

" Safety check: don't execute zig from current directory
if !exists('g:zig_std_dir') && exists('*json_decode') &&
    \  executable('zig') && dist#vim#IsSafeExecutable('zig', 'zig')
    silent let s:env = system('zig env')
    if v:shell_error == 0
        let g:zig_std_dir = json_decode(s:env)['std_dir']
    endif
    unlet! s:env
endif

On windows gvim system() call looks ugly and you probably don't want to see cmd.exe window popup upon opening zig file.

Should be either opt-in or just a ftplugin help entry on how to set up g:zig_std_dir

[dkearns]

Is there a reason not to send your more general changes through the maintainer's repo?

[habamax]

Is there a reason not to send your more general changes through the maintainer's repo?

They might disagree with the proposed changes, but yeah it needs to be addressed there too(first?).

PS,
Do we have vim runtime files guide? What to avoid in ftplugin/syntax files?

[Tiseno]

The general changes can be added to the zig.vim plugin after the fact, if they want them.
This is a proposal to split from that plugin where vim maintains its own ftplugin implementation (as zig.vim wants to do things that should probably not be in the vim runtime).

And please note that I am only referring to runtime/ftplugin/zig.vim not the syntax/indentation/compiler implementations, I see no problem with those, so they can still be upstreamed.

[Tiseno]

Just as an example, the go ft plugin was split into two, one bare bones and one more ide-like.
https://github.com/google/vim-ft-go
https://github.com/fatih/vim-go
The former one was merged into vim and is now obsolete.
zig.vim is more comparable to the latter in what it wants to be.

Edit: And again, the rust ftplugin implementation also has exactly the same problem as noted. We have merged a lot of special and opinionated support for rust fmt and cargo, which I don't believe should be a part of the vim runtime either.
But that implementation at least has sensible opt-in defaults (I haven't checked all the options).
But it still always registers an auto command on save even when not opting in which seems wrong (prior to fc93594d562dbbd9da03c89754538f91efd0c7ca it did not).

[flexagoon]

What is currently blocking this PR? Is it just the undocumented variable?

[chrisbra]

yes

[flexagoon]

@Tiseno could you please add the missing doc section? This is a blocking PR for neovim/neovim#28878

[gpanders]

Here is a patch for the docs:

diff --git a/runtime/doc/filetype.txt b/runtime/doc/filetype.txt
index deb947cf1..feb946252 100644
--- a/runtime/doc/filetype.txt
+++ b/runtime/doc/filetype.txt
@@ -829,6 +829,12 @@ The mappings can be disabled with: >
 	let g:no_vim_maps = 1
 
 
+ZIG							*ft-zig-plugin*
+
+						*g:zig_recommended_style*
+The Zig |ftplugin| sets options according to Zig's recommended style. This can
+be disabled by setting |g:zig_recommended_style| to 0.
+
+
 ZIMBU							*ft-zimbu-plugin*
 
 The Zimbu filetype plugin defines mappings to move to the start and end of

I put it in doc/filetype.txt rather than doc/syntax.txt since the variable is relevant for the ftplugin, not the syntax file.

[chrisbra]

Thanks, makes sense. I'll have a look later today

[Tiseno]

The doc comment!
But there is also the matter of the zig_std_dir, which I would like to remove because it also seems intrusive.
But I do not even know fully what its purpose is.

[gpanders]

But there is also the matter of the zig_std_dir, which I would like to remove because it also seems intrusive. But I do not even know fully what its purpose is.

It is used to set the 'path' variable, which is useful for e.g. gf, :find, [I, etc. If the ftplugin is setting 'include', 'includexpr', and 'define' already, then setting 'path' is probably a useful thing to do. And gating it behind the existence of g:zig_std_dir (so that users can just set it themselves) seems harmless enough.

[Tiseno]

Ok, seems good to me, but is the implementation good enough?
@habamax pointed out

On windows gvim system() call looks ugly and you probably don't want to see cmd.exe window popup upon opening zig file.
Should be either opt-in or just a ftplugin help entry on how to set up g:zig_std_dir

But maybe we can leave that to another time.

[gpanders]

Ok, seems good to me, but is the implementation good enough? @habamax pointed out

On windows gvim system() call looks ugly and you probably don't want to see cmd.exe window popup upon opening zig file.
Should be either opt-in or just a ftplugin help entry on how to set up g:zig_std_dir

We don't need to shell out or call system(). If the user sets g:zig_std_dir themselves, the ftplugin can use it to set 'path'. Otherwise leave 'path' alone. But g:zig_std_dir probably needs to be included in the docs too.

[chrisbra]

where is this system() call?

[gpanders]

@chrisbra The original implementation (upstream) has a system() call (system('zig env')). The question was (I believe) whether this should be preserved or removed.

[chrisbra]

Ah, okay, let's leave it out for now. I am not a big fan of running system commands just by loading a filetype. Just document that g:zig_std_dir can be set by system('zig env') in a custom filetype auto command

[gpanders]

Just document that g:zig_std_dir can be set by system('zig env') in a custom filetype auto command

Specifically, the std_dir field from the output of zig env. If we want a one-liner, it's :echo json_decode(system('zig env'))['std_dir'].

[Tiseno]

Ah, okay, let's leave it out for now. I am not a big fan of running system commands just by loading a filetype. Just document that g:zig_std_dir can be set by system('zig env') in a custom filetype auto command

Now I just declared that one can put it in a aucmd, and linked to aucmd, but do we want a full example for that instead?
How would one actually do that in the most idiomatic way?

[chrisbra]

thanks all!