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

Disallow discarding a value if a variable is used #9238

Closed
ikskuh opened this issue Jun 25, 2021 · 8 comments
Closed

Disallow discarding a value if a variable is used #9238

ikskuh opened this issue Jun 25, 2021 · 8 comments
Labels
accepted This proposal is planned. enhancement Solving this issue will likely involve adding new logic or components to the codebase. proposal This issue suggests modifications. If it also has the "accepted" label then it is planned.
Milestone

Comments

@ikskuh
Copy link
Contributor

ikskuh commented Jun 25, 2021

Zig forces you to reference or discard every value, including function parameters. It might be harmful in bigger functions that will use a discarded value later in the code, so i propose to forbid this code:

{
    var x: u32 = 0;
    _ = x;
    foo(x); // error: use of discarded value `x`. It was discarded here: ...
}

This will prevent misleading discards of values where the value isn't actually discarded, but was only marked "i don't need this right now" by a (potentially different) programmer

@Hadron67
Copy link
Contributor

I like the idea of this proposal, but how does this work for something like _ = (a);, i.e., the variable comes from a subexpression of rhs? We could disallow reusing of these variables as well, but I don't think it's feasible if the variable is used in a function call. If we allow them, statements like _ = <variable> and _ = <expr> would have different semantics.

@Vexu Vexu added the proposal This issue suggests modifications. If it also has the "accepted" label then it is planned. label Jun 26, 2021
@Vexu Vexu added this to the 0.9.0 milestone Jun 26, 2021
@yohannd1
Copy link

yohannd1 commented Jul 4, 2021

I've been thinking about essentially the same thing recently, but I think this _ = <variable> syntax seems more complex (as @Hadron67 mentioned).
I'm not sure if it would be the best to introduce a new keyword, but we could have discardvar or something like that instead of the _ =:

{
    var x: u32 = 0;
    discardvar x;
    foo(x); // error: use of discarded value `x`. It was discarded here: ...
}

Also, discardvar could also take more than one identifier at once:

{
    var x: u32 = 0;
    var y: u32 = 0;
    discardvar x, y;
    foo(x); // error: use of discarded value `x`. It was discarded here: ...
    foo(y); // error: use of discarded value `y`. It was discarded here: ...
}

but I'm not sure if it would be a good idea since it would be inconsistent with variable declarations (there is no way to declare two different identifiers on a signle statement).

@ghost
Copy link

ghost commented Jul 4, 2021

Fixing an "unused" error for a local variable shouldn't be done with _ = anyway. The proper thing to do will almost always be to delete, comment out, or change the name to _, depending on context. The exception to this are function parameters, which cannot always be deleted because they may be part of an API, and cannot be named _ without losing their documentation and introspection value.

The canonical way to discard a parameter in such a situation is to _ = it at the top of the function. This has three problems: First, it doesn't actually discard anything, it merely touches, and the parameter is still available for use afterwards. Then there is the verbosity, as you have to write out the name of a parameter that you don't even need, twice. Finally, if there are multiple discards, then you have to scan back to the parameter list and visually diff to determine which parameters are actually used. @Avokadoen has a nice example of both of the latter points in another thread #335 (comment).

This proposal addresses only the first problem. But the context-sensitivity thing makes me unsure whether to like it or not. And the other two problems would remain either way. To Express Intent Precisely it would be better to introduce a semantic annotation for discarding parameters, like fn(discard x: T, ... or unused maybe. @fengb's idea (#9239) of making unused params start with an underscore was already vetoed, but I then suggested making the name an optional parameter to _ (like this: fn(_(name): T, ...). I think acceptable syntax could be found, if wanted. On the other hand, introducing a language feature, for this? Hmm. I suppose it would depend on how often this actually comes up when working with APIs.

/ End rant.

@yohannd1
Copy link

yohannd1 commented Jul 5, 2021

Fixing an "unused" error for a local variable shouldn't be done with _ = anyway. The proper thing to do will almost always be to delete, comment out, or change the name to _, depending on context.
Now that I think of it, I probably misunderstood the intent of the proposal, and I think I agree with you then.

I was thinking on improving a coding pattern I've come across before but I think it would be better with a proposal I'm working on (mutable captures and function parameters):

pub fn func(arg: SomeStruct) SomeStruct {
    var arg_ = arg;
    // mutate arg_
    return arg_;
}

By the way, there's another problem - if discardvar can be applied to a variable that is used (but won't be in the lines after its discarding) this feature might make the programmer spend too much effort on just deciding when to use it. For example:

const x = 10;
doSomethingWith(x);

const x1 = 20;
const x2 = 30;
doSomethingWith(x1 + x2);

This is perfectly valid code, but someone could argue that, since x is not used after the declaration, there should be a discardvar after it:

const x = 10;
doSomethingWith(x);
discardvar x;

const x1 = 20;
const x2 = 30;
doSomethingWith(x1 + x2);

Someone could even argue that x1 and x2 should also be marked as discardable later on, in case the code gets extended later.

const x = 10;
doSomethingWith(x);
discardvar x;

const x1 = 20;
const x2 = 30;
doSomethingWith(x1 + x2);
discardvar x1, x2;

I think this ends up violating the "only one obvious way to do things" lemma.
To be honest, I think there are some other proposals which are also subject to this problem, but in this specific one there I feel like there aren't really that many situations where it'd be useful to have this mechanism (correct me if I'm wrong, please) and then this could end up hurting more than helping.

Still, if we decide to add it in, I guess we could have a set of conventions to mitigate the problem. But then:

  • Which rules would there be? How to decide them?
  • How would these conventions be enforced? By the compiler, or just in a separate document?
  • Conventions for this would make programming "ideal" Zig code - maybe just a little bit, but still - harder

@mjoerussell
Copy link

I agree that handling unused parameters with _ = x definitely feels like a hack right now. It feels like you're trying to trick the compiler rather than actually establishing that a variable is unused.

Personally if there's going to be a change for this I'm in favor of a new keyword - something like unused - that is very strict in it's semantics. I'm thinking like this:

// A...
fn a(x: u32) u32 {
  unused x; // mark the input as unused
  return x + 10; // error: x was used after being marked unused
}

// B...
fn b(x: u32) u32 {
  x += 20;
  unused x; // error: x was marked unused after being used
  return 100;
}

// C...
fn c(x: u32) u32 {
  // replaces _ = callFn(...)
  unused iDontCareAboutWhatThisFnReturns(x);
  return x;
}

// D...
fn d(x: u32) u32 {
  unused x;
  const x = 20; // error - name shadows unused identifier
  return x + 10;
}
  

I think that the strictness here of not allowing unused vars to:

  • Be referenced after being marked unused,
  • Be marked unused after being referenced,
  • Be shadowed after being marked unused,

will make it clear to anyone reading the code that, if they see unused x, they can be guaranteed that the value of x is never used anywhere in the scope. It should be encouraged to use unused x in the beginning of a function/scope for clarity, but not necessarily required.

Replacing _ = callFn() with unused callFn() is just for consistency, although I'm not sure if it's necessarily better.

@VojtechStep
Copy link

VojtechStep commented Jul 10, 2021

I believe the fourth example fails to compile even now, since variable shadowing is already disallowed.

@mjoerussell
Copy link

Yeah it would, but I'm just putting it there to demonstrate that marking x as unused wouldn't unset/reset the name or anything.

@andrewrk andrewrk modified the milestones: 0.9.0, 0.10.0 Nov 24, 2021
@andrewrk andrewrk modified the milestones: 0.10.0, 0.11.0 Apr 16, 2022
@andrewrk andrewrk added enhancement Solving this issue will likely involve adding new logic or components to the codebase. accepted This proposal is planned. labels Apr 9, 2023
@andrewrk
Copy link
Member

andrewrk commented Apr 9, 2023

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
accepted This proposal is planned. enhancement Solving this issue will likely involve adding new logic or components to the codebase. proposal This issue suggests modifications. If it also has the "accepted" label then it is planned.
Projects
None yet
Development

No branches or pull requests

7 participants