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

stage2 astgen: catch unused vars #9047

Merged
merged 29 commits into from
Jun 22, 2021
Merged

stage2 astgen: catch unused vars #9047

merged 29 commits into from
Jun 22, 2021

Conversation

g-w1
Copy link
Contributor

@g-w1 g-w1 commented Jun 9, 2021

I am almost positive the actual impl works (besides a preexisting TODO in asm which I will fix, and maybe some other bugs like the struct type not getting referenced), the main work is just fixing all the code which will take a bunch of work.

If you see your code is in some of the unused vars (either in the code or ci failure), it would help me a lot if you could tell me the correct way to do it as I may not know.

Issues to file:

                            return astgen.failNodeNotes(ident, "'{s}' not accessible from inner function", .{ident_name}, &.{
                                try astgen.errNoteTok(local_ptr.token_src, "declared here", .{}),
                                // TODO add crossed function definition here note.
                                // Maybe add a note to the error about it being because of the var,
                                // maybe recommend copying it into a const variable. -SpexGuy

@Vexu
Copy link
Member

Vexu commented Jun 9, 2021

Could you add the is_inline change from #8921 (comment) here?

@g-w1
Copy link
Contributor Author

g-w1 commented Jun 9, 2021

Oops, did not see that, I will.

@LemonBoy
Copy link
Contributor

LemonBoy commented Jun 9, 2021

The two occurrences in lib/std/compress/* can be safely removed.
cc @jedisct1 for the crypto stuff.

Shall we add some kind of escape hatch for this? Many languages exclude from this rule variables whose name starts with an underscore.

@g-w1
Copy link
Contributor Author

g-w1 commented Jun 9, 2021

#335 (comment) should work to disable them.

@LemonBoy
Copy link
Contributor

LemonBoy commented Jun 9, 2021

#335 (comment) should work to disable them.

Fair enough, it's a bit verbose but works without adding extra logic that's dependent on the variable's name. I like it.

@g-w1 g-w1 force-pushed the spider-astgen branch from ed5dc18 to 69b74fc Compare June 9, 2021 21:10
@g-w1
Copy link
Contributor Author

g-w1 commented Jun 10, 2021

Last few vars:

math/complex/ldexp.zig:56:11: error: unused local constant
    const kln2 = 1246.97177782734161156; // k * ln2
          ^
hash/wyhash.zig:169:15: error: unused local constant
        const seed = self.state.seed;
              ^
os/linux/io_uring.zig:1280:11: error: unused local constant
    const connect = try ring.connect(0xcccccccc, client, &address.any, address.getOsSockLen());
          ^
crypto/bcrypt.zig:51:13: error: unused local variable
        var t: u32 = undefined;
            ^
crypto/bcrypt.zig:78:13: error: unused local variable
        var t: u32 = undefined;
            ^
crypto/aes/soft.zig:52:15: error: unused local constant
        const src = &block.repr;
              ^
crypto/aes/soft.zig:69:15: error: unused local constant
        const src = &block.repr;
              ^
crypto/aes/soft.zig:91:15: error: unused local constant
        const src = &block.repr;
              ^
crypto/aes/soft.zig:108:15: error: unused local constant
        const src = &block.repr;
              ^
crypto/25519/scalar.zig:339:15: error: unused local constant
        const t23 = @truncate(u64, z32 + c21) & 0xffffffffffffff;
              ^
crypto/25519/scalar.zig:614:15: error: unused local constant
        const t102 = @as(u64, @truncate(u64, z31 + c20)) & 0xffffffffffffff;
              ^
crypto/chacha20.zig:447:25: error: unused local variable
                    var i: u32 = 0;
                        ^

Copy link
Contributor

@LemonBoy LemonBoy left a comment

Choose a reason for hiding this comment

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

This change surfaced a lot of big and small problems in the stdlib, awesome.
Fixing all the problems is probably better if done in a separate PR.

lib/std/zig/system.zig Outdated Show resolved Hide resolved
lib/std/testing.zig Outdated Show resolved Hide resolved
lib/std/math/complex/ldexp.zig Outdated Show resolved Hide resolved
lib/std/math/tanh.zig Outdated Show resolved Hide resolved
lib/std/hash/wyhash.zig Show resolved Hide resolved
lib/std/crypto/aes_ocb.zig Show resolved Hide resolved
lib/std/crypto/aes_gcm.zig Show resolved Hide resolved
lib/std/crypto/aes.zig Outdated Show resolved Hide resolved
lib/std/base64.zig Show resolved Hide resolved
var system_info: os.system_info = undefined;
const rc = os.system.get_system_info(&system_info);
// var system_info: os.system_info = undefined;
// const rc = os.system.get_system_info(&system_info);
Copy link
Contributor

Choose a reason for hiding this comment

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

@hoanga Please introduce the B_* constants for error codes. Returning 1 here is a safe default but I think errors should be bubbled up to the caller.
Speaking of get_system_info, this line looks wrong, they probably meant return error.

const count: u32 = if (os.system.get_system_info(&system_info) != 0)
    system_info.cpu_count
else
    1;

@jedisct1
Copy link
Contributor

Last few vars:

crypto/bcrypt.zig:51:13: error: unused local variable
        var t: u32 = undefined;
            ^
crypto/bcrypt.zig:78:13: error: unused local variable
        var t: u32 = undefined;
            ^
crypto/aes/soft.zig:52:15: error: unused local constant
        const src = &block.repr;
              ^
crypto/aes/soft.zig:69:15: error: unused local constant
        const src = &block.repr;
              ^
crypto/aes/soft.zig:91:15: error: unused local constant
        const src = &block.repr;
              ^
crypto/aes/soft.zig:108:15: error: unused local constant
        const src = &block.repr;
              ^
crypto/25519/scalar.zig:339:15: error: unused local constant
        const t23 = @truncate(u64, z32 + c21) & 0xffffffffffffff;
              ^
crypto/25519/scalar.zig:614:15: error: unused local constant
        const t102 = @as(u64, @truncate(u64, z31 + c20)) & 0xffffffffffffff;
              ^
crypto/chacha20.zig:447:25: error: unused local variable
                    var i: u32 = 0;
                        ^

Fine to remove them all.

@g-w1 g-w1 force-pushed the spider-astgen branch 2 times, most recently from fb07eca to fa0a948 Compare June 10, 2021 19:47
LemonBoy added a commit to LemonBoy/zig that referenced this pull request Jun 11, 2021
It turns out the code was not ported correctly from C and produced wrong
results for negative input values. As a bonus fix the NaN codepath by
adding yet another missing piece of code.

Spotted in ziglang#9047
Vexu pushed a commit that referenced this pull request Jun 11, 2021
It turns out the code was not ported correctly from C and produced wrong
results for negative input values. As a bonus fix the NaN codepath by
adding yet another missing piece of code.

Spotted in #9047
andrewrk pushed a commit that referenced this pull request Jun 11, 2021
It turns out the code was not ported correctly from C and produced wrong
results for negative input values. As a bonus fix the NaN codepath by
adding yet another missing piece of code.

Spotted in #9047
@g-w1 g-w1 force-pushed the spider-astgen branch 2 times, most recently from 2003f4a to e7cd0c8 Compare June 11, 2021 20:20
LemonBoy added a commit to LemonBoy/zig that referenced this pull request Jun 12, 2021
Two bugs in the implementation ported from musl made all the complex
functions relying on ldexp return incorrect results in some cases.

Spotted in ziglang#9047
Vexu pushed a commit that referenced this pull request Jun 12, 2021
Two bugs in the implementation ported from musl made all the complex
functions relying on ldexp return incorrect results in some cases.

Spotted in #9047
@g-w1 g-w1 marked this pull request as ready for review June 12, 2021 17:25
@g-w1 g-w1 force-pushed the spider-astgen branch 2 times, most recently from 3dd8e23 to ef58248 Compare June 12, 2021 18:54
@jedisct1
Copy link
Contributor

This is turning into an epic PR! 👍

@g-w1 g-w1 force-pushed the spider-astgen branch 4 times, most recently from df66dfe to 697c1c7 Compare June 14, 2021 03:58
g-w1 and others added 25 commits June 21, 2021 17:03
This is temporary and putting this as a seperate commit
so that it can easily be reverted as andrewrk suggested.
Also rename LocalPtr.is_comptime to LocalPtr.maybe_comptime as
it is a better name, as it could be runtime, but is not
always runtime.
Gotta catch 'em all!

also simplify identifier( logic
The printing code did not properly set the parent Decl node field, so
the source location calculations were wrong.

closes ziglang#8825
Before this, the continue expression of a while loop did not have the
capture variable in it, making it incorrectly emit a compile error for
not using the capture, even if it was referenced.
There was a typo here and the neg function referenced a non-existent
variable.
Previously the fd parameter was ignored and so the result would not get
populated. Now it passes the fd pointer to the inline assembly so that
the results can be observed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
frontend Tokenization, parsing, AstGen, Sema, and Liveness.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants