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

Handle block scopes in compile-time binding check #4379

Merged
merged 2 commits into from
Oct 7, 2024

Conversation

geoffromer
Copy link
Contributor

@geoffromer geoffromer commented Oct 7, 2024

This fixes a fuzzer-found crash when a compile-time binding occurs inside a block (which is represented by an invalid inst ID).

We now allow `let` bindings in all scopes except `class` and `impl`, which are
known to be problematic. This also switches to GetCurrentScopeAs instead of
PeekInstId, to better encapsulate handling of invalid scope inst IDs.
This fixes a fuzzer-found crash when a compile-time binding occurs inside a
block (which is represented by an invalid inst ID).

This partially reverts carbon-language#4338, but is consistent with @jonmeow's comments on
that PR.
@geoffromer geoffromer requested a review from zygoloid October 7, 2024 19:49
@github-actions github-actions bot requested a review from josh11b October 7, 2024 19:49
Copy link
Contributor

@zygoloid zygoloid left a comment

Choose a reason for hiding this comment

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

It's not captured in the review for #4338, but we intentionally chose to use a small list of allowed contexts here because other contexts are either not known to work yet or known to not work yet.

For example, given

let T:! type = {};
fn F() {}
fn G() { F(); }

... we produce a bogus error on the call to F() saying we can't deduce a value for T (because we expect all generic parameters in scope in the declaration of F to have deduced arguments), and

fn F(U:! type);
let T:! type = {};
fn F(U:! type) {}
fn G() { F({}); }

... crashes due to a mismatch between the two declarations of F.

So while I think this change gets us closer to matching the design, I don't think the rest of the toolchain is ready for us to go this far yet. Maybe for now we can check for and reject the case where PeekInstId returns InstId::Invalid? Or perhaps we can also reject let bindings in a namespace scope?

@geoffromer geoffromer changed the title Narrower check for disallowed compile time bindings. Handle block scopes in compile-time binding check Oct 7, 2024
@geoffromer
Copy link
Contributor Author

Let's try accepting the case where the inst ID is invalid. That seems brittle, but hopefully it can tide us over until we can figure out how to properly support this kind of query.

Copy link
Contributor

@zygoloid zygoloid left a comment

Choose a reason for hiding this comment

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

I'm happy to expand this and see if the fuzzer manages to find ways to break it :)

@zygoloid zygoloid added this pull request to the merge queue Oct 7, 2024
Merged via the queue into carbon-language:trunk with commit 6439f90 Oct 7, 2024
8 checks passed
@geoffromer geoffromer deleted the let-block branch October 7, 2024 23:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants