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

introduce @hasField builtin function #1439

Closed
shawnl opened this issue Aug 30, 2018 · 25 comments · Fixed by #2438
Closed

introduce @hasField builtin function #1439

shawnl opened this issue Aug 30, 2018 · 25 comments · Fixed by #2438
Labels
proposal This issue suggests modifications. If it also has the "accepted" label then it is planned.
Milestone

Comments

@shawnl
Copy link
Contributor

shawnl commented Aug 30, 2018

Accepted Proposal


Using a undefined symbol is a compiler error:

/home/shawn/git/zig/std/os/linux/index.zig:840:21: error: use of undeclared identifier 'SYS_mmap2'
    return syscall6(SYS_mmap2, @ptrToInt(address), length, prot, flags, @intCast(usize, fd), @bitCast(usize, offset / 4096));

It is possible to work around this by using buildin.arch and listing which arches have mmap2 (the 32-bit ones)

    if (builtin.arch == builtin.Arch.armv7) {
        return syscall6(SYS_mmap2, @ptrToInt(address), length, prot, flags, @intCast(usize, fd), @bitCast(usize, offset / 4096));

But instead of making all these assumption with code like that it would be cleaner to be able to comptime branch of a identifier existing:

    if (@defined(SYS_mmap2)) {
        return syscall6(SYS_mmap2, @ptrToInt(address), length, prot, flags, @intCast(usize, fd), @bitCast(usize, offset / 4096));
    } else {
        return syscall6(SYS_mmap, @ptrToInt(address), length, prot, flags, @intCast(usize, fd), @bitCast(usize, offset));
    }
@0joshuaolson1
Copy link

Which folders should be searched? In what order? If found in more than one place, shouldn't that information be returned?

@thejoshwolfe
Copy link
Contributor

Sorry if this is hijacking the issue thread, but on the topic of feature detection, are we talking about runtime or comptime variables? Like a feature of a filesystem or a feature of the standard library?

Feature detection may be possible with status quo zig.

@shawnl
Copy link
Contributor Author

shawnl commented Aug 30, 2018

Which folders should be searched?

the current scope

runtime or comptime variables

probably both, just if the symbol is defined, so even with a runtime variable it would be a comptime if it exists.

Feature detection may be possible with status quo zig.

Yes it is, but there are times where the feature could be expressed better in a symbol existing.

Here is a use-case:
kristate@abcd461

would be better expressed as if (@defined(linux.SYS_mmap2) { }

@shawnl
Copy link
Contributor Author

shawnl commented Aug 30, 2018

If found in more than one place, shouldn't that information be returned?

it is a compile error to define the same symbol twice

@0joshuaolson1
Copy link

if the symbol is defined, so even with a runtime variable it would be a comptime if it exists

The target may be a different machine. Also - random idea - could environment variables be symbols? This would be relevant to the build system.

@shawnl
Copy link
Contributor Author

shawnl commented Aug 30, 2018

Also - random idea - could environment variables be symbols?

environment variables are horribly wasteful of memory and are generally a bad idea.

@thejoshwolfe
Copy link
Contributor

@shawnl this issue as it is stated in the title and original post is a proposal for a language feature. We need a lot more information for this to have a chance at being accepted. How does it work? What does the status quo workaround look like? How is adding a language feature justified in this situation? Remember that zig wants to stay simple in general unless the complexity is worth it.

Feature detection can be a general discussion, not necessarily related to the proposed language feature. It seems like discussing how feature detection should work is an easier topic to start with. Is that what this is all about? If so, let's focus on that.

Thanks for the link to the commit, but i didn't quite understand the point. That code is years old with workarounds for bugs that were fixed years ago. Can you explain that use case in more detail?

@tgschultz
Copy link
Contributor

I believe the example use case would be solved if #1047 is implemented. Then such a feature could easily be implemented in userland by iterating over the typeInfo of the 'namespace'.

@shawnl
Copy link
Contributor Author

shawnl commented Aug 30, 2018

@thejoshwolfe Issue updated. I think it is now clear.

@tgschultz Indeed that would fix this use-case and in a more general manner.

@thejoshwolfe
Copy link
Contributor

Ah thanks. I see now that these symbols are being defined by a use statement:

pub use switch (builtin.arch) {
    builtin.Arch.x86_64 => @import("x86_64.zig"),
    builtin.Arch.i386 => @import("i386.zig"),
    else => @compileError("unsupported arch"),
};

I've always hated use. It obfuscates where things are coming from and enables unique situations like this, where an unqualified identifier may or may not be defined depending on comptime variables.

Here's a different workaround for this specific issue:

const arch_stuff = switch (builtin.arch) {
    builtin.Arch.x86_64 => @import("x86_64.zig"),
    builtin.Arch.i386 => @import("i386.zig"),
    else => @compileError("unsupported arch"),
};
pub use arch_stuff;
...
if (isDefined(arch_stuff, "SYS_mmap2")) {
    ...
}

And now isDefined() can be implemented in userspace using @typeInfo().

@shawnl
Copy link
Contributor Author

shawnl commented Aug 30, 2018

so as i understand it this is blocked on #1047
although I hope we can get a more performant way of iterating a struct than @memberName() and @membercount()

So for that reason we still need a way to search a struct by identifier.

it appears they are stored in a hash map
HashMap<Buf *, TypeStructField *, buf_hash, buf_eql_buf> fields_by_name;

@thejoshwolfe
Copy link
Contributor

Yeah, i think some kind of get-by-name api in TypeInfo makes sense.

@shawnl shawnl changed the title @defined() intrinsic get-by-name api in TypeInfo Aug 31, 2018
@andrewrk andrewrk added this to the 0.4.0 milestone Sep 2, 2018
@andrewrk andrewrk added the proposal This issue suggests modifications. If it also has the "accepted" label then it is planned. label Sep 2, 2018
@andrewrk andrewrk modified the milestones: 0.4.0, 0.5.0 Nov 21, 2018
@shawnl
Copy link
Contributor Author

shawnl commented Apr 5, 2019

On IRC andrew said this could be @hasField, but I think it would be better as @getField

@getField(comptime T: type, member_name: []u8) ?field

@andrewrk
Copy link
Member

andrewrk commented Apr 5, 2019

You could get the type with @typeOf(@field(a, b)) but if @getField always returned a ?type then it would force evaluation of the type. So if the type itself did something which depended on whether or not the declaration existed, that would be a false positive dependency loop. This would be solved with #2174, but that hasn't been accepted or implemented yet. Also the way you get the type normally of a top level declaration is @typeOf so it's a bit weird to have this other way of getting it.

@shawnl
Copy link
Contributor Author

shawnl commented Apr 5, 2019

my bad. I actually meant it returns the field. I just don't know what the type of the field is so I put type there. I think actually returning it is better than a bool.

@andrewrk
Copy link
Member

andrewrk commented Apr 5, 2019

How about @fieldOrNull? I want it to be crystal clear at the callsite that it could potentially be a double optional.

Also, at this point, why have @field? Because you could do @fieldOrNull(a, b).?.

@andrewrk
Copy link
Member

andrewrk commented Apr 5, 2019

Hmm. I'm going to stick with my original idea for now. @field is special; it really does field access but with a comptime []const u8. Trying to tack a ? on the result is a semantically weird and unique thing that this one function would be doing. It makes the language bigger. On the other hand, @hasField is incredibly simple and easy to understand. It's @field, but instead of a compile error for field not found you get false, and instead of a value for a field you get true.

Let's run with this and see what the code ends up looking like, and then we can re-evaluate.

So to make it clear, what is accepted is @hasField.

@andrewrk andrewrk added the accepted This proposal is planned. label Apr 5, 2019
@andrewrk andrewrk changed the title get-by-name api in TypeInfo introduce @hasField builtin function Apr 5, 2019
shawnl added a commit to shawnl/zig that referenced this issue May 6, 2019
This was quite straight-forward

Closes: ziglang#1439
@shawnl
Copy link
Contributor Author

shawnl commented May 6, 2019

Hmm, I realized this doesn't cover it. There also needs to be a way to examine whether something is defined in a container---like a constant or a function. This has to be a differn't builtin because it is a totally differn't thing.

@Hejsil
Copy link
Contributor

Hejsil commented May 7, 2019

@shawnl @hasField should be able to examine definitions, just like @field is able to.

const S = struct {
    a: u8,
    const A = 0;
};

test "" {
    var s: S = undefined;
    @import("std").testing.expect(@hasField(s, "a"));
    @import("std").testing.expect(@hasField(S, "A"));
}

@tgschultz
Copy link
Contributor

I think the terminology needs some consideration. "Field" is overloaded here and refers to both variables within a namespace and struct instance field.

const S = struct {
    a: u8,
    pub const a = 0;
};
const s = S{.a = 1};
testing.expect(@field(S, a) == 1); //typo => compiles but test fails

which is less obvious, I think, than if there were a separate builtin:

const S = struct {
    a: u8,
    pub const a = 0;
};
const s = S{.a = 1};
testing.expect(@field(S, a) == 1); //typo => compile error, type `type` has no field "a".
testing.expect(@definition(S, a) == 1); //compiles => true.

Consider also the following: in order to determine if an instance of a struct would have a certain member, you must initialize an instance: @hasField(S(undefined), "a"); and to determine if the type of a struct instance has a definition: @hasField(@typeOf(s), "a");

However, one can imagine there might be usecases where one would want to substitute an empty struct in place of a struct instance to a comptime function and have the same code work.

Personally, I think it would be better to have separate builtins for each case, but failing that I propose the following nomenclature to avoid confusion:

"member" => either a struct instance variable or a namespaced variable.
"field" => always a struct instance field.
"definition" => always a namespaced variable.

which would mean that @hasField and @field would become @hasMember and @member.

@andrewrk andrewrk removed the accepted This proposal is planned. label May 7, 2019
@shawnl
Copy link
Contributor Author

shawnl commented May 7, 2019

@tgschultz yes, that is basically the conclusion I came to (that the current @field() intrinsic has terminology that contradicts this proposal), but I realized I had to retract my working pull request, which implements the same @hasField() you specify, because this needed to be discussed first.

If @andrewrk accepts your terminology than my patch is still good to go, and I can work on the rest.

@andrewrk
Copy link
Member

I needed this (at least @hasDecl) for #2380 so I went ahead and did it. @tgschultz's terminology is good. I'm not convinced we need @hasMember but I would accept a PR for @hasField (which I believe is the one @shawnl has ready)

@shawnl
Copy link
Contributor Author

shawnl commented May 26, 2019

@hasMember can be done by just doing @hasField or @hasDecl.

@tgschultz
Copy link
Contributor

I agree that @hasMember is probably unnecessary, however either @field should be split into @decl and @field or be reneamed @member.

@andrewrk
Copy link
Member

either @field should be split into @decl and @field or be reneamed @member.

I agree with this.

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

Successfully merging a pull request may close this issue.

6 participants