Skip to content

std: fuck usingnamespace #19214

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

Merged
merged 8 commits into from
Mar 8, 2024
Merged

std: fuck usingnamespace #19214

merged 8 commits into from
Mar 8, 2024

Conversation

mlugg
Copy link
Member

@mlugg mlugg commented Mar 7, 2024

see title

@Rexicon226
Copy link
Contributor

ci failure: #19089

@mlugg
Copy link
Member Author

mlugg commented Mar 7, 2024

Whoops, I accidentally flipped the conditions in that commit.

@mlugg mlugg force-pushed the fuck-usingnamespace branch 4 times, most recently from 7431524 to 3c37d31 Compare March 7, 2024 23:22
@rofrol
Copy link
Contributor

rofrol commented Mar 7, 2024

Why fuck?

@mlugg
Copy link
Member Author

mlugg commented Mar 7, 2024

The PR title is kind of a joke, but: the usingnamespace construct is problematic for implementing incremental compilation, and broadly speaking, tends to result in worse, less readable code. It may be removed from the language in the future. We are replacing all uses of it in std, to clean up code as well as help us evaluate potential use cases for the feature.

@mlugg mlugg force-pushed the fuck-usingnamespace branch from 3c37d31 to c8880df Compare March 8, 2024 08:02
mlugg added 8 commits March 8, 2024 08:02
This is a trivial change - this code did `usingnamespace` into an
otherwise empty namespace, so the outer namespace was just unnecessary.

Eliminates one more usage of `usingnamespace` from the standard library.
This usage of `usingnamespace` was removed fairly trivially - the
resulting code is, IMO, more clear.

Eliminates one more usage of `usingnamespace` from the standard library.
Searching GitHub indicated that the only use of these types in the wild is
support in getty-zig, and testing for that support. This eliminates 3 more uses
of usingnamespace from the standard library, and removes some unnecessarily
complex generic code.
Eliminates one more usage of `usingnamespace` from the standard library.
I have no idea why this was even here...

Eliminates one more usage of `usingnamespace` from the standard library.
5 remain.
Thanks to Zig's lazy analysis, it's fine for these symbols to be
declared on platform they won't exist on. This is already done in
several places in this file; e.g. `pthread` functions are declared
unconditionally.

Eliminates one more usage of `usingnamespace` from the standard library.
4 remain.
Some of the structs I shuffled around might be better namespaced under
CONTEXT, I'm not sure. However, for now, this approach preserves
backwards compatibility.

Eliminates one more usage of `usingnamespace` from the standard library.
3 remain.
* `linux.IO_Uring` -> `linux.IoUring` to align with naming conventions.
* All functions `io_uring_prep_foo` are now methods `prep_foo` on `io_uring_sqe`, which is in a file of its own.
* `SubmissionQueue` and `CompletionQueue` are namespaced under `IoUring`.

This is a breaking change.

The new file and namespace layouts are more idiomatic, and allow us to
eliminate one more usage of `usingnamespace` from the standard library.
2 remain.
@mlugg mlugg force-pushed the fuck-usingnamespace branch from c8880df to 265f42d Compare March 8, 2024 08:02
@mlugg
Copy link
Member Author

mlugg commented Mar 8, 2024

@mitchellh and @kprotty (best ping determined from blame), I notice that libxev and tigerbeetle are two notable consumers of our io_uring API; do the changes I've made here look reasonable? It's just some renames and small reshuffles.

  • linux.IO_Uring --> linux.IoUring to align with naming conventions
  • All io_uring_prep_xyz are now prep_xyz methods on io_uring_sqe (I think this is only an internal detail?)
  • SubmissionQueue and CompletionQueue are namespaced under IoUring

@Inve1951
Copy link
Contributor

Inve1951 commented Mar 8, 2024

I'd argue that io_uring_sqe should be named IoUringSqe (or IoUring.SubmissionQueueEntry) and its prep* methods should return an instance of it instead of consuming a pointer.

Edit: And then maybe instead of calling them prep* calling them init*.

@mlugg
Copy link
Member Author

mlugg commented Mar 8, 2024

I agree that would probably make sense, but it can be a future enhancement. For now, I'm more concerned with validating that this new API - mainly motivated by the removal of this usingnamespace - is reasonable for end users.

@Jarred-Sumner
Copy link
Contributor

This is probably the wrong issue to mention this but I’d be really sad if usingnamespace is removed from Zig without an adequate replacement. We use it frequently in Bun for interface boundaries between different parts of the codebase and for ergonomically mixing handwritten code with generated code. I definitely agree it’s not perfect, but removing it without addressing any of the problems it solves would not be better for Zig’s users.

@kprotty
Copy link
Member

kprotty commented Mar 8, 2024

The io_uring refactors seem fine (namespacing io_uring_sqe under IoUring also seems appropriate given the other changes). The potential future usingnamespace replacements are verbose too, but in the end doable.

@andrewrk andrewrk added breaking Implementing this issue could cause existing code to no longer compile or have different behavior. standard library This issue involves writing Zig code for the standard library. labels Mar 8, 2024
@mlugg
Copy link
Member Author

mlugg commented Mar 8, 2024

Okay, I'm going to go ahead with this one - the breakages are pretty trivial, and I have the Protty Stamp Of Approval (tm). I agree with the other proposed changes to the io_uring API, but they can be handled at a later date.

@mlugg mlugg merged commit 9cf28d1 into ziglang:master Mar 8, 2024
@mitchellh
Copy link
Contributor

Re: libxev. I'm sure it'll be fine 😄 I'll report back if I run into any issues upgrading to nightly.

@silversquirl
Copy link
Contributor

@Jarred-Sumner For mixing generated and handwritten code, you can make your generator use a file like this as a template:

pub const Foo = struct {
    // GENERATED:Foo

    pub fn foo(self: Foo) void {
        // ...
    }
};

pub const Bar = struct {
    // GENERATED:Bar

    pub fn bar(self: Bar) void {
        // ...
    }
};

Instead of outputting full structs, it simply replaces the // GENERATED: comments. That way you can write whatever code you like around the generated code, without needing to use usingnamespace

@expikr
Copy link
Contributor

expikr commented Mar 9, 2024

what's the planned new syntax replacing usingnamesapces?

@Jarred-Sumner
Copy link
Contributor

@Jarred-Sumner For mixing generated and handwritten code, you can make your generator use a file like this as a template. Instead of outputting full structs, it simply replaces the // GENERATED: comments. That way you can write whatever code you like around the generated code, without needing to use usingnamespace

While yes it is possible to do that, it's hard to see how that is better than usingnamespace. This would break editor autocomplete and it adds a layer of indirection (the file itself cannot be imported, instead all usages must import the generated file) which would need to be explained and re-explained each time someone imports the non-generated file

@perillo
Copy link
Contributor

perillo commented Mar 10, 2024

@mlugg Commit 17f83ac removed IndexedMap, IndexedArray, IndexedSet. What about moving the code to https://github.com/ziglang/std-lib-orphanage?

@matklad
Copy link
Contributor

matklad commented Mar 11, 2024

This would break editor autocomplete

As someone who has spend a fair amount of time building editor auto-complete for Rust, I am 0.85 sure that removal of usingnamespace, in the long run, would be a boon for autocomplete. Dealing with star imports, and, more generally, with any kind of unbounded identifier search is a major complexity and performance pain point for IDE tooling.

That being said, yes, removing usingnamespace would make code-generation use-case worse. The question is about the tradeoffs between convenience of source-code generation on one side of the scale, and a faster, simpler & more powerful compiler on the other one.

The source code generation problem doesn't seem insurmountable to me. I'd say that any of the following solutions would work okay, if not perfect, in practice:

  • generate code to foo_generated.zig and require importing generated files at the call-site.

  • make sure that the entire code is generated. So, you'll have

    ./foo.zig
    ./foo_helpers.zig
    

    Then, foo.zig is the generated file, the entirety of file is generated, but the generated code also contains const helpers = @import("foo_helpers.zig") and calls functions from helpers when appropriate. The hand-written code goes to the helpers.zig file, but that's a private implementation detail, the call-sites always just import foo. Just how you generate the entirety of foo might be different, but the template file silversquirl is one option (as long as all the actual hand-written logic lives in the helpers

  • manually delegate all symbols. So, there's foo.zig and foo_generated.zig, and the former has hand-written code like const my_func = @import("./foo_generated.zig").my_func. Writing delegation by hand is tiresome, but you'll only need to do this twice.

    more generally, unless the generated/hand-written split is roughly even, one of the previous two solutions (generate the entire API vs manual delegation) should work fine

  • if there's a bunch of generated code, but also a bunch of hand-written code, one can consider self-modifying code: https://matklad.github.io/2022/03/26/self-modifying-code.html#Self-Modifying-Code-1

    This is similar to the template approach, except that it looks more like

    pub const Foo = struct {
         // GENERATEDSTART:Foo
         pub fn my_generated_func(self: Foo) void { ... }
         // GENERATEDEND:Foo
    
         pub fn foo(self: Foo) void {
             // ...
         }
     };

    the code generator then just updates the file in-place

Again, these all are work-around, and usingnamespace solves this particular advanced use-case better, but it also imposes diffused costs on more-or-less every other use-case.

@Jarred-Sumner
Copy link
Contributor

As someone who has spend a fair amount of time building editor auto-complete for Rust, I am 0.85 sure that removal of usingnamespace, in the long run, would be a boon for autocomplete. Dealing with star imports, and, more generally, with any kind of unbounded identifier search is a major complexity and performance pain point for IDE tooling.

You certainly know more about editor autocomplete than I do and that makes sense.

I'm more concerned by the mixin-shaped hole removing usingnamespace would leave (interface boundaries). Rust-like Traits, Go-like interfaces, or TypeScript-like interfaces (semantic analysis validates, but compiles equivalently to anytype) would be an improvement over usingnamespace, but removing it without replacing it with something else would leave us worse off than the status quo

@matklad
Copy link
Contributor

matklad commented Mar 11, 2024

Yeah, "mixings" argument feels stronger to me than "source code generation" one! I don't have any particular insights here though!

@blblack
Copy link
Contributor

blblack commented Mar 18, 2024

@mlugg I tried to naively remove usingnamespace from my PR in #19203 and of course broke the smoke test I had included there. The general pattern I'm looking at here is: without usingnamespace, if you want to test a libc/sys -call interface that only exists on a subset of platforms, what's the new correct way to do that?

Before I was relying on the platform-conditional declaration via usingnamespace and then using @hasDecl in the test block to skip the test on non-supporting platforms.

I suppose one answer would be to move the list of supporting platforms to a conditional over the test code, but that doesn't quite feel right, either. If another new platform is added which happens to have this call, the plumbing would work thanks to the universal declaration, but the tests would be skipping this new platform and nobody may notice or care.

I also naively tried something like this:

pub const has_mlockall = switch (native_os) {
    .linux, .freebsd, .netbsd, .openbsd, .dragonfly, .solaris => true,
    else => false,
};                                     
        
pub const mlockall = if (has_mlockall) struct {
    extern fn mlockall(flags: c_int) c_int;
}.mlockall else undefined;

... but this still results in the declaration existing, it just happens to have an undefined value.

@blblack
Copy link
Contributor

blblack commented Mar 18, 2024

I guess the other reasonable alternative would be to stop trying to be so DRY in lib/std/c.zig with these platform conditionals and just put the redundant declarations in the various lib/std/c/foo.zig files (but then that itself currently still relies on a remaining use of usingnamespace to conditionally import them).

@blblack
Copy link
Contributor

blblack commented Mar 18, 2024

Hmm, how horrible would it be for the language to just have an explicit way to say that a block doesn't create a new scope, and thus is only useful for condition/looping/etc. For example:

// normal block, x is in a new sub-scope
if (foo == bar) {
    pub const x = undefined;
}

// scope-less block, serves the purposes of "if" conditional,
// but creates x in containing scope.
if (foo == bar) <{
   pub const x = undefined;
}>

@nektro
Copy link
Contributor

nektro commented Mar 18, 2024

that's precisely what usingnamespace does

@blblack
Copy link
Contributor

blblack commented Mar 18, 2024

that's precisely what usingnamespace does

Kinda, but not precisely. usingnamespace expects the operand to be a struct, union, or enum (or similarly, opaque type). What I'm suggesting above is just "this block of code is a block for flow-control purposes, but does not introduce a new sub-level of naming scope".

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking Implementing this issue could cause existing code to no longer compile or have different behavior. standard library This issue involves writing Zig code for the standard library.
Projects
None yet
Development

Successfully merging this pull request may close these issues.