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

Special-case unreachable If conditions in IRBuilder #7095

Closed
wants to merge 2 commits into from

Conversation

tlively
Copy link
Member

@tlively tlively commented Nov 20, 2024

When IRBuilder completes a scope that contains an unreachable
expression, it has to find the last unreachable expression and drop any
concretely typed expression that precedes it. If the original
unreachable expression has been made a child of some other expression,
that's usually fine because that other expression will become
unreachable as well. However, if conditions are special because they can
be unreachable without making their ifs unreachable, so we have to work
harder to find unreachable if conditions.

When IRBuilder completes a scope that contains an unreachable
expression, it has to find the last unreachable expression and drop any
concretely typed expression that precedes it. If the original
unreachable expression has been made a child of some other expression,
that's usually fine because that other expression will become
unreachable as well. However, if conditions are special because they can
be unreachable without making their ifs unreachable, so we have to work
harder to find unreachable if conditions.
@tlively tlively requested a review from kripken November 20, 2024 02:35
@tlively
Copy link
Member Author

tlively commented Nov 20, 2024

This is a simpler but arguably hackier alternative to #7094.

sawUnreachable = true;
break;
}
}
Copy link
Member

Choose a reason for hiding this comment

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

Would it be simpler to change this?

// if the arms return a value, leave it even if the condition

I think we could just make an if with an unreachable condition be unreachable, unless I'm missing a reason not to.

Copy link
Member

Choose a reason for hiding this comment

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

(that would avoid special-casing here, and seems simpler overall: the code is dead, so let's mark it dead)

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm on board with this and this is what #7094 does, but it's not as simple as we would like. I'm still chasing down fuzz bugs on that PR.

@tlively
Copy link
Member Author

tlively commented Nov 20, 2024

Closing this in favor of #7094.

@tlively tlively closed this Nov 20, 2024
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.

2 participants