ziglang / zig

General-purpose programming language and toolchain for maintaining robust, optimal, and reusable software.
https://ziglang.org
MIT License
34.09k stars 2.49k forks source link

compile errors for unused things #335

Open andrewrk opened 7 years ago

andrewrk commented 7 years ago

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.

ghost commented 5 years ago

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?)

andrewrk commented 5 years ago

You could disable an error by doing _ = the_unused_variable.

ghost commented 5 years ago

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 commented 5 years ago

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?

andrewrk commented 3 years ago

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 commented 3 years ago

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.

Avokadoen commented 3 years ago

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?

andrewrk commented 3 years ago

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

tauoverpi commented 3 years ago

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.

andrewrk commented 3 years ago

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

ryuukk commented 3 years ago

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

andrewrk commented 3 years ago

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).

Ristovski commented 3 years ago

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

jedisct1 commented 3 years ago

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.

name-changer commented 3 years ago

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.

Avokadoen commented 3 years ago

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

ekipan commented 3 years ago

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 ....
}
emekoi commented 3 years ago

~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 ....
}
ekipan commented 3 years ago

@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.

errantmind commented 3 years ago

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.

ghost commented 3 years ago

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.

bb010g commented 2 years ago

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.

JustinWick commented 2 years ago

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)

Avokadoen commented 2 years ago

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.

ryuukk commented 2 years ago

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

nektro commented 2 years ago

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

Avokadoen commented 2 years ago

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.

JustinWick commented 2 years ago

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.

Avokadoen commented 2 years ago

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.

nektro commented 2 years ago

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.
errantmind commented 2 years ago

The underscore assignment trick existing at all goes against the reason for this change. I argue requiring a kludge is worse than just allowing it in the first place.

And yes, there definitely is mental burden to commenting a line out, because that line has a good chance of containing another variable that is now unused causing an error. It is a chain reaction.

The stated reason for this is that Zig is meant to support maintenance more than new dev but I think it will encounter a chicken or the egg problem. There won't be any software to maintain if people don't build it in the first place.

Perhaps you are right though, Zig just may not be 'for me'. That is ok. You may find you are bisecting potential adopters more than you thought though.

I want to add that I'm vocal about this issue because I like Zig, not because I don't like it. If I didn't like it I wouldn't be here commenting.

GreenBeard commented 2 years ago

I just wanted to say that as someone just trying out Zig for the first time: This feature is causing me to downgrade to 0.8.1 for now as it makes learning much harder. Having the mentality of "no warnings" because it is undesirable in production code feels ridiculous to me; people can write bad code even if you force all warnings to be errors. Also, warnings are fine in code that is a work in progress. I agree that warnings are bad in production code (and probably should be banned there). That is just my two cents :man_shrugging:.

ghost commented 2 years ago

A lot of the frustration that shows through in this thread (and several related ones) is ultimately rooted in Zig's One Way to Do Things. Don't get me wrong -- this is a sound design principle in general. You do not want to be in a situation like C++, where there are often three different ways to do almost the same thing, but none of them does exactly what you want, and all of them introduce subtle incompatibilities under composition. In a clean sheet design it should be one of the main design goals to avoid this -- to have an idiomatic way of doing things whenever possible.

At the same time, care should be taken not to overdo it, lest One Way to Do Things becomes One Size Fits All. It doesn't. Everyone will not use the same coding style and workflow, no matter how good it is. Not all projects will have the same quality, security and performance requirements. Not all application domains require the same language features. A programming language meant for broad adoption should account for such differences where possible. It's a delicate balancing act. On one hand, it would not be good to dilute the core design philosophy in an attempt to please everybody. But neither is it good to overfit the language to the preferences of a single developer, even a highly capable and productive one.

I don't think there's a general rule for deciding such issues, but in this particular case, my gut feeling is that a hard error for unused items will hurt people who prefer iterative development a lot more than it will benefit maintainers to be sure that warnings are always enabled without exceptions.

Sorry for rambling. I guess the TL;DR is: If Zig had warnings and not just hard errors, we wouldn't be having this discussion.

JacekAndrzejewski commented 2 years ago

@JustinWick

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

This is something that could be helped without demotion to warnings: either filtering of compile errors (a flag that would only hide errors from the report, not change compilation logic), generation of compile error reports with grouping or some way to generate a baseline of compile errors. All of those do help in your case and don't go against the vision of Zig (judging by this whole discussion and similar ones).

I think options like that would be really helpful, because even if you try to keep people from writing code with errors it's still possible to write hundreds of lines of code without ever trying to compile.

TL;DR there is no reason for compile errors and report of errors being tightly coupled, with no way to influence reporting.

marler8997 commented 2 years ago

I wanted to share one idea I've had in the back of my mind for those that feel this is a showstopper or reason for staying away from Zig. If there are enough people that care about this issue, then a fork of Zig (maybe "zig-sloppy") could be maintained alongside the main project that accepts "sloppy code". Developers could use "zig-sloppy" until they're ready to finalize their changes and call the real "zig" compiler.

For those who really want to use Zig and think this is an important issue it may also be worth maintaining such a fork to get data on how much it would take to maintain it and how many people would actually use it. The last point of Zig Zen is to "Together we serve the users", if enough users want this and are willing to put in work for it then I'd say it's likely Andrew would change his mind on adding a "sloppy mode", but even if not, there's nothing stopping anyone from using this mechanism to address this issue.

nektro commented 2 years ago

that would splinter the ecosystem and tremendously hurt Zig at large. that's the reason this isn't being reverted. the error being in now means that all code that you depend on and run adheres to the same standard of Zig. and that's a wonderful thing.

ghost commented 2 years ago

@nektro

the error being in now means that all code that you depend on and run adheres to the same standard of Zig. and that's a wonderful thing.

  1. This error in no way guarantees high code quality. At best, it is a small contributor. At worst, it may reduce quality by annoying developers and lowering their overall effectiveness.
  2. Likewise, this error not being there does not preclude high code quality.
  3. If the project is not public, then the standards it may or may not adhere to are none of your (or my) business.

Most of the benefits of catching unused items can already be harnessed with a warning. The additional benefit of making it an error seems to be small (to me at least). The negative impact on iterative development and debugging is felt by many to be quite large.

JustinWick commented 2 years ago

@nektro:

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.

I appreciate your response, but in the interests of clarity I'd like to respond in kind:

  1. If you are lucky enough to use code where commenting out just the parts you need to for debugging, etc, is easy under these stringent error requirements, then congratulations: you have the life I want. Commenting out a few lines of code is generally not a big deal if one's tracking down some simple bug in code they themselves wrote, but I have regularly worked on 500+ line functions (not my doing) written years ago by multiple people (not me) performing complicated operations. This debugging might encompass any of dozens of potential culprits, and has required me to hold a complex mental model of hundreds of thousands of lines of algorithmically complicated code in my head, pushing my limited debugging abilities near the breaking point. I was lucky in that I could throw in an #if 0 and turn off warnings so that I could get on with my actual work. This was possible because the tools were designed with the philosophy that it is the developer on the scene, deeply immersed in the code, not the compiler implementer, who knows best which potentially harmless code smells are a problem in debug builds. Marginal mental cost to appeasing the demands of the compiler increase relative to the current mental cost of the operation involved--every moment I spend thinking about how to make this spurious error go away and re-running the build process because the compiler refused to let me build my code is one less I spend figuring out why an incredibly complex piece of software is not functioning in a specific instance. If the mental burden is already near my limit this becomes increasingly less acceptable. Indeed, as @errantmind mentioned there is often a transitive quality to code elimination when unused bound values (or unbound return values) are considered an error, and this further magnifies the effect. The magnification is important because the mental cost scales nonlinearly with each added impediment. I'm sure many programmers have significantly better working memories than I do and thus feel no burden here, but I see no reason to punish the rest of us for completely harmless code smells during a debugging session.

  2. That's only true if the affected code is small. This kind of warning/error is not unique to Zig, and is not a breeze on large legacy codebases. I have dealt with changes that required me to spend days writing code just to automate modifying existing code to be correct to remove warnings, etc, when a relatively small but pervasive change was introduced to a codebase. Having no way to quantify the level of effort involved is a serious problem when managing a large project. Heck, if I'm going to have to change 40 files to do a single refactor (not unusual for me), I might simply rethink it.

  3. Completely agreed for release code. Again, why I am willing to accept this choice for release builds, even if I'm not thrilled about it. Completely disagree for debug code because it does not generally apply, and other than some vague notion of "purity" I have trouble understanding how anyone could be worried about debug code being held to a lower standard.

  4. Good question, and apologies for not being clearer. When there's, say, a return value that might be potentially useful in the future, or useful to print out in code that's commented out (but can be optionally un-commented for a debugging session) it is sometimes easier and more maintainable to capture things like return values that aren't actually used in current code. This is not a huge concern of mine, but if your codebase is large and complicated, these kinds of things can be nice. Also, by naming the return value, the reader has some idea of what that return value is, whereas _ tells the reader to go try and figure out where the docs are and what's going on themselves. You can safely ignore this point if you wish, it's probably the least the concerns about this change.

  5. Agreed, but that documentation is only useful for production code, and even then it only means you're not using it on some builds. Transient debugging code does not require this documentation, it's just more noise the debugging programmer needs to look at. See my point 1.

  6. The fact that it's contentious proves only that people don't agree on it. That's nice, programmers tend not to agree on things, it's more of a craft than a science. But in this particular case, this is not a small disagreement, it's not even merely philosophical, there are real-life ramifications of this change that have been documented by multiple people on this chain. There is also absolutely no demonstration whatsoever by anyone on this discussion that having this as an error during the debugging process leads to any improvement in Zig code, and I would be surprised if such a thing is possible. If the intent is to keep the Zig community focused on the sorts of people who enjoy constant nannying even during things that don't improve code quality (debug builds) then this change will help avoid future disagreements by convincing potential heretics and apostates to use something else.

  7. That's the entire point of my comments, and again, I don't think any programming language needs to be for everyone. I really like Zig, am impressed by what I see; I want to be able to use it, and recommend it to others. As it stands I will not be able to use it, nor recommend it to others.

  8. I can't guarantee that any code anyone writes is good, linter or not. I don't trust any code written in a systems programming language, as frequently they involve unsafe operations. I get what I get and hope someone pays me for whatever I'm able to put together using what's available. I'm much more concerned about code that's improperly designed or tested than if someone didn't use some variable or return value in a particular build. But regardless, if this is only an error in the release build it will still perform the function you wish here--there is absolutely no reason it must be an error for non-release builds.

Again, I absolutely love that the Zig compiler performs this analysis, and I would grudgingly accept it for release code (even though that is a nontrivial problem as I have mentioned in my earlier post about importing external legacy libraries meant for earlier versions of a Zig compiler). And you make good points that forcing release builds of random libraries to at least pass this check might clean up the ecosystem a bit. I'd go further to say that the Zig community should pick a linter (Ziglint?) and have package manager websites and GitHub display lint status. In fact, it might not be a bad thing to add to Gyro, a warning that pops up any time a library that doesn't pass Ziglint is installed, and an option to perform an audit for this. That might put additional pressure on library developers to fix or document (via warning suppression) their use of lint-unfriendly code.

But I'm the one on the scene, the one with the deadline, the one who knows the needs and goals of the project, the cost/benefit/risk of this kind of code--and if the language designers consider schedule risk on large projects to be an unimportant factor, then this language should be relegated to small libraries, hobby code and things like games that have little consequence if the development cycle unexpectedly takes a hit. Unnecessarily complicating and slowing down the debug/build/test cycle or refactor/build/test cycle and injecting more uncertainty (especially into the latter) makes predictable deliveries a problem for the intangible benefit of making debug builds "cleaner".

I'd also like to unequivocally agree, on the record, with the statements from @GreenBeard, @errantmind and especially @zzyxyzz. "This is not a problem" seems to be a demonstrably false statement at this point. "We are not willing avoid preventing a certain set of potential problems by appealing to people for whom this hardline stance is itself a problem" is a reasonable, if lamentable statement. Again, how developer-friendly the Zig language and compiler designers want to make the language and compiler is up to them.

I appreciate that you have made your efforts open source so that people who have to deal with the compiler have the option of modifying it to be more friendly to their project. I also wish all of you good luck in finding whatever balance you choose.

nektro commented 2 years ago

great points, thanks for the response :) It being in Zig isn't my decision but I will add unused variable checks to https://github.com/nektro/ziglint

JustinWick commented 2 years ago

great points, thanks for the response :) It being in Zig isn't my decision but I will add unused variable checks to https://github.com/nektro/ziglint

I'm a huge fan of linting programs, so thanks for your contribution; I pyflakes for Python and it's really saved my bacon. It's a lot less necessary in strongly-typed languages, but systems languages tend to be weakly typed, like C. Unfortunately I've not been able to use much in the way of linting at my day job with C/C++ because of too many false positives, but we definitely tried!

ryuukk commented 2 years ago

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 ....
}

is that the official way to deal with it? removing the name of the argument? what if i forget what the name was? what if i swap the order of the arguments??

pub fn do_this(a: f32, b: f32) void {
}
pub fn do_this(_: f32, _: f32) void {
}
pub fn do_this(b: f32, a: f32) void {
}

oops

nektro commented 2 years ago

pub fn do_this(_: f32, _: f32) void is intended to work, it just doesn't yet in the current compiler

andrewrk commented 2 years ago

pub fn do_this(_: f32, _: f32) void is intended to work, it just doesn't yet in the current compiler

It's not intended to work as a way of dismissing compile errors for unused parameters, because this would lead to deleting useful information (the parameter name).

InKryption commented 2 years ago

What if something like fn func(_ paramater_name: T) void was supported? Sort of like what was proposed in #10245, but without switching _ for a keyword.

ryuukk commented 2 years ago

for the people who are turned off by this compile error, don't give up yet!

zig is an amazing project, that would be sad to miss the opportunity to try it because of this (i was; almost, until i tried this workflow and found peace again)

pragmatic workflow zig is very very easy to compile https://github.com/ziglang/zig/wiki/Building-Zig-From-Source (thanks for making it easy!) so if you like me and don't like wasting time during development and want fast iteration time over anything else, then comment out the lines that generate the error and setup an alias so whenever you are prototyping, just use that alias that points to your local zig with the change, and you are ready to go ``zig build x`` for when you contribute to existing code ``zag build x`` for when you prototype things this way it doesn't stop you from using zig, you can learn to appreciate the compile error as you go i personally like the compile error when i'm working on existing code, the design is already laid out and it helps maintain good and readable code ```diff diff --git a/src/AstGen.zig b/src/AstGen.zig index 7c855fb..31952f1 100644 --- a/src/AstGen.zig +++ b/src/AstGen.zig @@ -2469,11 +2469,11 @@ fn genDefers( } fn checkUsed( - gz: *GenZir, + _: *GenZir, outer_scope: *Scope, inner_scope: *Scope, ) InnerError!void { - const astgen = gz.astgen; + //const astgen = gz.astgen; var scope = inner_scope; while (scope != outer_scope) { @@ -2482,14 +2482,14 @@ fn checkUsed( .local_val => { const s = scope.cast(Scope.LocalVal).?; if (!s.used) { - try astgen.appendErrorTok(s.token_src, "unused {s}", .{@tagName(s.id_cat)}); + // try astgen.appendErrorTok(s.token_src, "unused {s}", .{@tagName(s.id_cat)}); } scope = s.parent; }, .local_ptr => { const s = scope.cast(Scope.LocalPtr).?; if (!s.used) { - try astgen.appendErrorTok(s.token_src, "unused {s}", .{@tagName(s.id_cat)}); + // try astgen.appendErrorTok(s.token_src, "unused {s}", .{@tagName(s.id_cat)}); } scope = s.parent; }, ```
bb010g commented 2 years ago

for the people who are turned off by this compile error, don't give up yet!

zig is an amazing project, that would be sad to miss the opportunity to try it because of this (i was; almost, until i tried this workflow and found peace again)

This really should be a compiler flag; not a separate copy of the Zig compiler that people have to discover & install. (No offense to @ryuukk; this should be functionality that's built-in to Zig).

JustinWick commented 2 years ago

for the people who are turned off by this compile error, don't give up yet! zig is an amazing project, that would be sad to miss the opportunity to try it because of this (i was; almost, until i tried this workflow and found peace again)

This really should be a compiler flag; not a separate copy of the Zig compiler that people have to discover & install. (No offense to @ryuukk; this should be functionality that's built-in to Zig).

I think the point of @ryuukk's idea (and correct me if I'm wrong, @ryuukk) is that we're simply not going to get that compiler flag given the culture that's been established. The compiler designers have made the clear through the last nearly 4 years of people discussing this serious and divisive problem. The designers are the ones who are putting in their life's effort, and that decision belongs to them, and rightly so.

Luckily they've been incredibly altruistic in releasing the Zig compiler (and related work) under a license that lets people modify it to suit their needs. Part of the joy of Free/Libre Open Source Software is that no single release of a software artifact has to be everything to all people; users and the community are free to choose and create our own destiny. Perhaps a friendly fork that supports developer-empowering error control would prove/disprove the notion that such a thing is needed, and perhaps carefully isolate the complainers (like me, see my decidedly unconcise walls of text above) away from the people who are working hard and making Zig possible. But, perhaps Zig will eventually take the pragmatic path and end up like Python, where they once said "There's One Way to Do It" and are now somewhere between "You Don't Need Both Hands to Count How Many Ways There Are To Do It" and "Hey, At Least It's Not Perl".

Either way, I think someone who is more experienced with Zig should create such a fork, at least for the purposes of experimenting with how this feature might work. It might not go anywhere, but it will extend the amount of time some people are able to productively use Zig.

This design decision has rendered this compiler literally unusable for my day job, but I respect the designers in their convictions and their goals: to create a language that supports and encourages simplicity, clarity, and code you can count on, even if it was written by someone else. It's a damn tough tightrope to walk, and everyone has a weight pulling them well to one or another side of that rope.

name-changer commented 2 years ago

A few months ago I found a message in Discord suggesting @ryuukk s solution: just 'fix' the compiler. Building the compiler is very straightforward, only compiling LLVM takes a bit of time. Then in AstGen.zig in CheckUsed() add a return; at the top of the function. Problem solved.

I added two shell-scripts to switch between the 'tarball compiler' and the 'source compiler'. It's really convenient, and basically exactly what I wanted in the first place.

As long as we can do this, I have no beef with these errors anymore. If you are interested in Zig and this holds you back or annoys you, it's definitely worth to try this.

marler8997 commented 2 years ago

I was thinking of creating a "friendly fork" and CI that could automatically create binaries and maybe even try to automatically merge changes from the main repo(sounds like a fun little project to me), but everyone seemed to hate the idea when I suggested it (10 downvotes): https://github.com/ziglang/zig/issues/335#issuecomment-1005308673

But now I've read a few comments that people want this? Leave a rocket 🚀on this comment if you think you would use this.

jean-dao commented 2 years ago

I think it is fine to solve this issue with external tools, this whole discussion has prompted me to try some experimentation here. This is for now only supporting unused vars/parameters and unreachable code. Supporting more will mean basically re-implementing a compiler front end, so it's unclear if this is worth the effort versus maintaining a fork of the compiler. On the other hand having an external tool instead of having to use a different compiler might be more user-friendly.

Anyways I don't see the issue with this being solved outside of the main compiler project.