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

Require mutable variables outside scope to be qualified #8012

Open
marler8997 opened this issue Feb 13, 2021 · 7 comments
Open

Require mutable variables outside scope to be qualified #8012

marler8997 opened this issue Feb 13, 2021 · 7 comments
Labels
proposal This issue suggests modifications. If it also has the "accepted" label then it is planned.
Milestone

Comments

@marler8997
Copy link
Contributor

marler8997 commented Feb 13, 2021

Here's an interesting proposal I thought worth consideration. Require mutable variables outside the "immediate scope" to be accessed with their containing type as a qualifier. For example:

var some_var: u32 = 0;

fn foo() void {
    some_var+= 1; // compile error: cannot access varible `some_var` without a type qualifier

    @This().some_var += 1; // ok
}

Why?

The footgun here is confusing "namespace variables" with "instance variables".

const S = struct{
  var z: i32 = 0;
  fn increment(self: *S) void {
    z += 1;
  }
}

Zig veterans will immediately see the problem here; z is meant to be an instance variable but is mistakenly declared as a namespace variable. The poor soul who wrote this code has a long debug session in their future.

Proposal #2859 was also meant to address this issue but took a different route. It proposed fixing the problem at the definition site by marking instance fields with the field keyword. That proposal was rejected but the idea of alleviating this confusion still has merit. This proposal tackles the problem at the "access" site. With this proposal, rather than a runtime bug, the example above would produce a compile-error:

error: cannot access varible `z` without a type qualifier

Now the user has a choice to either qualify z with S.z or self.z, explicitly stating whether z is part of the namespace or a field of S.

One other thing to note is that I don't think accessing mutable variables outside the current scope is very common. So requiring qualification for this means that having to use a qualifier is more of a corner case and making it more verbose can actually be an advantage.

Also note that this proposal only restricts access to mutable variables. This is because this footgun is less likely with const variables because const variables more clearly not instance variables and also cannot be mutated directly (although indirect mutation is possible).

Variation

Require qualification on both mutable and const variables. This one would be a much bigger change, however, some kinds of const variables are still susceptible to the footgun because they can mutate internally. If we wanted to go this route, we could also consider whether Zig could make a distinction between const and mutable variables recursively. So the proposal could be modified to say that a variable with any mutability must be qualified.

@daurnimator daurnimator added the proposal This issue suggests modifications. If it also has the "accepted" label then it is planned. label Feb 13, 2021
@ghost
Copy link

ghost commented Feb 13, 2021

How would the following code qualify the access to x?

var x: i32 = 0;

const S = struct {
    fn increment() void {
        x += 1;
    }
};

Just testing the limits of the syntax, so pardon the contrived example.

@marler8997
Copy link
Contributor Author

Good question:

const GlobalScope = @This();
var x: i32 = 0;

const S = struct {
    fn increment() void {
        GlobalScope.x += 1;
    }
};

@ghost
Copy link

ghost commented Feb 13, 2021

I'm still split on whether this is a good idea for vars, but the above example pretty much rules out doing the same for consts, IMO. That would make global imports, definitions, etc. a huge pain to work with from within struct methods.

@marler8997
Copy link
Contributor Author

That would make global imports, definitions, etc. a huge pain to work with from within struct methods.

If you included all those as well you actually wouldn't be able to reference anything outside your immediate scope:)

Since type/imports are immutable there's no issue, the issue only applies when your variables can mutate.

@ghost
Copy link

ghost commented Feb 14, 2021

After thinking about it some more, I'm mostly against this proposal. The issue it addresses is real, but also a beginner mistake. While it would be nice to prevent it, Zig's modules-are-structs thing automatically propagates the "fix" to all mutable global variables, making them more cumbersome to use. And while global mutable state should be used in moderation, I feel that deliberately discouraging it with more cumbersome syntax is taking things too far. Prefixing all global vars with @This(). kind of hurts my eyes, though that is subjective of course. Finally, this creates an inconsistency in the language, because local consts and vars are accessed the same way, but outerstruct/global-scope consts and vars are not.

Individually, none of these objections are deal-breakers, but taken together, they tip the balance against doing this, IMO.

@marler8997
Copy link
Contributor Author

marler8997 commented Feb 14, 2021

  • is a beginner mistake

Agreed. I would also add that this mistake will be easier to make for those who work more in other languages since alot of languages use the same syntax for local variables and instance fields.

  • makes using mutable globals more cumbersome to use

Agreed. However, in some ways I see this as an advantage because it discourages use of global mutable variables. But like you say, when you really need to do this it's going to look more ugly. I'm personally not sure whether this one goes in the Pro or Con column.

  • creates an inconsistency in the language

This is true but I'm not seeing why this is bad. Does this cause some sort of footgun? The worst case I can think of is the user gets a compile error for not qualifying their mutable global state even though that's what they really want to do, the best case being that it wasn't their intention and a nasty bug was just caught by the compiler. Inconsistency isn't inherently good or bad, even though it's commonly use to describe things that are bad. For example, Zig now requires "arrayish" types be formatted with a specifier because the user's intention is ambiguous, but it doesn't require one for integer types. This is inconsistent but I would say its a good inconsistency that makes the library "easier to use correctly". I believe the same argument applies here.

To be clear, I'm on the fence about this one. This particular footgun isn't a problem for me because I write alot of Zig code, but I can empathize with others who don't have as much experience with Zig. Ironically, it's likely the newer users who will have the most relevant perspective on this.

@ghost
Copy link

ghost commented Feb 14, 2021

creates an inconsistency in the language

This is true but I'm not seeing why this is bad. Does this cause some sort of footgun? [...] Inconsistency isn't inherently good or bad, even though it's commonly use to describe things that are bad.

No footgun -- it's just one more rule to learn about the language. Once you are familiar with it, it shouldn't matter much. But as a beginner you would probably stumble over it once or twice.

@Vexu Vexu added this to the 0.8.0 milestone Feb 21, 2021
@andrewrk andrewrk modified the milestones: 0.8.0, 0.9.0 May 19, 2021
@andrewrk andrewrk modified the milestones: 0.9.0, 0.10.0 Nov 23, 2021
@andrewrk andrewrk modified the milestones: 0.10.0, 0.11.0 Apr 16, 2022
@andrewrk andrewrk modified the milestones: 0.11.0, 0.12.0 Apr 9, 2023
@andrewrk andrewrk modified the milestones: 0.13.0, 0.12.0 Jul 9, 2023
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