vlang / v

Simple, fast, safe, compiled language for developing maintainable software. Compiles itself in <1s with zero library dependencies. Supports automatic C => V translation. https://vlang.io
MIT License
35.69k stars 2.15k forks source link

`-Wimpure-v` should apply to tests as well as compiling #20696

Open JalonSolov opened 7 months ago

JalonSolov commented 7 months ago

Describe the bug

Adding -Wimpure-v will give a warning if C code is used in plain .v files:

warning: C code will not be allowed in pure .v files, please move it to a .c.v file instead

However, this is only when compiling. When C code is used in a test file, there is no warning given.

Reproduction Steps

x.v

C.printf(c'Hi\n')

x_test.v

fn test_x() {
        C.printf(c'Hi\n')
}

Expected Behavior

Warnings for both .v and _test.v when -Wimpure-v is specified.

Current Behavior

$ v run x.v
Hi
$ v -Wimpure-v run x.v
x.v:1:1: warning: C code will not be allowed in pure .v files, please move it to a .c.v file instead
    1 | C.printf(c'Hi\n')
      | ^
x.v:1:3: warning: C code will not be allowed in pure .v files, please move it to a .c.v file instead
    1 | C.printf(c'Hi\n')
      |   ~~~~~~
Hi
$ v test x_test.v
---- Testing... --------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
 OK       3.759 ms /home/jalon/x_test.v
------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
Summary for all V _test.v files: 1 passed, 1 total. Elapsed time: 134 ms, on 1 job. Comptime: 129 ms. Runtime: 3 ms.
$ v -Wimpure-v test x_test.v
---- Testing... --------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
 OK       3.713 ms /home/jalon/x_test.v
------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
Summary for all V _test.v files: 1 passed, 1 total. Elapsed time: 132 ms, on 1 job. Comptime: 128 ms. Runtime: 3 ms.
$

Possible Solution

Test files with backend specific code can't be used when testing other backends, so they should be put in backend specific files.

Additional Information/Context

No response

V version

V 0.4.4 804a7bd

Environment details (OS name and version, etc.)

V full version: V 0.4.4 7008059.804a7bd
OS: linux, "Manjaro Linux"
Processor: 32 cpus, 64bit, little endian, AMD Ryzen 9 7950X 16-Core Processor

getwd: /home/jalon/git/v
vexe: /home/jalon/git/v/v
vexe mtime: 2024-01-29 18:44:42

vroot: OK, value: /home/jalon/git/v
VMODULES: OK, value: /home/jalon/.vmodules
VTMP: OK, value: /tmp/v_1000

Git version: git version 2.43.0
Git vroot status: weekly.2024.05-2-g804a7bdd
.git/config present: true

CC version: cc (GCC) 13.2.1 20230801
thirdparty/tcc status: json2_perf 12f392c3

[!NOTE] You can use the 👍 reaction to increase the issue's priority for developers.

Please note that only the 👍 reaction to the issue itself counts as a vote. Other reactions and those to comments will not be taken into account.

spytheman commented 7 months ago

v -Wimpure-v test x_test.v ... will run the test x_test.v, ignore the warnings, then report whether the test passed (which it does).

v -Wimpure-v -W test x_test.v ... will turn the warnings into errors, then report the compilation failure.

v -Wimpure-v x_test.v will show you the warnings.

spytheman commented 7 months ago

For example: ./v -Wimpure-v -W test vlib/compress/zstd/ reveals that vlib/compress/zstd/zstd.v should be renamed to vlib/compress/zstd/zstd.c.v .

JalonSolov commented 7 months ago

I think -Wimpure-v should work without -W test... which I never knew existed.

In any case, if -Wimpure-v is turned on by default, it will definitely affect all modes.

spytheman commented 7 months ago

It does work without -W. It just produces warnings, and warnings do not cause compilation to fail.

-W just turns warnings into errors, and errors make the compilation fail.

-prod would have worked too.

v test . just passes all the options before test as compile flags to the underlying v file_test.v etc commands.

spytheman commented 7 months ago

Try for example: v -Wimpure-v x_test.v , i.e. running the test directly by v, not by v test, and you will see the warnings.

JalonSolov commented 7 months ago

Yes, I understand, but I'm saying it should "just work" without having to know a special sequence of things to type. v -Wimpure-v <followed by anything else> should always activate that message.

spytheman commented 7 months ago

But it does already. v test just does not display notices, warnings, and the output of running individual _test.v files to you, unless it failed to compile it, or when it ran, but exited with status code != 0.

Currently -Wimpure-v, generates warnings. Warnings do not stop compilation -> v test will compile, and then it will run the test, which will succeed, and v test will just discard the output (including the warnings).

spytheman commented 7 months ago

If what you are arguing for, is towards making v test display warnings and notices by default, then that is reasonable, even though I think that it can be confusing, since in most cases, they do not matter much for tests.

JalonSolov commented 7 months ago

I think this is a special case, and should be displayed in all cases. Otherwise, it can cause confusion as to why tests won't work with other backends, unless you open the source and scan for C..

mengzhuo commented 3 days ago

FYI coreutils CI failed due to this issue. For example: https://github.com/vlang/coreutils/actions/runs/10865356775/job/30151722872