Skip to content

chore: restore v fmt compliance on master - #528

Closed
metif12 wants to merge 1 commit into
vlang:masterfrom
metif12:fix/vfmt-master-failures
Closed

metif12 wants to merge 1 commit into
vlang:masterfrom
metif12:fix/vfmt-master-failures

Conversation

@metif12

@metif12 metif12 commented Oct 3, 2026

Copy link
Copy Markdown
Contributor

Problem

master fails the CI formatting gate on all three platforms, so no other CI step
runs at all:

build (ubuntu-latest):   X Check formatting
build (macos-latest):    X Check formatting
build (windows-latest):  X Check formatting

v fmt -verify vls/ reports 17 of the 24 .v files as unformatted:

call_hierarchy.v  compilation_test.v  diagnostics_scheduler.v  document_highlight.v
folding_range.v   handlers.v          handlers_test.v          index.v
index_test.v      integration_test.v  interop.v                interop_test.v
lsp.v             lsp_test.v          main.v                   run_command.v
semantic_tokens.v
Encountered a total of: 17 formatting errors.

See run 35838854684.

Cause

Formatting drifted as features landed — mostly struct field alignment going stale
when a field or trailing comment was added, plus a handful of call sites that no
longer fit on one line:

 struct LSPDiagnostic {
 	message  string
 	severity int
 	source   ?string @[json: 'source'] // diagnostic source identifier, e.g. 'vlang'
-	code     ?string @[json: 'code'] // optional diagnostic code, e.g. 'unused_variable'
-	tags     ?[]int @[json: 'tags'] // 1 = unnecessary, 2 = deprecated
+	code     ?string @[json: 'code']   // optional diagnostic code, e.g. 'unused_variable'
+	tags     ?[]int  @[json: 'tags']   // 1 = unnecessary, 2 = deprecated

The gate only runs on master and pull requests, so this is invisible until the
formatting step is reached.

Fix

Run v fmt -w . and commit the result. 1273 insertions, 1259 deletions across the
17 files. No semantic change: git diff -w reduces to 46 insertions / 32
deletions, all of it line reflow, e.g.

 fn build_v_line_info_args_multifile(rel_file string, line_info string) []string {
-	return ['-w', '-check', '-nocolor', '-vls-mode', '-line-info', '${rel_file}:${line_info}', '.']
+	return ['-w', '-check', '-nocolor', '-vls-mode', '-line-info', '${rel_file}:${line_info}',
+		'.']
 }

Formatter version

Produced with V 0137eb5 — the same vlang/v revision CI builds from, so this
does not introduce formatter-version churn. Verified with the same compiler:

v fmt -verify .   ->  exit 0

Validation

V 0137eb5, Windows.

  • v . builds clean.
  • compilation_test.v, interop_test.v, lsp_test.v: 3 passed, 3 total.
    These are the files that compile on master today.

index_test.v and handlers_test.v are excluded because they currently fail to
compile on master; that is fixed separately in
#526, and this branch is independent of
it.

@metif12

metif12 commented Oct 5, 2026

Copy link
Copy Markdown
Contributor Author

Consolidated rather than merged on its own.

handlers_test.v does not compile on master, so no branch that leaves that broken can
have a green CI run, and this change is what fixes it. Keeping it separate would have meant
every other PR stayed red.

Folded into #535, along with the v fmt compliance work, the handshake path escaping fix, the
dup2 outside assert fix, and a new fix for resolving vlib through a compiler wrapper
script that this change exposed.

Please review #535 instead. Nothing here is lost.

@metif12

metif12 commented Oct 5, 2026

Copy link
Copy Markdown
Contributor Author

Closed in favour of #535: v fmt compliance folded into #535 (green-build restoration).

@metif12 metif12 closed this Oct 5, 2026
medvednikov added a commit that referenced this pull request Oct 5, 2026
* fix: make index_max_file_bytes usable as a string repeat count

PR #521 changed index_max_file_bytes from int to u64 so it compares
directly against os.file_size. string.repeat still takes an int, so the
three oversized-file tests stopped compiling and the whole suite failed to
build on every platform. Convert at the call site and keep the constant
u64, which is what index.v compares against.

* fix: escape interpolated paths in the Sublime Text handshake test

test_integration_sublime_text_lsp_handshake hand-writes the initialize
payload to mimic a real client, then interpolates the native project path
straight into the rootPath JSON string. On Windows that path contains
backslashes, and \U is an unknown JSON escape, so json2.decode of the
params fails and on_initialize returns InvalidParams. received_initialize
never becomes true and the test fails on every Windows run.

Real clients escape those separators, so escape them here too. The
workspace_roots assertion now compares against the URI's own path form,
because roots are resolved from the folder URI and carry '/' separators
rather than the native backslashes.

* chore: restore v fmt compliance on master

* fix: recognize a launcher that cannot reach the V1 compatibility compiler

Hover, completion, signature help, and go to definition all go through
`v -vls-mode -line-info`, which the V launcher routes to the V1
compatibility compiler. When that compiler is missing, the launcher
refuses with a single line and exits:

    `-vls-mode` requires the compatibility compiler, but no usable V
    0.5.2 fallback was found and make is unavailable. Install make,
    then run `make v1` in `C:\Users\me\v`.

That refusal is not an "unknown option" line, so neither
`compiler_rejects_line_info` nor `compiler_refused_and_stopped`
recognized it. `line_info_mode` stayed `.direct`, so VLS kept spawning
the compiler once per request for an answer that can never arrive, and
every one of those lookups resolved to empty with nothing said about why.

Recognize the refusal, retire the lookups as `.missing` so they are
answered from VLS's own index instead of paying a process launch each,
and tell the user what to install. A launcher that can build the
fallback itself announces "running `make v1` now" and then answers, so
that form is explicitly not a dead end.

The notice needs no "already warned" flag: the caller sets
`line_info_mode` to `.missing` first, and from then on `run_v_line_info`
returns from its early `.missing` check without reaching this point, so
it is sent at most once per session.

* fix: keep the authority when encoding a UNC path as a file URI

`uri_to_path` already resolves a `file://host/...` URI to a `//host/share/...`
UNC path, but `path_to_uri` then treated that as an ordinary absolute path and
emitted four slashes:

    client sends   file://server/share/proj/main.v
    uri_to_path    //server/share/proj/main.v
    path_to_uri    file:////server/share/proj/main.v

The share ends up in the path instead of the authority. That is not a valid
file URI (RFC 8089 puts the host in the authority), and it breaks the round
trip in a way that matters: VLS keys open buffers, the index, and every
published diagnostic and code lens by URI, so the URI it derives for a file on
a network share never matches the one the client sent for the same file.

Encode the host as the authority instead:

    path_to_uri('//server/share/proj/main.v') == 'file://server/share/proj/main.v'

Percent-encoding of the path component is unchanged, so a share with a space
still encodes correctly and still round-trips. Single-slash absolute paths and
Windows drive paths are untouched, so POSIX and local Windows behaviour is
identical.

* fix: preserve CRLF line endings when formatting a document

`v fmt` always writes LF, on every platform including Windows. VLS formats by
writing the buffer to a temp file, running `v fmt -inprocess -w` on it, and
reading the result back, so a CRLF document came back with every CR stripped:

```
module main\r\n\r\nfn main() {\r\n\tprintln('hi')\r\n}\r\n
                        |
                 v fmt -inprocess -w
                        v
module main\n\nfn main() {\n\tprintln('hi')\n}\n
```

That output was then returned verbatim as a whole-document `TextEdit`. Two
problems follow, and the second is the worse one:

1. Format Document silently converts the file's line endings, so every line
   shows as changed and the diff is the entire file.
2. `format_content` returns no edits when the formatted text equals the input.
   With the CRs gone that comparison could never hold for a CRLF document, so
   VLS reported a change even for code that was already correctly formatted.

Restore the terminator the document already uses:

```v
formatted = restore_line_endings(content, formatted)
```

The terminator is taken from the document's first line break. An LF document is
untouched, output that already contains CRLF is not given a second CR, and a
document with no line break has no convention to preserve. A file with mixed
endings is normalized to its first ending, which is what a formatter that
respects the dominant convention does.

This is the same bug reported independently by users on Windows, where CRLF is
the default for `core.autocrlf` and for editors that preserve the file's
endings. The fix is platform-neutral: a CRLF document keeps CRLF on any
platform.

* test: recognise the reworded "make is unavailable" refusal too

vlang/v#29369 (merged as 76d88b9) rewrites the tail of the launcher's refusal:
the sentence after "make is unavailable" is now a platform-specific hint, and
the comma became a period. On Windows the clause reads

    On Windows, install GNU make in MSYS2 (`make` or `mingw32-make`) and put
    its tools, including `sh`, on PATH.

and elsewhere it is still "Install make.".

`compiler_lacks_compatibility_compiler` keys on "requires the compatibility
compiler", which both spellings contain, so detection is unaffected. This test
pinned the old wording verbatim, so it now asserts both: green before the
compiler change and after it, and the coupling is written down instead of being
rediscovered the next time the wording shifts.

Validated against V 0.5.2 76d88b9, the merged compiler: the Windows sample above
is its output verbatim. `interop_test.v` passes; the module suite reports the
same 3 failures with and without this change - `index_test.v`, `handlers_test.v`
and `integration_test.v`, none of them in this file. `integration_test.v` fails
on an empty completion list, which is the symptom of the missing V1 fallback
this refusal describes, and is what vlang/v#29369 and vlang/vscode-vlang#543 are
about.

* cgen: keep side effects out of `assert` conditions in stdio tests

`-prod` removes assert statements whole, as documented in doc/docs.md and
implemented in `vlib/v/gen/c/stmt.v`:

```v
.assert_stmt {
	if g.is_prod {
		return
	}
}
```

Two tests here used a side-effecting call *as* the assertion's condition:

```v
assert os.fd_dup2(transport.read_fd, 0) >= 0
```

That dup2 is the operation the test depends on, not a check on it. Under `-prod`
the statement disappears, fd 0 keeps pointing at the real stdin, and the read
that follows blocks on a descriptor nobody is going to write to. So:

- `v -prod test lsp_test.v` never terminated (>40 min for a file that takes 25s)
- `v -prod test integration_test.v` likewise

Perform the call outside the assert, matching how the neighbouring code already
does it (`os.fd_close(transport.write_fd)` a few lines above has no check either).

V's behaviour here is correct and is not changed by this commit; the defect was
in the tests. Verified on Windows with V 0137eb5:

- `v -prod test lsp_test.v` -> OK, 53s (was hanging)
- `v -prod test integration_test.v` -> OK, 62s (was hanging)
- `v -prod test .` -> completes in 76s, 4/6; the two failures are the unrelated
  `index_max_file_bytes` compile error that #526 fixes. Applying that fix on top
  gives `6 passed, 6 total` under `-prod`.
- Normal builds unchanged: `v test lsp_test.v` OK, and `integration_test.v`
  fails only on the pre-existing Sublime Text handshake bug that #527 fixes.

Reported upstream as vlang/v#29426, where the `-prod` codegen evidence is
included. That issue is closed as not-a-bug: the behaviour is documented and
matches C's `assert` under `NDEBUG`.

* ci: compile VLS with V3 on Windows too

V3 is the default backend on Linux and macOS, so the V3 compile step was the
only place its output was ever checked, and it was gated to `runner.os !=
'Windows'`. Windows keeps V1 as its default, so a V3-only codegen breakage could
sit on master until somebody built with `-new-compiler` by hand and reported it.

Dropping the condition covers all three platforms. The extra step is one compile
of a program this size, so it costs a fraction of the build time already spent.

Verified on Windows with V 0137eb5, using the same environment CI sets:

    $env:V_MACOS_V3_NO_FALLBACK = '1'
    v -no-memory-limit -nocache -new-compiler .    # exit 0

The separate Windows-only `Compile project` step stays, since that is the
backend Windows users actually get by default.

* ci: run the -prod test suite on Windows as well

`-prod` removes assert statements, and that silently changes the behaviour of any
test that performs work inside one. In this repository that reached Windows as a
hang rather than a failure: `test_stdio_reader_processes_frame_before_eof` and
`test_integration_stdio_initialize_completion_and_hover` both did

    assert os.fd_dup2(transport.read_fd, 0) >= 0

so under `-prod` the descriptor was never redirected and the following read
blocked forever. Nothing caught it because the `-prod` step was gated to Linux:

    - name: Run tests with production optimizations
      if: runner.os == 'Linux'

Two changes:

1. Add the same step for Windows. `V_MACOS_V3_NO_FALLBACK` is "0" to match the
   existing Windows test step, since Windows defaults to V1 and 0 permits
   fallback.

2. Put `timeout-minutes: 30` on both. A hang in a test step otherwise consumes the
   job's entire budget and reports nothing useful; a bounded step turns it into
   a red build that says which step stopped. The Linux step gets the same guard
   because it has the same failure mode. Drop that one line if you would rather
   keep the diff to Windows only.

The Windows `-prod` step deliberately does not set `VLS_VLANG_V_REPO`, so it does
not repeat the slow vlang/v workspace tests. Those are already covered on Windows
by the non-prod step above, which does set it. That also keeps the added CI time
to the ~1-2 minutes the suite takes locally under `-prod`, instead of the
considerably longer run the workspace tests add on top of `-prod`.

Verified on Windows with V 0137eb5:

    v -no-memory-limit -nocache -prod test vls/   ->  completes in 76s

**Ordering: this must land after #532.** Before that fix the two stdio tests
above hang under `-prod` on Windows, so this step goes red. With the timeout in
place it fails in a bounded, legible way instead of hanging, but it is still red.
The Windows job also needs #526 (the suite must compile) and #528 (the job
currently dies at `Check formatting`) to be green at all.

Practical merge order: #526, #527, #528, #532, then this.

* fix: resolve vlib through a compiler wrapper script

find_v_dir trusted the resolved compiler executable directory and returned it
unconditionally. V's own Windows launcher is a .bat wrapper kept in .bin/ that
forwards to the real v.exe one level up, so that directory contains no vlib at
all. Every caller builds <find_v_dir()>/vlib, so all of them silently resolved
to nothing: import completions returned an empty list, and hover and
go-to-definition stopped resolving vlib symbols.

find_v_dir_from_exe now walks up from the executable until it finds a directory
that actually contains vlib, bounded to eight levels. A directory with no vlib
anywhere on the way up now returns an empty string instead of a path whose vlib
does not exist.

Measured on Windows with v resolved to the wrapper: 7 of 454 handlers_test cases
failed before this change and all 454 pass after it, under identical conditions.
The same run also fixes 2 of the 3 failing integration cases that a stdio test
would otherwise have masked.

* test: canonicalize vlib discovery temporary roots

* fix: preserve working compiler fallback and CRLF range edits

* fix: stop vlib discovery at absolute filesystem roots

---------

Co-authored-by: metif12 <metif12@users.noreply.github.com>
Co-authored-by: Alexander Medvednikov <alexander@medvednikov.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant