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

compiler: add error for unnecessary use of 'var' #18017

Merged
merged 15 commits into from
Nov 19, 2023

Conversation

mlugg
Copy link
Member

@mlugg mlugg commented Nov 16, 2023

Depends on #18006, which corrects some unnecessary uses of var in Aro.

This PR implements #224. The compile error is emitted when a var is never used as an lvalue - more detailed analysis is unfortunately not possible under Zig's current analysis model, as whether a var is actually mutated may depend on semantic analysis where conditional compilation may incorrect affect the answer.

Despite how limited this analysis is, the change identified around 1000 incorrect uses across the standard library and compiler, which have been manually corrected. It also led to me catching a few bugs in std, and one in the compiler - these have been fixed, aside from one in std which I wasn't sure of the correct solution for, so a temporary @compileError has been put in place to point out the issue. (This shows the usefulness of the error!)

Since the analysis is simply based on whether a variable is ever used as an lvalue, there is a quite convenient way to silence this error when needed: _ = &x. This is kind of a dirty secret, and should ideally never be used in user code, but it's useful in the test cases and behavior tests, where we sometimes want to force values to be semantically runtime-known.

The actual implementation of this change is fairly trivial - the bulk of this PR is spent fixing the entire codebase to not trigger this error.

Copy link
Member

@andrewrk andrewrk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work! I'm very happy about this change. I've been letting contributions slide that make this mistake knowing that the compiler would solve the problem eventually. Thanks for going through and making the fixes.

Copy link
Member

@andrewrk andrewrk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops forgot to mark as approved

@mlugg mlugg force-pushed the var-never-mutated branch 2 times, most recently from e3db2d7 to 3b27815 Compare November 17, 2023 04:42
@mlugg mlugg added the breaking Implementing this issue could cause existing code to no longer compile or have different behavior. label Nov 17, 2023
@mlugg mlugg force-pushed the var-never-mutated branch 4 times, most recently from 10fc9a9 to 8756fbf Compare November 17, 2023 05:04
Copy link
Contributor

@IntegratedQuantum IntegratedQuantum left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

More tautological cases you missed in math/big_int_test.zig

lib/std/math/big/int_test.zig Outdated Show resolved Hide resolved
lib/std/math/big/int_test.zig Outdated Show resolved Hide resolved
lib/std/math/big/int_test.zig Outdated Show resolved Hide resolved
lib/std/math/big/int_test.zig Outdated Show resolved Hide resolved
lib/std/math/big/int_test.zig Outdated Show resolved Hide resolved
lib/std/math/big/int_test.zig Outdated Show resolved Hide resolved
lib/std/math/big/int_test.zig Outdated Show resolved Hide resolved
lib/std/math/big/int_test.zig Outdated Show resolved Hide resolved
lib/std/math/big/int_test.zig Outdated Show resolved Hide resolved
lib/std/math/big/int_test.zig Outdated Show resolved Hide resolved
@mlugg mlugg force-pushed the var-never-mutated branch 4 times, most recently from 0393df6 to ea8dc83 Compare November 18, 2023 18:26
When a local variable is never used as an lvalue, we can determine that
`const` would be sufficient for this variable, so emit an error in this
case. More sophisticated checking is unfortunately not possible with
Zig's current analysis model, since whether an lvalue is actually
mutated depends on semantic analysis, in which some code paths may not
be analyzed, so attempting to determine this would result in false
positive compile errors.

It's worth noting that an unfortunate consequence of this is that any
field call `a.b()` will allow `a` to be `var`, even if `b` does not take
a pointer as its first parameter - this is again a necessary compromise
because the parameter type is not known until semantic analysis.

Also update `translate-c` to not trigger these errors. This is done by
replacing the `_ = @typeof(x)` emitted with `_ = &x` - the reference
there means that the local is permitted to be `var`. A similar strategy
will be used to prevent compile errors in the behavior tests, where we
sometimes want to force a value to be runtime-known.

Resolves: ziglang#224
@mlugg mlugg merged commit 6b1a823 into ziglang:master Nov 19, 2023
10 checks passed
scheibo added a commit to pkmn/engine that referenced this pull request Nov 20, 2023
guess you really do need a machine to enforce these things
@InKryption
Copy link
Contributor

A small issue I've just encountered with this change is that unlike with _ = x;, _ = &x; doesn't become an error after it becomes unnecessary. If _ = &x; is to become the official way to allow unused mutability, then it should probably be recognized as unnecessary as well.

@Atomk
Copy link
Contributor

Atomk commented Nov 21, 2023

In my opinion _ = &x; looks a bit too much like a hack to be the "official" way to do it, maybe something like a @forceRuntimeKnown that can only be used in tests would help to clarify the intent.

@andrewrk
Copy link
Member

The official way is to fix your code. Don't work around the error.

@mlugg
Copy link
Member Author

mlugg commented Nov 21, 2023

The workaround has the following uses:

  • working around compiler bugs where making something const triggers the bug
  • writing tests for the compiler itself (behavior tests or test cases)
  • writing std tests where you absolutely need to force a value to be runtime-known; this came up in bigint tests and a small handful of other places

This means that outside of the compiler codebase, there is pretty much no use in it.

Also note: currently, translate-c emits _ = &x discards all over the place to avoid triggering the error, but I hope that eventually we can instead internally give the translate-c output to zig fmt --autofix, so things just become const (which is the simple, correct, readable solution).

linusg added a commit to linusg/parser-toolkit that referenced this pull request Nov 22, 2023
This no longer builds with the latest version of Zig.

See: ziglang/zig#18017
linusg added a commit to linusg/zig-libgc that referenced this pull request Nov 22, 2023
This no longer builds with the latest version of Zig.

See: ziglang/zig#18017
@linusg
Copy link
Sponsor Contributor

linusg commented Nov 22, 2023

This has a limitation that I'd assume to be quite common as it applies to built-ins like @memset() and many C APIs:

fn foo(ptr: *usize) void {
    ptr.* = 42;
}

fn main() void {
    const s: struct { i: usize = 0 } = .{};
    var i = &s.i; // error: local variable is never mutated
    foo(i);
}

@andrewrk
Copy link
Member

The compiler is correct; the code is wrong. It should be const not var.

@linusg
Copy link
Sponsor Contributor

linusg commented Nov 22, 2023

That doesn't build either (0.12.0-dev.1676+40b8c993f):

file.zig:8:9: error: expected type '*usize', found '*const usize'
    foo(i);

@Snektron
Copy link
Collaborator

You mean to write

fn foo(ptr: *usize) void {
    ptr.* = 42;
}

fn main() void {
    var s: struct { i: usize = 0 } = .{};
    const i = &s.i;
    foo(i);
}

ikskuh pushed a commit to ikskuh/parser-toolkit that referenced this pull request Nov 23, 2023
This no longer builds with the latest version of Zig.

See: ziglang/zig#18017
joachimschmidt557 pushed a commit to joachimschmidt557/zig-wcwidth that referenced this pull request Nov 24, 2023
This no longer builds with the latest version of Zig.

See: ziglang/zig#18017
joachimschmidt557 pushed a commit to joachimschmidt557/linenoize that referenced this pull request Nov 26, 2023
* Update var to const in build.zig where applicable

This no longer builds with the latest version of Zig.

See: ziglang/zig#18017

* Update zig-wcwidth

This includes fixes to make build.zig build again.

* Add now-required paths field to build.zig.zon
@mlugg mlugg mentioned this pull request Dec 22, 2023
3 tasks
joelvaneenwyk pushed a commit to joelvaneenwyk/ncdu that referenced this pull request Feb 1, 2024
Fixes these errors (introduced in ziglang/zig#18017
and ziglang/zig@6b1a823 ):

```
src/main.zig:290:13: error: local variable is never mutated
        var line_ = line_fbs.getWritten();
            ^~~~~
src/main.zig:290:13: note: consider using 'const'
src/main.zig:450:17: error: local variable is never mutated
            var path = std.fs.path.joinZ(allocator, &.{p, "ncdu", "config"}) catch unreachable;
                ^~~~
src/main.zig:450:17: note: consider using 'const'

...
```

Will be included in future Zig 0.12, this fix is backward compatible:
ncdu still builds and runs fine on Zig 0.11.0.

Signed-off-by: Eric Joldasov <bratishkaerik@getgoogleoff.me>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking Implementing this issue could cause existing code to no longer compile or have different behavior.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants