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

make @import require a string literal rather than a comptime expression #2206

Closed
andrewrk opened this issue Apr 7, 2019 · 17 comments
Closed
Labels
accepted This proposal is planned. proposal This issue suggests modifications. If it also has the "accepted" label then it is planned.
Milestone

Comments

@andrewrk
Copy link
Member

andrewrk commented Apr 7, 2019

Zig's comptime evaluation is already powerful enough to allows conditional importing without having to have the @import target be a comptime expression. For example:

const target = if (builtin.mode == .Debug) "debug_mutex.zig" else "release_mutex.zig";
const mutex = @import(target);

This could be rewritten - and I would even say would be preferred to be rewritten - as:

const debug_mutex = @import("debug_mutex.zig");
const release_mutex = @import("release_mutex.zig");
const mutex = if (builtin.mode == .Debug) debug_mutex else release_mutex;

If import syntax is required to be trivially resolvable to either a file or a package name, then Zig can find all the source files for testing without this kind of thing:

comptime {
_ = @import("behavior/align.zig");
_ = @import("behavior/alignof.zig");
_ = @import("behavior/array.zig");
_ = @import("behavior/asm.zig");
_ = @import("behavior/atomics.zig");
_ = @import("behavior/bit_shifting.zig");
_ = @import("behavior/bitcast.zig");
_ = @import("behavior/bitreverse.zig");
_ = @import("behavior/bool.zig");
_ = @import("behavior/bswap.zig");
_ = @import("behavior/bugs/1025.zig");
_ = @import("behavior/bugs/1076.zig");
_ = @import("behavior/bugs/1111.zig");
_ = @import("behavior/bugs/1120.zig");
_ = @import("behavior/bugs/1277.zig");
_ = @import("behavior/bugs/1322.zig");
_ = @import("behavior/bugs/1381.zig");
_ = @import("behavior/bugs/1421.zig");

There is still a problem though, because if zig ran all tests indiscriminately, this would stop working:

zig/std/os/linux.zig

Lines 1537 to 1541 in 7c38651

test "import" {
if (builtin.os == builtin.Os.linux) {
_ = @import("linux/test.zig");
}
}

The general problem here is that @import has compile-time side effects:

  • changes the set of unit tests
  • causes additional symbols to be exported, with export or @export - potentially conflicting.
  • could cause @compileError to trigger based on build mode or something else in top level comptime blocks.
  • could cause a @cImport which causes a compile error

It would be weird to activate these side effects for tests but not for other builds. And these side effects are important; they're not going away.

So I think the benefits of this proposal are not clear. But I think this is a sort of important topic in Zig so it deserves an issue that we can reference.

@andrewrk andrewrk added the proposal This issue suggests modifications. If it also has the "accepted" label then it is planned. label Apr 7, 2019
@andrewrk andrewrk added this to the 0.5.0 milestone Apr 7, 2019
@andrewrk andrewrk modified the milestones: 0.5.0, 0.6.0 Sep 20, 2019
@andrewrk andrewrk added the accepted This proposal is planned. label Jan 7, 2020
@andrewrk andrewrk modified the milestones: 0.6.0, 0.7.0 Jan 7, 2020
@Vexu
Copy link
Member

Vexu commented May 22, 2020

One thing to consider here is that the path might be a compile time variable from somewhere like @import("build_options").

@andrewrk
Copy link
Member Author

One thing to consider here is that the path might be a compile time variable from somewhere like @import("build_options").

const tag = @import("build_options").which_package_do_you_want;
const thing = switch (tag) {
    .one => @import("one.zig"),
    .two => @import("two.zig"),
}

This pattern will work for the linked use case, and it will help with static code analysis tools. Is there another use case that would be problematic?

@minierolls
Copy link

How would you handle the case of when a user might want to specify a path to include? (e.g. through command line args or maybe a loaded "build configuration" file)

@Vexu
Copy link
Member

Vexu commented Sep 14, 2020

That could be better handled by having the build file declare a package pointing to that file.

@minierolls
Copy link

minierolls commented Sep 14, 2020

How would the build file declare a package if @import only allows for string literals? I might be missing something here.

Edit: Sorry, realized that this was under the assumption that addPackage used @import in its implementation, which I now realize is not the case(?)

@ikskuh
Copy link
Contributor

ikskuh commented Sep 14, 2020

@minierolls

zig build-exe my-source.zig --pkg-begin package_name /some/path/to/package.zig --pkg-end

will add a package called package_name pointing to /some/path/to/package.zig

@ikskuh
Copy link
Contributor

ikskuh commented Sep 15, 2020

As @andrewrk suggested, i want to propose the following improvement in addition to "only strings":

As @import is a built-in, it might allow both string literals (always user packages) and enum literals (.root, .self, .std, those are always magic) to be imported:

const network = @import("network");
const std = @import(.std);

This would make clear that these packages are magic and provided by the compiler whereas every string is to be given on the command line or in build.zig. It would also free those magic package names and make a clear distinction.

This change is even non-breaking, as it can be easily fixed with zig fmt

@Tetralux
Copy link
Contributor

Is "std" really magic? Is it not just a compiler-provided addPackagePath, or whatever it's called.

@ikskuh
Copy link
Contributor

ikskuh commented Sep 15, 2020

@Tetralux

Is "std" really magic? Is it not just a compiler-provided addPackagePath, or whatever it's called.

Both proposed "root" and "self" (see #6279) are special as they differ for each file in the project. The same is for "std" as it is not added by the user but is (afaik) always provided by the compiler (which makes them magic as i don't have control over that atm)

@andrewrk andrewrk modified the milestones: 0.7.0, 0.8.0 Oct 9, 2020
@nektro
Copy link
Contributor

nektro commented Jan 9, 2021

at the time of writing it appears this change of forcing an @import value to be a literal instead of a comptime_string removes the possibility of using dependencies in build.zig without hardcoding paths.

Take https://github.com/Snektron/vulkan-zig for example, using this with https://github.com/nektro/zigmod (as opposed to git submodules or something similar) requires the following code: (deps.zig is generated by zigmod before zig build is ran, so all the values inside are comptime known.)

const deps = @import("./deps.zig");
const vkgen = @import(deps.pkgs.vkgen.path);

@marler8997
Copy link
Contributor

marler8997 commented Feb 25, 2021

at the time of writing it appears this change of forcing an @import value to be a literal instead of a comptime_string removes the possibility of using dependencies in build.zig without hardcoding paths.

@nektro I believe this proposal should solve that: #8070

@tau-dev
Copy link
Contributor

tau-dev commented Apr 3, 2021

[snip...] this would stop working:

zig/std/os/linux.zig

Lines 1537 to 1541 in 7c38651

test "import" {
if (builtin.os == builtin.Os.linux) {
_ = @import("linux/test.zig");
}
}

That could be solved if imports were resolved lazily − I do not know how practical that would be to implement.

Implementing this proposal would enable external tools such as ZLS to correctly determine (potential) dependencies.

@nektro
Copy link
Contributor

nektro commented May 25, 2021

As suggested, i want to propose the following improvement in addition to "only strings":

As @import is a built-in, it might allow both string literals (always user packages) and enum literals (.root, .self, .std, those are always magic) to be imported:

const network = @import("network");
const std = @import(.std);

This would make clear that these packages are magic and provided by the compiler whereas every string is to be given on the command line or in build.zig. It would also free those magic package names and make a clear distinction.

is this a separate proposal for this change specifically?

@frmdstryr
Copy link
Contributor

Is there another use case that would be problematic?

This doesn't scale very well... At ~20 imports and 5 boards so that grows quickly..

pub const McuFamily = enum {
    stm32h7,
    //etc...
}
pub const system = @import("bsp/" ++ @tagName(family) ++  "/system.zig");

elerch added a commit to elerch/aws-sdk-for-zig that referenced this issue Jun 30, 2021
This method was in violation of the accepted proposal
ziglang/zig#2206
and disallowed in the compiler as of adc2aed,
now in 0.8.0.
@andrewrk andrewrk closed this as completed Jul 5, 2021
@hronro
Copy link

hronro commented Feb 11, 2022

In my case, I have a lot of zig files which all expose a function with same name, but the implementation of that function is different. For example:

// a.zig
pub fn doSomething() bool {
    // do something
}

// b.zig
pub fn doSomething() bool {
    // do something differently from a.zig
}

Now I want to call the doSomething() by the filename:

pub fn callByName(name: []const u8) !bool {
	const names = [_][]const u8 { "a", "b", "c" };  // this should actually generated automatically by scanning the file system in `build.zig`.

	inline for (names) |n| {
		const imported = @import("./" ++ n ++ ".zig");
		if (@import("std").mem.eql(u8, name, n)) {
			return imported.doSomething();
		}
	}

	return error.NotFound;
}

If I call callByName("a"), I actually called doSomething() in a.zig; I call callByName("b"), I actually called doSomething() in b.zig.

However, this can't be done now since @import now requires a string literal rather than a comptime expression.

@marler8997
Copy link
Contributor

@hronro how about this?

const Import = struct {
    name: []const u8,
    import: type,
};

pub fn callByName(name: []const u8) !bool {
    const imports = [_]Import {
        .{ .name = "a", .import = @import("a.zig") },
        .{ .name = "b", .import = @import("b.zig") },
    };
    inline for (imports) |imported| {
        if (@import("std").mem.eql(u8, name, imported.name)) {
            return imported.import.doSomething();
        }
    }

    return error.NotFound;
}

@inkeliz
Copy link

inkeliz commented Jan 6, 2024

@marler8997 but how can you do it without knowing the files? I mean, considering the @hronro comments "// this should actually generated automatically by scanning the file system in build.zig.".

I'm quite new to Zig, so I tried to create a custom package in "build.zig":

    var backend_build_meta_files = std.ArrayList([]const u8).init(std.heap.page_allocator);
    var dir_backend_path = "src/backend/";
    var dir_backend = try std.fs.cwd().openIterableDir(dir_backend_path, .{ .access_sub_paths = true });
    var it_backend = dir_backend.iterate();
    while (try it_backend.next()) |file| {
        
        // ... to simplify the code

        try backend_build_meta_files.append(name);
    }
    
    const backend_build_meta = b.addOptions();
    backend_build_meta.addOption([][]const u8, "types", backend_build_meta_files.items);

Then, I tried use @import:

pub const Types = KnownTypes();

fn KnownTypes() type {
    const backend_meta = @import("backend_build_meta");

    comptime var fields: [backend_meta.types.len]std.builtin.Type.StructField = undefined;
    comptime var decls = [_]std.builtin.Type.Declaration{};

    for (0..backend_meta.len, backend_meta.types) |i, f| {
        fields[i] = .{
            .name = f,
            .type = type,
            .alignment = @alignOf(type),
            .is_comptime = false,
            .default_value = @import(f),
        };
    }

    return @Type(std.builtin.Type{ .Struct = .{
        .layout = .Auto,
        .fields = &fields,
        .decls = &decls,
        .is_tuple = false,
    } });
}

The idea was to create a struct which each fields would have @import(f).SomeFunction, similar to @hronro case. However, I don't know how to do it due to "@import operand must be a string literal".

EDIT: The only easy alternative is to create a file in build.zig, such as:

    var backend_types_gen = try std.fs.cwd().createFile("src/backend/BackendTypes.zig", .{});
    try backend_types_gen.writeAll("pub const Types = struct{");

    var dir_backend_path = "src/backend/";
    var dir_backend = try std.fs.cwd().openIterableDir(dir_backend_path, .{ .access_sub_paths = true });
    var it_backend = dir_backend.iterate();
    while (try it_backend.next()) |file| {
        
        // ...

        try std.fmt.format(backend_types_gen.writer(),"\n\t{s}: type = @import(\"{s}{s}\"),", .{
            file.name[0..file.name.len-4], 
            dir_backend_path, 
            file.name,
        });
    }
    try backend_types_gen.writeAll("\n};");

Now, we can import "BackendTypes.zig" that is created by build.zig. However, that sounds wrong for Zig.

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