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

missing constCast in c fn call cause compiler error without any tips and notes #19781

Closed
oxfrancis opened this issue Apr 28, 2024 · 11 comments · Fixed by #19835
Closed

missing constCast in c fn call cause compiler error without any tips and notes #19781

oxfrancis opened this issue Apr 28, 2024 · 11 comments · Fixed by #19835
Labels
bug Observed behavior contradicts documented or intended behavior

Comments

@oxfrancis
Copy link

oxfrancis commented Apr 28, 2024

Zig Version

0.12.0

Steps to Reproduce and Observed Behavior

after the recent feature in 0.12, never mutated local variable will cause a compiler error "local variable is never mutated", with note "consider using const".
but if it happens in a c function call where missing a constCast, the compiler gives no note, just fails, make it hard to locate the causing.

Expected Behavior

just gives some notes on error

@oxfrancis oxfrancis added the bug Observed behavior contradicts documented or intended behavior label Apr 28, 2024
@silversquirl
Copy link
Contributor

Could you provide a code sample that reproduces this issue?

@oxfrancis
Copy link
Author

Could you provide a code sample that reproduces this issue?

Could you provide a code sample that reproduces this issue?

c.XSetIMValues(null, c.XNDestroyCallBack, &c.XIMCallback{.client_data=null, .callback=null}, c.NULL)
in 0.11, this will be ok, but it will cause the compiler fails in 0.12.0 without any notes.

the workaround is:
c.XSetIMValues(null, c.XNDestroyCallBack, @constcast(&c.XIMCallback{.client_data=null, .callback=null}), c.NULL)

@ehaas
Copy link
Sponsor Contributor

ehaas commented Apr 28, 2024

There's still not enough context here to help diagnose what is going on - do you have a link to a repo we could look at? At first glance it looks like you're taking the address of a temporary variable (via &c.XIMCallback{.client_data=null, .callback=null}) which has produced a const pointer since 0.10.0 - so using @constCast is the wrong solution - you should either update the prototype to accept a const pointer to XIMCallback, or you should create var callback: c. XIMCallback = .{ .client_data = null, .callback = null } and pass &callback to c.XSetIMValues

@unisgn
Copy link

unisgn commented May 1, 2024

@ehaas @silversquirl here is the problem:

_ = std.c.printf("format", .{"abc"});
or
_ = std.c.printf("format", &.{"abc"});
this simple code will make compiler fails without any hints.

_ = std.c.printf("format", @constcast(&.{"abc"})); // this is ok

@Rexicon226
Copy link
Contributor

Rexicon226 commented May 1, 2024

Stack Trace
/home/dr/zig/lib/std/debug.zig:403:14: 0x8004b53 in assert ()
    if (!ok) unreachable; // assertion failure
             ^
/home/dr/zig/src/Sema.zig:1904:11: 0x8ed6e2f in resolveType ()
    assert(air_inst != .var_args_param_type);
          ^
/home/dr/zig/src/Sema.zig:4648:32: 0x94b33e9 in zirValidateArrayInitTy ()
    const ty = sema.resolveType(block, ty_src, extra.ty) catch |err| switch (err) {
                               ^
/home/dr/zig/src/Sema.zig:1377:48: 0x8dfaf1c in analyzeBodyInner ()
                try sema.zirValidateArrayInitTy(block, inst, true);
                                               ^
/home/dr/zig/src/Sema.zig:910:30: 0x891bded in analyzeInlineBody ()
    if (sema.analyzeBodyInner(block, body)) |_| {
                             ^
/home/dr/zig/src/Sema.zig:936:39: 0x8510abd in resolveInlineBody ()
    return (try sema.analyzeInlineBody(block, body, break_target)) orelse .unreachable_value;
                                      ^
/home/dr/zig/src/Sema.zig:7414:65: 0x9e52319 in analyzeArg ()
                const uncoerced_arg = try sema.resolveInlineBody(block, arg_body, zir_call.call_inst);
                                                                ^
/home/dr/zig/src/Sema.zig:7977:49: 0x96c351d in analyzeCall ()
            arg_out.* = try args_info.analyzeArg(sema, block, arg_idx, param_ty, func_ty_info, func);
                                                ^
/home/dr/zig/src/Sema.zig:7125:43: 0x9387c04 in zirCall__anon_89488 ()
    const call_inst = try sema.analyzeCall(block, func, func_ty, callee_src, call_src, modifier, ensure_result_used, args_info, call_dbg_node, .call);
                                          ^
/home/dr/zig/src/Sema.zig:1015:62: 0x8dee6e2 in analyzeBodyInner ()
            .field_call                   => try sema.zirCall(block, inst, .field),

My personal opinion is that array init without an explicit type should be illegal in varargs.

@unisgn
Copy link

unisgn commented May 1, 2024

@ehaas @silversquirl here is the problem:

_ = std.c.printf("format", .{"abc"}); or _ = std.c.printf("format", &.{"abc"}); this simple code will make compiler fails without any hints.

_ = std.c.printf("format", @constcast(&.{"abc"})); // this is ok

the problem seems to be that you can't use a temporary variable in a va_list, unless it's a @constcast modified pointer

@mlugg
Copy link
Member

mlugg commented May 1, 2024

The examples above trigger an assertion in handling validate_array_init_[ref_]ty due to missing handling for var_args_param_type. That should be simple to fix. That said: @andrewrk, is there any reason var_args_param_type exists at all as opposed to simply using generic_poison_type? I suppose it could help us emit errors for type-unsafe behavior on API boundaries, such as this exact one.

@unisgn, the issue is not directly related to @constCast -- this code is invalid. You can't pass a Zig tuple to a C function and expect it to work. C varargs are, well, varargs -- use printf as you would in C.

@unisgn
Copy link

unisgn commented May 1, 2024

The examples above trigger an assertion in handling validate_array_init_[ref_]ty due to missing handling for var_args_param_type. That should be simple to fix. That said: @andrewrk, is there any reason var_args_param_type exists at all as opposed to simply using generic_poison_type? I suppose it could help us emit errors for type-unsafe behavior on API boundaries, such as this exact one.

@unisgn, the issue is not directly related to @constCast -- this code is invalid. You can't pass a Zig tuple to a C function and expect it to work. C varargs are, well, varargs -- use printf as you would in C.

let me show some more case.

const S = struct {a: u8=0};
var var_s = S{};
const const_s = S{};

_ = std.c.printf("",

var_s, // fail#1 with some tips
S{}, // also fail#2 with some tips

&var_s, // ok#1
&const_s, // ok#2

&S{}, // fail#3 without any tips
@constcast(&S{}), // ok#3

.{"str"}, // fail#4 without any tips
&.{"str"}, // fail#5 without any tips
@constcast(&.{"str"}), // ok#4
);

here is my question:
1, why fail#4 gives no tips like fail#1 and fail#2
2, since ok#2 is ok, why fail#3 fails, and give no tips.
3, since fail#3 fails, why ok#3 and ok#4 is ok

note
1, think tuple as an anonymous struct
2. the printf is chosen as a convenient fn that takes a varargs to test.

@mlugg
Copy link
Member

mlugg commented May 2, 2024

  1. bug (the one I described above)
  2. bug (cc anyone working on this: coerce_ptr_elem_ty also needs a fix)
  3. it's a bug that fail#3 fails

Varargs aren't as tested as they should be! I might quickly put a PR up to fix this in a bit.

mlugg added a commit to mlugg/zig that referenced this issue May 2, 2024
This was a "fake" type used to handle C varargs parameters, much like
generic poison. In fact, it is treated identically to generic poison in
all cases other than one (the final coercion of a call argument), which
is trivially special-cased. Thus, it makes sense to remove this special
tag and instead use `generic_poison_type` in its place. This fixes
several bugs in Sema related to missing handling of this tag.

Resolves: ziglang#19781
@oxfrancis
Copy link
Author

oxfrancis commented May 2, 2024 via email

@mlugg
Copy link
Member

mlugg commented May 2, 2024

Sorry, I only write "cc" to indicate a note in my comment to another party (in this case to whoever ends up working on the fix). It's not relevant to you, and since I've fixed the issue in #19835 (currently failing CI but it is a fix), it didn't end up being relevant to anyone else either!

mlugg added a commit to mlugg/zig that referenced this issue May 4, 2024
This was a "fake" type used to handle C varargs parameters, much like
generic poison. In fact, it is treated identically to generic poison in
all cases other than one (the final coercion of a call argument), which
is trivially special-cased. Thus, it makes sense to remove this special
tag and instead use `generic_poison_type` in its place. This fixes
several bugs in Sema related to missing handling of this tag.

Resolves: ziglang#19781
mlugg added a commit to mlugg/zig that referenced this issue May 4, 2024
This was a "fake" type used to handle C varargs parameters, much like
generic poison. In fact, it is treated identically to generic poison in
all cases other than one (the final coercion of a call argument), which
is trivially special-cased. Thus, it makes sense to remove this special
tag and instead use `generic_poison_type` in its place. This fixes
several bugs in Sema related to missing handling of this tag.

Resolves: ziglang#19781
mlugg added a commit that referenced this issue May 4, 2024
This was a "fake" type used to handle C varargs parameters, much like
generic poison. In fact, it is treated identically to generic poison in
all cases other than one (the final coercion of a call argument), which
is trivially special-cased. Thus, it makes sense to remove this special
tag and instead use `generic_poison_type` in its place. This fixes
several bugs in Sema related to missing handling of this tag.

Resolves: #19781
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Observed behavior contradicts documented or intended behavior
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants