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

stage2: fix extern fn decl parsing and astgen #10040

Merged
merged 2 commits into from Oct 27, 2021

Conversation

mattbork
Copy link
Contributor

AstGen's fnDecl skips the call to detectLocalShadowing for parameter names of external functions. However, LocalVal scopes are still added for each parameter, so shadowing can change the type of later parameters. For example, in

const X = u32;
extern fn f(X: *X, b: *X) void;

the emitted param block looks like

%7 = param("X", {
  %4 = decl_val("X") token_offset:2:17
  %5 = ptr_type_simple(%4, One)
  %6 = break_inline(%7, %5)
}) token_offset:2:13
%10 = param("b", {
  %8 = ptr_type_simple(%7, One)
  %9 = break_inline(%10, %8)
}

which makes the type of b invalid.

These changes preserve the permissive shadowing by not adding any extern function parameter names to the current scope. This makes a test for extern functions with bodies in compile_errors.zig fail because parse.zig does not check for this like parser.cpp does, so I added that as well.

Note that this change prevents any reference to earlier parameters in an extern function, so extern fn f(a: u32, b: @TypeOf(a)) void becomes invalid. I think this is probably the right choice when shadowing is allowed, but one alternative would be to add just the parameter names that don't shadow anything to the current scope. Or perhaps the best choice is to simply apply the usual rules for shadowing to extern functions as well. Both are easy changes.

@andrewrk
Copy link
Member

Thanks!

I think this is probably the right choice when shadowing is allowed

I agree with your conclusion.

@andrewrk andrewrk merged commit 9024f27 into ziglang:master Oct 27, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants