Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
JalonSolov opened this issue Jan 30, 2024 · 10 comments
Open

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

JalonSolov opened this issue Jan 30, 2024 · 10 comments
Labels
Bug This tag is applied to issues which reports bugs.

Comments

@JalonSolov
Copy link
Contributor

JalonSolov commented Jan 30, 2024

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.

@JalonSolov JalonSolov added the Bug This tag is applied to issues which reports bugs. label Jan 30, 2024
@spytheman
Copy link
Member

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
Copy link
Member

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
Copy link
Contributor Author

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
Copy link
Member

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
Copy link
Member

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
Copy link
Contributor Author

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
Copy link
Member

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
Copy link
Member

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
Copy link
Contributor Author

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
Copy link
Contributor

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug This tag is applied to issues which reports bugs.
Projects
None yet
Development

No branches or pull requests

3 participants