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

Astgen: more unreachable checks #9645

Closed
wants to merge 5 commits into from

Conversation

nektro
Copy link
Contributor

@nektro nektro commented Aug 29, 2021

Also fixes #9630

Copy link
Member

@ifreund ifreund left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why have only some uses of expr() been replaced with reachableExpr()? It seems to me that every nearly every usage of expr() needs to check if the expr() is noreturn. I wonder if renaming expr() to exprMaybeNoReturn() and reachableExpr() to expr() would be the best way forward.

Note that the current reachableExpr() also should be updated to use GenZir.endsWithNoReturn().

src/AstGen.zig Outdated Show resolved Hide resolved
src/AstGen.zig Outdated Show resolved Hide resolved
@ifreund
Copy link
Member

ifreund commented Aug 29, 2021

On further investigation, I think the only place where we wouldn't be able to use the semantics of the current reachableExpr() would be in unusedResultExpr().

@ifreund
Copy link
Member

ifreund commented Aug 29, 2021

Though now I've remembered about #3257 and wonder if we're on the totally wrong path here...

@nektro nektro force-pushed the astgen-more-unreachable-checks branch from b0f73a6 to 1a1624e Compare August 30, 2021 10:53
@ifreund
Copy link
Member

ifreund commented Sep 17, 2021

Note: if we apply these checks wherever technically possible this would make the following pattern fail to compile:

pub const ensureCapacity = @compileError("deprecated for ensureUnusedCapacity and ...");

however, this could be worked around by wrapping the @compileError() call in a function or block:

pub const ensureCapacity = {
    @compileError("deprecated...");
};
// or
pub fn ensureCapacity(self: *Self, new_capacity: usize) !void {
    @compileError("deprecated...");
}

Applying these checks wherever possible would certainly be the most consistent approach IMO, and I don't find the workarounds too unpalatable. This feels quite arbitrary though...

const result_inst = try expr(gz, scope, rl, node);
if (gz.refIsNoReturn(result_inst)) {
return gz.astgen.failNodeNotes(src_node, "unreachable code", .{}, &[_]u32{
try gz.astgen.errNoteNode(node, "control flow is diverted here", .{}),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what happened to this nice error note?

@andrewrk
Copy link
Member

If someone wants to pick this back up:

  • use the reachableExpr function that I have provided, for example in c42763f
  • add test coverage for each one. yes, each one. again, example is in c42763f

@andrewrk andrewrk closed this Nov 25, 2021
@nektro nektro deleted the astgen-more-unreachable-checks branch November 25, 2021 03:07
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.

Return statements allow an arbitrary number of return keywords
3 participants