-
-
Notifications
You must be signed in to change notification settings - Fork 2.8k
Compiler: allow functions prototypes to have their own scopes #18044
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
Conversation
}; | ||
|
||
if (fn_proto.ast.addrspace_expr != 0) { | ||
return astgen.failNode(fn_proto.ast.addrspace_expr, "addrspace not allowed on function prototypes", .{}); | ||
} | ||
var addrspace_gz = block_scope.makeSubBlock(params_scope); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unused.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unused.
This is true but without it the compiler crashes with 'use of null' at line 11328 in AstGen.zig
in addFunc
. There is following doc comment in above this fuction
/// Must be called with the following stack set up:
/// * gz (bottom)
/// * align_gz
/// * addrspace_gz
/// * section_gz
/// * cc_gz
/// * ret_gz
/// * body_gz (top)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Huh, it's strange how it properly checks each of them individually in the else branch but only ret_gz
in the then branch.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should I rewrite this check in addFn or let it be as it is?
|
||
if (fn_proto.ast.section_expr != 0) { | ||
return astgen.failNode(fn_proto.ast.section_expr, "linksection not allowed on function prototypes", .{}); | ||
} | ||
var section_gz = block_scope.makeSubBlock(params_scope); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also unused.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also unused.
Same here
@@ -10926,6 +10976,7 @@ const Scope = struct { | |||
/// The category of identifier. These tag names are user-visible in compile errors. | |||
const IdCat = enum { | |||
@"function parameter", | |||
@"function prototype parameter", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think there is any benefit to separating this from @"function parameter"
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think there is any benefit to separating this from
@"function parameter"
.
I think with such wording compiler diagnostics are more accurate. But I can delete it if necessary
As mentioned on Discord, I do not think this is a desirable change. Allowing references to prior parameters when writing function types implies stronger guarantees on generic function types than actually exist. For instance, under this change, the following equality holds: fn (x: anytype) @TypeOf(x) == fn (x: anytype) ?@TypeOf(x) Zig does not have a good way to represent specific generic function types at the type level; unless this changes. allowing you to write these types and then "demoting" them to the representable generic type (in this case printed as |
This behavior already in master. And users can face to the very same issue using fn foo(x: anytype) @TypeOf(x) {
return x;
}
fn bar(x: anytype) ?@TypeOf(x) {
return null;
}
pub fn main() void {
@compileLog(@TypeOf(foo) == @TypeOf(bar));
}
|
No, this patch takes a behavior from one part of the language, and applies it to a different part of the language. Consistency between these syntax forms is not itself a fundamental end goal here: to make this change, you must justify why allowing backreferences is useful/important despite these equivalences. Justifying this is the job of a language proposal. In function declarations, we have to allow backreferences, because this is fundamental to Zig's system of generics. The way the type system interacts with generics there is certainly unfortunate, but it does not call for us to eliminate backreferences entirely, since the type of a generic function is only a small detail of it (it's actually fairly rare to interact much, if at all, with generic function types in Zig code). On the other hand, in function types, there is zero practical use for these backreferences in a way which is not explicitly misleading - writing generic function types, where (since the whole thing you're doing is writing a type) the syntax strongly implies that the type is encoding the constraints you are writing when, in reality, it is not. I believe this change should be a language proposal (and it is a proposal I would personally oppose). |
I've converted #15409 into a language proposal; we can continue the discussion there. This code is still available to be resurrected depending on the outcome. |
Closes #15409
This patch adds scopes to functions prototypes. It may be useful for describing dependence of one parameter from another one. For instance:
With this capability compiler must be able to detect shadowing from current and enclosing scopes. However, when generics are involved in cases like the following:
compiler cannot infer second type and treats it as anytype. But the very same behavior appears in the latest compiler dev version with generic functions (not prototypes). I'm pretty sure that this doesn't apply to the original issue
Finally, it turned out that there was several local shadowings in the compiler_rt and std source code. I fixed it as well.