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

Add compile error for overwriting a value that was never used #20250

Open
TheHonestHare opened this issue Jun 10, 2024 · 2 comments
Open

Add compile error for overwriting a value that was never used #20250

TheHonestHare opened this issue Jun 10, 2024 · 2 comments
Labels
proposal This issue suggests modifications. If it also has the "accepted" label then it is planned.
Milestone

Comments

@TheHonestHare
Copy link

Proposal

Add a compiler error for when you overwrite the value in a local var, but have never used it beforehand. The following code would produce an error:

var x: u32 = 3;
foo(x);
x = 5; // valid, foo(x) uses x
x = 4; // error

In addition, overwriting undefined would be exempted from this. This remains valid:

var x: u32 = undefined;
x = 3;

What counts as using?

  • Getting the value of a variable via its name
  • Getting the address of the variable will mark it as having being used throughout the rest of its life
  • Conditionally using the variable. This should include branches comptime-known to never happen, because if not conditional compilation might become hell on earth
var x: u32 = 0;
if(!(comptime onWindows())) {
    setupThing(x);
x = 2; // this is now a compile error, but only if you're targetting Windows :)
  • Discarding (_ = foo) would show the reader that the value is intentionally being discarded
  • volatile is always marked as having being used
  • foo_ptr.* marks foo_ptr as used (it's the same as reading the value of foo_ptr, then going to that address in memory)
  • Calling a method on it or accessing one of its members
  • Using operators such as +=, -=, *=, etc

Assigning to the variable would reset its used flag unless a pointer has been taken to it or it is volatile. Assigning to a variable that doesn't have its used flag set would be a compiler error.

Details to flesh out

Should overwriting struct fields be a compiler error?

i.e. should var x: Foo = .{}; x.bar = 4; be a compiler error?
I think not, just because there are legitimate use cases for something like this:

const Foo = struct {
  pub const special_config: Foo = .{.x = 2, .y = 1, .z = 29};
  x: u32 = 0, y: u32 = 0, z: u64 = 0
};
var result: Foo = Foo.special_config;
result.x = actuallyIWantThisValue();
// . . .

Should overwriting global vars be a compiler error?

I also think not

  • Could be/not be used from a different function
  • Could be/not be used from a different file
  • Could be/not be used from a different thread

You would have to check ALL occurrences of the global variable, leading to confusing error messages caused by very far away code. This change should be similar in scope to the "local variable is never mutated" change.

Does overwriting the variable have to unconditionally happen?

i.e. should this code produce an error?

var x: u32 = 3;
if(myCondition()) x = 4;

I'm tempted to say that this code should error, because if you've NEVER read 3 from x beforehand then why didn't you decide if it should be 3 or 4 with var x = if(myCondition()) 4 else 3. However, there may be legitimate usecases for something like this I can't think of.

Should detecting overwrites happen in complex control flow (i.e. loops)?

I want to say yes, but I don't know how much more complicated that would make the detection. In particular, consider:

var x: u32 = undefined;
while(true) {
    x = 1;
    foo(x);
    x = 0;
}

In a perfect world, this should definitely error; however, detecting such cases might make implementing this somewhat more complex (in this case it would just have to go through the loop a second time, but there could be edge cases I can't think of) especially since this is basically a lint check and there's technically not anything WRONG with the code itself. It may not be worth the extra effort.

@Vexu Vexu added the proposal This issue suggests modifications. If it also has the "accepted" label then it is planned. label Jun 10, 2024
@Vexu Vexu added this to the 0.14.0 milestone Jun 10, 2024
@rohlem
Copy link
Contributor

rohlem commented Jun 10, 2024

One scenario that frequently comes up for me is copying a value before changing only some of the fields:

fn canonicalize(c: Config) Config {
  var result = c;
  result.x = result.x orelse default_x;
  result.y = result.y orelse result.x.?;
  // ... change or keep other fields
  return result;
}
fn canonicalizeLessReadable(c: Config) Config {
  return .{
    .x = c.x orelse default_x,
    .y = c.y orelse (c.x orelse default_x),
    // ... change or keep other fields
  };
}

The idea about flagging based on assignments in branches also feels too strict to me.
Intuitively I think front-loading what could be a multi-step process
(with semantic grouping of several assignments/checks, space for explanatory comments, etc.)
to a single earlier point can often reduce readability.
But to be fair, I also can't think of a clear convincing example at the moment.

I generally think this check would be very useful, say you insert an assignment and don't notice it has no effect.
It's possible that with enough scrutiny even the additional scenarios listed would always lead to more canonical/regular code shapes.

To me the strongest point against this proposal would be an example of a a small, contained change without this restriction,
which would have to balloon to several, disparate changes, or require larger restructuring of the code to satisfy this new requirement.
I assume the escape hatch of _ = x; discarding could always prevent this, but it would also be unfortunate if real code bases had to resort to this too often.

@wooster0
Copy link
Contributor

related: #12047

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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

4 participants