zig icon indicating copy to clipboard operation
zig copied to clipboard

compile errors for unused things

Open andrewrk opened this issue 7 years ago • 153 comments

  • [ ] compile error for unused non-pub global variable
  • [x] compile error for unused local variable
  • [ ] compile error for unused non-pub function decl
  • [ ] compile error for unreachable code (astgen only)
  • [ ] ~even unused pub functions are an error if they are not accessible from outside the package~

Note that because of conditional compilation, some things may seem unused when they are in fact used but only depending on some compile variables. Some thought will have to be put into how to make this work without sacrificing compile speed or lazy evaluation.

andrewrk avatar Apr 19 '17 06:04 andrewrk

Would you be able to easily disable these errors? They could get annoying when you're debugging something by e.g. commenting out sections of code. (How does Golang deal with this?)

ghost avatar Nov 01 '18 23:11 ghost

You could disable an error by doing _ = the_unused_variable.

andrewrk avatar Nov 02 '18 04:11 andrewrk

On a related note, the "unreachable code" error is already annoying sometimes, albeit far less annoying than "unused import" errors would be. Adding a "return" statement halfway up a function for testing purposes is something I (try to) do often. I really wish there was an option to turn these into warnings (as an opt-in compiler flag).

Some Go developers say[1] they "get used to" the errors after a while, but there must be a better way (not involving an IDE plugin)? Adding underscores really slows me down when I'm trying to troubleshoot something, constantly commenting/uncommenting and recompiling, and I have to scroll to the top of the file and add/remove underscores from imports every time as well. I'm not always the most patient when trying to learn a new language at home and troubleshooting problems. And then it certainly doesn't help my mood when I search for the problem online and get official FAQs[2] lecturing me not to leave code quality for later and so on.

[1] https://groups.google.com/forum/#!topic/golang-nuts/OBsCksYHPG4 [2] https://golang.org/doc/faq#unused_variables_and_imports

ghost avatar Jul 18 '19 05:07 ghost

Note that because of conditional compilation, some things may seem unused when they are in fact used but only depending on some compile variables. Some thought will have to be put into how to make this work without sacrificing compile speed or lazy evaluation.

But here comes the tricky part (as if it's not tricky enough already)! Hot code swapping #68. If implemented, hot code swapping will the favorite feature among those that want to make quick experimental changes and see the results immediately.

For example:

fn gameLoop() void {
    normalStuff();
    crazyStuff();
}

The function crazyStuff has been created to contain all of the code additions that you want to experimentally turn on and off while the game is running. If every time you commented out crazyStuff you had to comment out not only crazyStuff itself but all of the functions that only it calls then the speed advantaged and comfort of hot reload is substantially reduced.

With hot code swapping do we even know which functions might get called?

Edit: Would a comptime { _ = crazyStuff; } block solve this? Is it that simple?

ghost avatar Aug 07 '19 22:08 ghost

Note that because of conditional compilation, some things may seem unused when they are in fact used but only depending on some compile variables. Some thought will have to be put into how to make this work without sacrificing compile speed or lazy evaluation.

solved by #8516

andrewrk avatar Apr 21 '21 23:04 andrewrk

Zig is designed primarily for the use case of modifying existing Zig code and secondarily for the use case of writing new Zig code from scratch. This is why it accepts the tradeoff of the annoyance of compile errors for unused things, in exchange for helping to rework existing code without introducing bugs.

andrewrk avatar Jun 23 '21 17:06 andrewrk

Currently i can do the following to stop the compiler from complaining:

fn messageCallback(
    message_severity: vk.DebugUtilsMessageSeverityFlagsEXT.IntType,
    message_types: vk.DebugUtilsMessageTypeFlagsEXT.IntType,
    p_callback_data: *const vk.DebugUtilsMessengerCallbackDataEXT,
    p_user_data: *c_void,
) callconv(vk.vulkan_call_conv) vk.Bool32 {
    // keep parameters for API
    _ = message_severity;
    _ = message_types;
    _ = p_user_data;
   // more code ....
}

Is there a way of telling the compiler that the parameters are there because of API compatibility, or is the example above idiomatic?

Avokadoen avatar Jul 02 '21 16:07 Avokadoen

The above example is idiomatic. I like to consider it to be semantic documentation that the parameters are unused.

andrewrk avatar Jul 02 '21 18:07 andrewrk

I like to consider it to be semantic documentation

Should discarded parameters/variables which are later used within the function be an error too? I've noticed that it's easy to forget to remove the discard when updating the function leading to communicating that the parameter doesn't matter even if it now does.

tauoverpi avatar Jul 02 '21 20:07 tauoverpi

I think that would make sense, yes. The error message could be "ineffectual discard of local variable".

andrewrk avatar Jul 02 '21 21:07 andrewrk

this hurt iteration when experimenting A LOT, i think this should be disabled for debug builds and only enforced for release builds

i speak from experience, it's very annoying

there is a limit to how much you have to help the developer, freedom is what allows creating new things, too much friction and rules can harm creativity

please remove my comment if that is not the right place to give feedback, i apologies in advance

i agree it is design issue if i leave things unused, BUT you don't automatically erase everything from your notebook when you sketch new ideas, the flow should be smooth

ryuukk avatar Jul 09 '21 01:07 ryuukk

Thanks for the feedback @ryuukk. Feedback such as this is useful especially when I see that you have been using the language in earnest (with zark).

andrewrk avatar Jul 09 '21 01:07 andrewrk

Instead of only having this for release builds as ryuukk suggested, I propose a flag that demotes the error to an annoying warning.

Ristovski avatar Jul 22 '21 09:07 Ristovski

This can indeed be annoying, especially when using ZLS.

VSCode is now constantly displaying errors.

But having variables not being used while the code eventually using them is being written is expected :) This encourages closing the errors tab and not paying attention to errors at all.

jedisct1 avatar Jul 22 '21 10:07 jedisct1

I also like to add that this is very annoying and hurts productivity.

This is as if a safety commisioner would complain to the architect that a building doesn't meet the necessary specifications while it is being build.

It's useful to handle this as an error in release builds, but having this in debug builds doesn't make sense to me.

name-changer avatar Jul 23 '21 06:07 name-changer

I do not really have any problems with this feature so I hope it remains in some shape or form

Avokadoen avatar Jul 23 '21 07:07 Avokadoen

How about reusing _ for ignored parameters? @Avokadoen's code would look like:

fn messageCallback(
    _: vk.DebugUtilsMessageSeverityFlagsEXT.IntType, // message_severity
    _: vk.DebugUtilsMessageTypeFlagsEXT.IntType, // message_types
    p_callback_data: *const vk.DebugUtilsMessengerCallbackDataEXT,
    _: *c_void, // p_user_data
) callconv(vk.vulkan_call_conv) vk.Bool32 {
   // code ....
}

ekipan avatar Jul 28 '21 19:07 ekipan

~i think being allowed to prepend an underscore to the parameter name should be enough to silence the error. it preserves more information succinctly than an underscore and a separate comment.~ see https://github.com/ziglang/zig/issues/335#issuecomment-889328280 and https://github.com/ziglang/zig/issues/9239#issuecomment-868783930

fn messageCallback(
    _message_severity: vk.DebugUtilsMessageSeverityFlagsEXT.IntType,
    _message_types: vk.DebugUtilsMessageTypeFlagsEXT.IntType,
    p_callback_data: *const vk.DebugUtilsMessengerCallbackDataEXT,
    _p_user_data: *c_void,
) callconv(vk.vulkan_call_conv) vk.Bool32 {
   // code ....
}

emekoi avatar Jul 29 '21 17:07 emekoi

@emekoi I found out after posting that your suggestion was already proposed and rejected: https://github.com/ziglang/zig/issues/9239#issuecomment-868783930.

In that proposal fengb mentioned using just _ but dismissed it because it hides intent. Underscore _ by itself is already special and is planned to become a keyword: https://github.com/ziglang/zig/issues/1802. Personally, I'm for it. The idea is that it just wouldn't bind a name at all so you can use it several times and there's no chance of accidentally using the parameter.

Wouldn't this also resolve https://github.com/ziglang/zig/issues/9238? Are there any other use cases for _ = name; besides unused parameters (and discarding function results)? I feel like any other unused local variables should just be deleted.

ekipan avatar Jul 29 '21 17:07 ekipan

As a longtime dev, I also find this to be very counter-productive to my iteration speed while developing. I'm fairly new to Zig and like almost everything, but this is painful. I often want to write some code then test to see if everything compiles, I find this error hounding my last line of code a decent portion of the time, breaking my flow and creating needless work to get around it. I personally think this goes too far but I respect the overall vision.

errantmind avatar Sep 22 '21 06:09 errantmind

Instead of only having this for release builds as ryuukk suggested, I propose a flag that demotes the error to an annoying warning.

This is the infamous "sloppy mode" proposal (https://github.com/ziglang/zig/issues/3320), which was rejected.

ghost avatar Sep 28 '21 20:09 ghost

I think this would be a lot less harsh if https://github.com/ziglang/zig/issues/9239 were implemented, allowing you to still communicate unuse while also not forgetting what a parameter is if you have a future need for it, which is especially common in refactoring.

Plus, prefixing & removing single leading underscores is easier than replacing whole identifiers with single underscores & later replacing single identifiers with a variety of identifiers; automating converting a normal identifier to discard & back currently pretty much forces either comments with old names or use of version control.

bb010g avatar Dec 09 '21 03:12 bb010g

I'm not sure if this is helpful feedback, but the new mandatory compile error for unused local variable is preventing me from using Zig. Others have explained why this is an ergonomic disaster for iterative development, and as someone who is required to use this setting on a ~1M line C++ program, I can confirm, it's a fantastic way to waste time while making small changes and debugging, and gets really fun with conditional compilation between platforms. I do not commit code that has unused locals, etc, in it, but there is absolutely no reason that I can't or shouldn't run a debug build that does.

I second the calls for making these errors something that can be demoted to warnings, at least in debug builds--while not ideal, that sort of compromise will hopefully prevent code with illegal unused stuff from being committed into various libraries, etc.

Zig appears to be an excellent systems programming language apart from this, but it sounds like the ergonomics for iterative development are going to continue to get worse as more errors that may-or-may-not-be helpful are forced on all users.

(Please feel free to delete this comment if it's not helpful, I've been following Zig for years and hoped for a chance to use it, so this is a bummer for me)

JustinWick avatar Dec 26 '21 07:12 JustinWick

I'll just reiterate: Zig has no warnings

One of the reasons for this is that in languages that does have them developers tends to ignore them. It seems to me that people here intend on ignoring warnings and therefore view this as a solved issue by converting errors to warnings... Anyways, this means that there are ~~three~~ two options:

  1. Remove "unused local" errors (do not validate code in this way at all)
  2. ~~Add a lazy mode where validation errors are turned off~~
  3. Keep them as is

There is no in-between.

I think it's a bit odd that people seem to find it hard to simply comment out lines (most modern IDEs can help you here) as they prototype or even prototype by doing (this will be illegal later as per Andrew's comment above)

var my_demo_var: u8 = 0;
_ = my_demo_var; // keep this line as long as I prototype

I personally would much rather spend a few seconds extra here and there at the rare occurrence that i just write unused variables in order to test something, than having to debug potential bugs caused by obvious mistakes.

Avokadoen avatar Dec 26 '21 10:12 Avokadoen

I think it's a bit odd that people seem to find it hard to simply comment out lines (most modern IDEs can help you here)

if you need an IDE to program, then something failed somewhere Also recommending to workaround it means there is a design issue somewhere

also: _ = my_demo_var; shouldn't compile since it's unused, forgetting about it will introduce bugs

there, the sloppy mode you didn't want, but you implemented anyways

ryuukk avatar Dec 26 '21 16:12 ryuukk

if you need an IDE to program, then something failed somewhere

that is simply untrue. additionally, "IDE" here means any code editor more complex than notepad.exe


I would also like to reiterate, since this comment comes up pretty often,

https://github.com/ziglang/zig/issues/335#issuecomment-867029639

this error will get easier to work with in the future but it is very likely not going away any time soon

nektro avatar Dec 26 '21 18:12 nektro

Also recommending to workaround it means there is a design issue somewhere

Is fixing a compile error (commenting or removing invalid code) a "workaround"?

also: _ = my_demo_var; shouldn't compile since it's unused, forgetting about it will introduce bugs

Like my previous comment mention: this works temporarily and will be fixed later. If you are for some reason doing a lot of prototyping, then feel free to abuse that it works for now. I would suggest getting used to your text editor and comment out invalid code instead.

there, the sloppy mode you didn't want, but you implemented anyways

Yes, its a rejected proposal. I'll remove it from the list

The intent of my previous comment was to steer the discussion away from suggestions of "warnings" since they are currently not a concept in Zig.

Avokadoen avatar Dec 26 '21 20:12 Avokadoen

RE: #335 (comment) , my concern with this feature is precisely because Zig wants to support modifying Zig code in the future.

Here is a real-life example (details modified) that I have lived, and it would have been a nightmare when working under the general principle here that the compiler should error on things that are both potentially unsafe/unintended, and can be fixed:

'Tis the week before $HOLIDAY and all through the place, not an error is stirring, not even a race. You finally can use that "unlimited vacation"--a rarity for a dev of your hallowed station. 'tis a bright future, and typesafe indeed: your million lines are scriven in Zig, not in C.

Suddenly arises a foreboding clatter--your manager bursts in! You say "what's the matter?"

"CTO of $SOMECO has spoke a decree: we must support WhiZbang, and thus Log4Z."

"Why worry?" you ask, for Zig makes all ease. 'Ts been long since you've seen exception or freeze.

"One million lines come not easy, my friend, and Zig has been working to that very end. WhiZbang does not work with Zig Version 2, migrate to v3 is what we must do. And that path's no sweat for most Zig-worthy folk, but I tried it myself--the build's hopelessly broke. Zig thought it knew better than we what we need--half our source libraries are goners indeed."

...the rest of the poem is left as an exercise to the reader (given the text of my attempts, I assure you that this is for the best, and that they truly deserved to be a compiler error), but this story really did happen to me when upgrading from GCC 4.4 to GCC 7. That said, I may have exaggerated its catastrophic impact on $HOLIDAY (but certainly not my sanity).

Alien code not touched in 10 years but absolutely critical to our project was completely broken by the compiler's new warnings (we rightly treat those as errors in C++ and suppress those that are spurious). No one at the company fully understood how the code worked, any more than any dev understands the entirety of every single library that they use, and changing even a single line of that code could have unforeseen consequences. Every commit to our codebase must pass over a thousand integration tests that simulate real-life use cases with comparisons to hundreds of gigabytes of known-good logfiles saving all necessary state. This code is used by hundreds of millions of users and has a well-below-acceptable rate of bugs despite being developed in C++98 over the course of nearly 30 years. Unfortunately system code that has to deal with ~100 different embedded platforms is going to be messy, and even well-known FOSS code written with older paradigms in mind might trigger fancy new warnings/errors in modern compilers.

It was up to me to decide which of the literal tens of thousands of warnings in our own code were worth pursuing--they could, in some bizarre circumstances have actually constituted a problem, and a few of them were legitimately useful. I love that we have the capability in GCC/Clang to get all kinds of fantastic static analysis and to promote them to errors when they will add more than they cost to our project--the vast majority of them we do--and I believe that anyone who simply ignores compiler warnings when they have the option of fixing or marking them irrelevant is a fool. Compiler fanciness can make errors harder to accidentally commit, but they can't fix bad development processes nor, IMHO, should they try. I rely heavily on static typechecking, etc to check mistakes (most of my code is in functional languages like F# and Scala), but anyone determined to just spew out bad code without care for what they are doing, is going to find a way to do so, and it's hardly the compiler's fault of some moron decides to ignore useful warnings.

Furthermore by not allowing a demotion to warnings, I can't run a full build of a massive codebase and see what the errors would be when I turn them back on--instead I must fix them one by one, and when builds can take a half hour (if you think that's not possible in $COMPILED_LANGUAGE then you are very lucky in what you've had to work on) and you need to be able to scope out the work now this is absolutely unacceptable.

What's the largest Zig codebase that's used in the real world (say, by more than 10,000 people?) Is it up to a million lines yet? Half? There are so many pain points that don't matter when one is writing compact, elegant code on a small team with an expected lifetime of a few years. The codebase I referred to above was startlingly clean for how sprawling and complex it had to be, but it stretched C++ to near breaking because C++ keeps trying to change what it is and how it should be used.

If I invested in Zig 1.0+, and I upgrade, will the compiler suddenly start deciding that random things like unused return values are errors? This kind of thing is totally fine in functional code where the value of an expression is generally the point of it, but maddening in imperative code unless everyone in the entire ecosystem is onboard with not spamming you with random return values you probably don't need if you're not using them. I don't have time to fix a million lines of complicated code to match changing ideas of language developers, even if they have merit. Upgrading to the latest version of WhiZbang or whatever that placates the compiler properly (or allows one to placate the compiler easily) is not always an easy option on projects where things such as change control boards exist, and security patches are an especially important problem in an "unsafe" language like Zig, and it won't necessarily be possible to backport these fixes to a library that works with an older Zig compiler that trusts the developer more.

It's fine if the answer is "don't use Zig if you don't want to spend hundreds of human-hours to refactor a large codebase", but I think that needs to be communicated upfront. Perhaps include somewhere that "at any moment the Zig compiler could cause you to have to un-fsck an arbitrary amount of legacy code that you might not have written". No language needs to be all things to all people, and like I said, there are other suitable alternatives, like Rust out there, so it's not like you're kicking anyone out into the cold, but I feel like the use case for this feature must be radically different than the use case for many large, monolithic codebases out there. And yes, microservices and other liberally-partitioned forms of software development are a solid fit for some projects, but certainly not all of them.

In summary:

  1. This adds a significant mental and temporal burden when doing incremental development that requires temporary commenting-out of code for debugging or experimentation. Systems programming is notorious for the kinds of errors one can make when one's model of the system isn't perfect, or perfectly reasoned-about, and dev-test-debug ergonomics matter.
  2. Because this kind of thing can't be made a warning/info/or simply disabled, if I make a refactoring change to my code, who knows how many builds it'll take to find and fix all of this? For an unused local that's probably fine (unless it's autogenerated source) but for something like "compile error for unreachable code" or the dreaded "compile error for unused function return value"... in a large codebase that could be many files.
  3. These kinds of changes break 3rd party/legacy code without recourse for the developer short of forking it or updating, which potentially leads to all sorts of new errors introduced.
  4. It's totally reasonable to bind things like return values, etc to names that are only used for debugging purposes (commented-out printing, for instance). They may not be on the same screen, and Zig doesn't want us using macros all over the place.
  5. The name of the unused local is documentation. Turning it into "" documents that you aren't using it, but even console-based text editors should be able to detect that using a language server, it doesn't need to break the build. Furthermore one might want to test a partially-implemented function without adding all sorts of spurious "" bindings that they then have to remove.
  6. Developers who cannot/will not do this shouldn't be doing systems programming, and there's never a need to cater to them. I can and do read warnings about unused locals in every programming language I use, regardless of the purpose of the code. If a project lead wants to force this on morons, let them set the equivalent of "-Werror" and accept the consequences for their project. Better yet, give them the option of doing so on release builds and letting things slide a little on debug.
  7. I legitimately want and would use all of these features and am fine with having them as the default, but having them mandatory is a complete dealbreaker for me, full stop. Judging by comments here and elsewhere, I'm hardly the only one, and as well as some dogmatic approaches to software development can work fantastically for certain types of software teams in specific domains, this sort of shoved-down-your-throat startling breakage sets a precedent that might as well be a huge red flag surrounded by fireworks. There is a reason that other popular languages implement this static analysis without making it a catastrophic compile-time error with no configurable workarounds.
  8. This can be handled by a linter. Linters are great for this kind of pedantic analysis, and anyone who actually cares to make their code super tidy will use one. Anyone who doesn't is taking a known risk, that should be fine. And if there's no solid linters available for Zig, that should be addressed.

Zig is an awesome language, and I wish I'd gotten to use it rather than C/C++ for the last decade. I want to love it, but this is sadly a bridge too far for me. That said, I don't expect every language to cater to my needs. Perhaps some day hearts will soften on these issues, and we'll have fewer friction points regarding this kind of static analysis. Until then, thank you all for your hard work, but I cannot use it.

JustinWick avatar Dec 27 '21 01:12 JustinWick

I'll add one final input from me. These changes, as far as I am aware are planned before the big 1.0.0 release. On top of this, the language will not change between any 1.x.x version as I understand it.

Avokadoen avatar Dec 27 '21 09:12 Avokadoen

To go over your points, in entirely my personal opinion:

  1. I disagree that needing to comment out things you're not using requires "significant mental and temporal burden"
  2. as many as it takes, but in practice not too many. and zig's great compile errors make finding and fixing these issues a breeze. additionally the code that detects this error runs almost instantly, you could add it to your editor and fix them before you ever run zig build
  3. by Zig adding this error in early, all code online already adheres to this rule. because it has to to build. as a user of 3rd party code, this is one more assurance I can make that I know Zig provides me
  4. unsure what the point here was exactly
  5. perhaps? and then _ = clearName; is documentation you're not using it.
  6. the fact this has become as contentious of an issue as it has, disproves the idea that you think that that's the minority of people. calling back to point 3, Zig having this error makes Zig code better for everyone
  7. then Zig might not be for you in this moment
  8. sure I might be using a linter, and you might be using a linter, but can you guarantee that every single one of your dependencies is adherent to and willing to fix issues brought up by that linter? because I sure can't, but what I can guarantee is that they're using the Zig compiler that asserts this error and all the other ones too.

nektro avatar Dec 27 '21 20:12 nektro