Problem:
Dirs "config", "packaging", and "third-party" are all closely related
but this is not obvious from the layout. This adds friction for new
contributors.
Solution:
- rename config/ to cmake.config/
- rename test/config/ to test/cmakeconfig/ because it is used in Lua
tests: require('test.cmakeconfig.paths').
- rename packaging/ to cmake.packaging/
- rename third-party/ to cmake.deps/ (parallel with .deps/)
Problem:
RunTests.cmake adds $TEST_PATH to $TMPDIR with the implication that it
gives more isolation. But this is misleading because $TEST_PATH is only
defined once. Full test runs use the same $TMPDIR for all tests.
This was likely added with the intention of invoking RunTests.cmake
once-per-testfile from a wrapper than does the isolation/orchestration.
But even so, Nvim's vim_maketempdir() / vim_mktempdir() _already_
creates a unique tempdir per session.
Solution:
Don't append $TEST_PATH to $TMPDIR. Avoids confusion and makes the path
shorter.
Avoids this warning in the Windows build:
2022-06-22T08:58:13.0542153Z LINK : warning LNK4044: unrecognized option '/municode'; ignored [D:\a\neovim\neovim\build\test\functional\fixtures\printenv-test.vcxproj]
Problem:
1. CI logs have too many (40+) logs mentioning SIGHUP:
```
WRN 2022-06-18T16:05:47.075 T3568.22499.0/c deadly_signal:177: got signal 1 (SIGHUP)
WRN 2022-06-18T16:05:47.273 T3569.91095.0/c deadly_signal:177: got signal 1 (SIGHUP)
WRN 2022-06-18T16:05:47.651 T3570.59545.0/c deadly_signal:177: got signal 1 (SIGHUP)
```
2. TS parser test still sometimes fails on BSD CI.
3. remote_spec test fails too often.
Solution:
1. Log deadly signals at INFO level. It hasn't been helpful in CI, and
for local troubleshooting it's reasonable to adjust the loglevel as
needed.
2. Adjust the TS parser test again. ref #18911
3. Skip the remote_spec test. The `--remote` feature was merged before
it was fully formed and needs to be revisited.
luacheck and clint are the most important, and they are bundled.
The others are nice-to-have, and not always available (and not bundled),
so make them optional. This allows the "lint" target to function as
a convenient and low-hassle handle for contributors.
We still get the full power of lint in CI.
TODO: bundle uncrustify (and then set it as required for "lint")?
Problem:
Coverity reports use after free:
*** CID 352784: Memory - illegal accesses (USE_AFTER_FREE)
/src/nvim/buffer.c: 1508 in set_curbuf()
1502 if (old_tw != curbuf->b_p_tw) {
1503 check_colorcolumn(curwin);
1504 }
1505 }
1506
1507 if (bufref_valid(&prevbufref) && prevbuf->terminal != NULL) {
>>> CID 352784: Memory - illegal accesses (USE_AFTER_FREE)
>>> Calling "terminal_check_size" dereferences freed pointer "prevbuf->terminal".
1508 terminal_check_size(prevbuf->terminal);
1509 }
1510 }
1511
1512 /// Enter a new current buffer.
1513 /// Old curbuf must have been abandoned already! This also means "curbuf" may
Solution:
Change terminal_destroy and terminal_close to set caller storage to NULL,
similar to XFREE_CLEAR. This aligns with the pattern found already in:
terminal_destroy e897ccad3e
term_delayed_free 3e59c1e20d
Problem:
If startup finishes (starting=false) before the logger ever happens to
see a v:servername, we're stuck with the "?.<PID>" fallback name
forever.
Solution:
Drop the `starting` condition. Discard the "?.<PID>" fallback after
using it for the current log message. So logging will always check
v:servername next time.
Problem:
- Unix sockets are created in random /tmp dirs.
- /tmp is messy, unclear when OSes actually clear it.
- The generated paths are very ugly. This adds friction to reasoning
about which paths belong to which Nvim instances.
- No way to provide a human-friendly way to identify Nvim instances in
logs or server addresses.
Solution:
- Store unix sockets in stdpath('state')
- Allow --listen "name" and serverstart("name") to given a name (which
is appended to a generated path).
TODO:
- is stdpath(state) the right place?
Problem:
1. Log messages (especially in CI) are hard to correlate with tests.
2. Since b353a5c05f#11886, dumplog() prints the logs next to test
failures. This is noisy and gets in the way of the test results.
Solution:
1. Associate an incrementing id with each test and include it in log
messages.
- FUTURE: add v:name so Nvim instances can be formally "named"?
2. Mention "child" in log messages if the current Nvim is a child (based
on the presence of $NVIM).
BEFORE:
DBG … 12345 UI: event
DBG … 12345 log_server_msg:722: RPC ->ch 1: …
DBG … 12345 UI: flush
DBG … 12345 inbuf_poll:444: blocking... events_enabled=1 events_pending=0
DBG … 23454 UI: stop
INF … 23454 os_exit:594: Nvim exit: 0
AFTER:
DBG … T57 UI: event
DBG … T57 log_server_msg:722: RPC ->ch 1: …
DBG … T57 UI: flush
DBG … T57 inbuf_poll:444: blocking... events_enabled=1 events_pending=0
DBG … T57/child UI: stop
INF … T57/child os_exit:594: Nvim exit: 0
Problem:
Since 6d57bb89c1#18543, luacheck is not found on some systems when
running the "lintlua" target.
Solution:
- Move the find_program() to the top-level CMakeLists.txt
- Define a def_cmd_target() function with fewer assumptions than the old
lint() function.
- Move "lintuncrustify" to src/nvim/CMakeLists.txt so it can reuse the
$LINT_NVIM_SOURCES already defined there.
- Make the lint targets _fatal_ by default. There is little reason for
the "lint" umbrella target defined in Makefile to exist if it's going
to ignore the absence of the actual linters.
- For now, keep the uncrustify call in a separate cmake script so that
it can be silenced (too noisy).
The "first run" has high variability. Looks like the test failures
correlate with 545dc82c1b
, which makes sense because that improves "first run" performance.
So the `1000*` factor of this test could be adjusted to e.g. `300*` or `500*`.
ref https://github.com/neovim/neovim/pull/16945
Problem:
Since 22b52dd462#11501, log_path_init is called in log_init, so it is
now called at a deterministic time. So the "just in time" complexity of
log_path_init is no longer needed.
Solution:
Remove logic intended to try to "heal" partial initialization.
Problem:
1. The main log routine does not protect itself against recursion.
log_lock() doesn't guard against recursion, it would deadlock...
2. 22b52dd462 (#11501) regressed 6f27f5ef91 (#10172), because
set_init_1..process_spawn tries to log (see backtrace below), but the
mutex isn't initialized yet. Even if the mutex were valid, we don't
want early logging to fallback to stderr because that can break
embedders when stdio is used for RPC.
frame 1: 0x00000001001d54f4 nvim`open_log_file at log.c:205:7
frame 2: 0x00000001001d5390 nvim`logmsg(log_level=1, context="UI: ", func_name=0x0000000000000000, line_num=-1, eol=true, fmt="win_viewport") at log.c:150:20
frame : 0x000000010039aea2 nvim`ui_call_win_viewport(grid=2, win=1000, topline=0, botline=1, curline=0, curcol=0, line_count=1) at ui_events_call.generated.h:321:3
frame 4: 0x00000001003dfefc nvim`ui_ext_win_viewport(wp=0x0000000101816400) at window.c:939:5
frame 5: 0x00000001003ec5b4 nvim`win_ui_flush at window.c:7303:7
frame 6: 0x00000001003a04c0 nvim`ui_flush at ui.c:508:3
frame 7: 0x00000001002966ba nvim`do_os_system(argv=0x0000600000c0c000, input=0x0000000000000000, len=0, output=0x0000000000000000, nread=0x00007ff7bfefe830, silent=false, forward_output=false) at shell.c:894:3
frame 8: 0x0000000100295f68 nvim`os_call_shell(cmd="unset nonomatch; vimglob() { while [ $# -ge 1 ]; do echo \"$1\"; shift; done }; vimglob >/var/folders/gk/3tttv_md06987tlwpyp62jrw0000gn/T/nvimwwvwfD/0 ~foo", opts=kShellOptExpand | kShellOptSilent | kShellOptHideMess, extra_args=0x0000000000000000) at shell.c:663:18
frame 9: 0x0000000100295845 nvim`call_shell(cmd="unset nonomatch; vimglob() { while [ $# -ge 1 ]; do echo \"$1\"; shift; done }; vimglob >/var/folders/gk/3tttv_md06987tlwpyp62jrw0000gn/T/nvimwwvwfD/0 ~foo", opts=kShellOptExpand | kShellOptSilent | kShellOptHideMess, extra_shell_arg=0x0000000000000000) at shell.c:712:14
frame 10: 0x0000000100294c6f nvim`os_expand_wildcards(num_pat=1, pat=0x00007ff7bfefeb20, num_file=0x00007ff7bfefee58, file=0x00007ff7bfefee60, flags=43) at shell.c:328:7
...
frame 23: 0x000000010028ccef nvim`expand_env_esc(srcp=",~foo", dst="~foo", dstlen=4094, esc=false, one=false, prefix=0x0000000000000000) at env.c:673:17
frame 24: 0x000000010026fdd5 nvim`option_expand(opt_idx=29, val=",~foo") at option.c:1950:3
frame 25: 0x000000010026f129 nvim`set_init_1(clean_arg=false) at option.c:558:19
frame 26: 0x00000001001ea25e nvim`early_init(paramp=0x00007ff7bfeff5f0) at main.c:198:3
frame 27: 0x00000001001ea6bf nvim`main(argc=1, argv=0x00007ff7bfeff848) at main.c:255:3
Solution:
1. Check for recursion, show "internal error" message.
- FUTURE: when "remote TUI" is merged, can we remove log_lock()?
2. Skip logging if log_init wasn't called yet.
In scripts/vim-patch.sh line 335:
printf '
^-- SC2183 (warning): This format string has 4 variables, but is passed 3 arguments.
In scripts/vim-patch.sh line 597:
list_missing_vimpatches 1 "$@" | while read -r vim_commit; do
^--------^ SC2030 (info): Modification of vim_commit is local (to subshell caused by pipeline)
In scripts/vim-patch.sh line 626:
done < <(git -C "${VIM_SOURCE_DIR}" diff-tree --no-commit-id --name-only -r "${vim_commit}" -- . ':!src/version.c')
^-----------^ SC2031 (info): vim_commit was modified in a subshell. That change might be lost.
For more information:
https://www.shellcheck.net/wiki/SC2183 -- This format string has 4 variable...
https://www.shellcheck.net/wiki/SC2030 -- Modification of vim_commit is loc...
https://www.shellcheck.net/wiki/SC2031 -- vim_commit was modified in a subs...
vim-patch:8.0.1119: quitting a split terminal window kills the job
N/A, or tracked in https://github.com/neovim/neovim/issues/5431
vim-patch:8.0.1307: compiler warning for ignoring return value
N/A
vim-patch:8.0.1335: writefile() using fsync() may give an error
N/A
vim-patch:8.0.1339: no test for what 8.0.1335 fixes
already merged in 5972ff0056
vim-patch:8.0.1367: terminal test hangs, executing abcde
N/A
vim-patch:8.0.1562: the terminal debugger can't set breakpoint with mouse
we have all :Termdebug changes
vim-patch:8.0.1609: shell commands in the GUI use a dumb terminal
tracked in https://github.com/neovim/neovim/issues/5431
vim-patch:8.0.1616: Win32: shell commands in the GUI open a new console (guioptions="!")
tracked in https://github.com/neovim/neovim/issues/1496
vim-patch:8.0.1706: cannot sent CTRL-\ to a terminal window
already possible via :call chansend()
PROBLEM
------------------------------------------------------------------------
$NVIM_LISTEN_ADDRESS has conflicting purposes as both a parameter ("the
current process should listen on this address") and a descriptor ("the
current process is a child of this address").
This contradiction means the presence of NVIM_LISTEN_ADDRESS is
ambiguous, so child Nvim always tries to listen on its _parent's_
socket. This is the cause of lots of "Failed to start server" spam in
our test/CI logs:
WARN 2022-04-30… server_start:154: Failed to start server: address already in use: \\.\pipe\nvim-4480-0
WARN 2022-04-30… server_start:154: Failed to start server: address already in use: \\.\pipe\nvim-2168-0
SOLUTION
------------------------------------------------------------------------
1. Set $NVIM to the parent v:servername, *only* in child processes.
- Now the correct way to detect a "parent" Nvim is to check for $NVIM.
2. Do NOT set $NVIM_LISTEN_ADDRESS in child processes.
3. On startup if $NVIM_LISTEN_ADDRESS exists, unset it immediately after
server init.
4. Open a channel to parent automatically, expose it as v:parent.
Fixes#3118Fixes#6764Fixes#9336
Ref https://github.com/neovim/neovim/pull/8247#issuecomment-380275696
Ref #8696
helpers.source() was a hack to work around the lack of anonymous
:source. Its "create tempfile" behavior is not a required part of most
tests that use it.
Some tests still need the old "create tempfile" behavior either because
they test SID behavior, or because of missing nvim_exec features: #16071
Problem:
1. "unpack" has an unrelated meaning in Lua:
https://www.lua.org/manual/5.1/manual.html#pdf-unpack
2. We already have msgpackparse()/msgpackdump() and
json_encode()/json_decode(), so introducing another name for the same
thing is entropy.
Solution:
- Rename vim.mpack.pack/unpack => vim.mpack.encode/decode
Caveat:
This is incongruent with the `Unpacker` and `Packer` functions.
- It's probably too invasive to rename those.
- They also aren't part of our documented interface.
- This commit is "reversible" in the sense that we can always revert
it and add `vim.mpack.encode/decode` as _aliases_ to
`vim.mpack.pack/unpack`, at any time in the future, if we want
stricter fidelity with upstream libmpack. Meanwhile,
`vim.mpack.encode/decode` is currently the total _documented_
interface of `vim.mpack`, so this change serves the purpose of
consistent naming in the Nvim stdlib.
PVS is worried about typos. Now we need it to stop worrying...
Disable these checks entirely, they are all false positives.
tui.c:1873 V1074 Boundary between escape sequence and string is unclear. The escape sequence ends with a letter and the next character is also a letter. Check for typos.
tui.c:1983 V1074 Boundary between escape sequence and string is unclear. The escape sequence ends with a letter and the next character is also a letter. Check for typos.
regexp_nfa.c:6189 V1051 Consider checking for misprints. It's possible that the 'pim->result' should be checked here.
screen.c:2928 V1051 Consider checking for misprints. It's possible that the 'vcol_sbr' should be checked here.
screen.c:3187 V1051 Consider checking for misprints. It's possible that the 'line_attr' should be checked here.
screen.c:3267 V1051 Consider checking for misprints. It's possible that the 'multi_attr' should be checked here.
screen.c:4747 V1051 Consider checking for misprints. It's possible that the 'redraw_next' should be checked here.
syntax.c:3448 V1051 Consider checking for misprints. It's possible that the 'arg_end' should be checked here.
syntax.c:3625 V1051 Consider checking for misprints. It's possible that the 'arg_end' should be checked here.
tui.c:1836 V1051 Consider checking for misprints. It's possible that the 'data->unibi_ext.set_cursor_style' should be checked here.
tui.c:1863 V1051 Consider checking for misprints. It's possible that the 'data->unibi_ext.set_cursor_style' should be checked here.
tui.c:1882 V1051 Consider checking for misprints. It's possible that the 'data->unibi_ext.set_cursor_style' should be checked here.
https://pvs-studio.com/en/docs/warnings/v507/
"Pointer to local array 'sourcing_name_buf' is stored outside the scope
of this array. Such a pointer will become invalid."
False positive: `sourcing_name = save_sourcing_name` before returning
from this scope.
Problem
- `redir_exec` is obsolete, but it keeps getting used in new tests
because people copy existing tests.
- Disadvantages of `redir_exec`:
- Captures extra junk before the actual error/message that we _want_ to test.
- Does not fail on error, unlike e.g. `command()`.
Solution
- Use new functions like `nvim_exec` and `pcall_err`.
Problem:
Subdirectories like "visual", "insert", "normal" encourage people to
separate *related* tests for no good reason. Typically the _mode_ is
not the relevant topic of a test (and when it is, _then_ create
an appropriate describe() or it()).
Solution:
- Delete the various `test/functional/<mode>/` subdirectories, move
their tests to more meaningful topics.
- Rename `…/normal/` to `…/editor/`.
- Move or merge `…/visual/*` and `…/insert/*` tests into here where
appropriate.
- Rename `…/eval/` to `…/vimscript/`.
- Move `…/viml/*` into here also.
* test(reorg): insert/* => editor/mode_insert_spec.lua
* test(reorg): cmdline/* => editor/mode_cmdline_spec.lua
* test(reorg): eval core tests => eval_spec.lua
## Overview
- Move vim.lsp.diagnostic to vim.diagnostic
- Refactor client ids to diagnostic namespaces
- Update tests
- Write/update documentation and function signatures
Currently, non-LSP diagnostics in Neovim must hook into the LSP subsystem. This
is what e.g. null-ls and nvim-lint do. This is necessary because none of the
diagnostic API is exposed separately from the LSP subsystem.
This commit addresses this by generalizing the diagnostic subsystem beyond the
scope of LSP. The `vim.lsp.diagnostic` module is now simply a specific
diagnostic producer and primarily maintains the interface between LSP clients
and the broader diagnostic API.
The current diagnostic API uses "client ids" which only makes sense in the
context of LSP. We replace "client ids" with standard API namespaces generated
from `nvim_create_namespace`.
This PR is *mostly* backward compatible (so long as plugins are only using the
publicly documented API): LSP diagnostics will continue to work as usual, as
will pseudo-LSP clients like null-ls and nvim-lint. However, the latter can now
use the new interface, which looks something like this:
```lua
-- The namespace *must* be given a name. Anonymous namespaces will not work with diagnostics
local ns = vim.api.nvim_create_namespace("foo")
-- Generate diagnostics
local diagnostics = generate_diagnostics()
-- Set diagnostics for the current buffer
vim.diagnostic.set(ns, diagnostics, bufnr)
```
Some public facing API utility methods were removed and internalized directly in `vim.diagnostic`:
* `vim.lsp.util.diagnostics_to_items`
## API Design
`vim.diagnostic` contains most of the same API as `vim.lsp.diagnostic` with
`client_id` simply replaced with `namespace`, with some differences:
* Generally speaking, functions that modify or add diagnostics require a namespace as their first argument, e.g.
```lua
vim.diagnostic.set({namespace}, {bufnr}, {diagnostics}[, {opts}])
```
while functions that read or query diagnostics do not (although in many cases one may be supplied optionally):
```lua
vim.diagnostic.get({bufnr}[, {namespace}])
```
* We use our own severity levels to decouple `vim.diagnostic` from LSP. These
are designed similarly to `vim.log.levels` and currently include:
```lua
vim.diagnostic.severity.ERROR
vim.diagnostic.severity.WARN
vim.diagnostic.severity.INFO
vim.diagnostic.severity.HINT
```
In practice, these match the LSP diagnostic severity levels exactly, but we
should treat this as an interface and not assume that they are the same. The
"translation" between the two severity types is handled transparently in
`vim.lsp.diagnostic`.
* The actual "diagnostic" data structure is: (**EDIT:** Updated 2021-09-09):
```lua
{
lnum = <number>,
col = <number>,
end_lnum = <number>,
end_col = <number>,
severity = <vim.diagnostic.severity>,
message = <string>
}
```
This differs from the LSP definition of a diagnostic, so we transform them in
the handler functions in vim.lsp.diagnostic.
## Configuration
The `vim.lsp.with` paradigm still works for configuring how LSP diagnostics are
displayed, but this is a specific use-case for the `publishDiagnostics` handler.
Configuration with `vim.diagnostic` is instead done with the
`vim.diagnostic.config` function:
```lua
vim.diagnostic.config({
virtual_text = true,
signs = false,
underline = true,
update_in_insert = true,
severity_sort = false,
}[, namespace])
```
(or alternatively passed directly to `set()` or `show()`.)
When the `namespace` argument is `nil`, settings are set globally (i.e. for
*all* diagnostic namespaces). This is what user's will typically use for their
local configuration. Diagnostic producers can also set configuration options for
their specific namespace, although this is generally discouraged in order to
respect the user's global settings. All of the values in the table passed to
`vim.diagnostic.config()` are resolved in the same way that they are in
`on_publish_diagnostics`; that is, the value can be a boolean, a table, or
a function:
```lua
vim.diagnostic.config({
virtual_text = function(namespace, bufnr)
-- Only enable virtual text in buffer 3
return bufnr == 3
end,
})
```
## Misc Notes
* `vim.diagnostic` currently depends on `vim.lsp.util` for floating window
previews. I think this is okay for now, although ideally we'd want to decouple
these completely.
Problem:
13748512f6#15610 The no-colors codepath of the nvim.lua test output
handler does not handle nil, leading to weird symptoms if e.g. a test
has a syntax error:
test/busted/outputHandlers/nvim.lua:105: attempt to concatenate a nil value
Solution:
Coerce to string in no-colors handler.
Problem:
- If I run 'make test' on Linux (Fedora), a few lines like
/path/to/neovim/build/bin/shell-test REP 31 line
/path/to/neovim/build/bin/shell-test REP 41 line
are written to my ~/.bash_history.
These comes from from test/functional/terminal/scrollback_spec.lua.
- If $HISTFILE is unset, shell will not write to history file.
But bash sets $HISTFILE to default value (~/.bash_history) if it is unset.
- Unknown how to set an env var to empty string in CMake. These do NOT work:
set(HISTFILE "")
set(ENV{HISTFILE} "")
unset(ENV{HISTFILE})
Solution:
Set HISTFILE=/dev/null
Problem
-------
Because test/busted/outputHandlers/nvim.lua doesn't know if it's running
in a terminal (no "isatty" equivalent), it outputs color codes in CI
logs and local tooling that runs the tests in a pipe:
[1m[33m[ SKIPPED ][0m[0m [36m
This is just noise, hard for humans to read.
Solution
--------
Disable the color codes. If we later find a clever way to detect
a terminal in nvim.lua, we might consider re-enabling colors, but that
would still affect the CI build logs...
Analogous to nodejs's `on('data', …)` interface, here on_key is the "add
listener" interface.
ref 3ccdbc570d#12536
BREAKING_CHANGE: vim.register_keystroke_callback() is now an error.
Set a maximum test run-time of 20 minutes to:
- fail faster
- avoid wasting CI resources
- set a bound on the test time (if tests take longer than 20 minutes we
need to invest in parallelizing them...)
Timeout looks like:
-- Tests exited non-zero: Process terminated due to timeout
-- No output to stderr.
CMake Error at /…/neovim/cmake/RunTests.cmake:86
(message):
functional tests failed with error: Process terminated due to timeout
I mistakenly suggested maxlines=&cmdwinheight, forgetting that it is
calculated from topline, not cursor. maxlines=1 makes the most sense in
cmdwin.
ref #15401622a36b1f1
Problem:
"set filetype=man" assumes the user wants :Man features, this does extra
stuff like renaming the buffer as "man://".
Solution:
- old entrypoint was ":set filetype=man", but this is too presumptuous #15487
- make the entrypoints more explicit:
1. when the ":Man" command is run
2. when a "man://" buffer is opened
- remove the tricky b:man_sect checks in ftplugin/man.vim and syntax/man.vim
- MANPAGER is supported via ":Man!", as documented.
fixes#15487
- The previous commit lost information in the tests. Instead, add some
more "normalization" substitutions in pcall_err(), so that the general
shape of the stacktrace is included in the asserted text.
- Eliminate contains(), it is redundant with matches()
Seems like redundant env var separators (";" on Windows) in $PATH can
cause weird behavior. From #7377:
> After some time, system(['win32yank', '-o']) and system('win32yank -o')
> start returning different results: specifically first returns an
> empty string.
>
> 1. $PATH weirdly contains double semicolon followed by path to the
> “installation directory” (unpacked directory from archive).
> 2. If I run `let $PATH=substitute($PATH, ';;', ';', 'g')` the problem is fixed.
close#7377
ref 224f99b85d
Justin M. Keyes