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

[compiler][nocommit] Quick sketch of types on places #31575

Draft
wants to merge 1 commit into
base: gh/josephsavona/60/base
Choose a base branch
from

Conversation

josephsavona
Copy link
Contributor

@josephsavona josephsavona commented Nov 19, 2024

Stack from ghstack (oldest at bottom):

This is a quick sketch of moving types from Identifier to Places so that we can have flow-sensitive types. The intent isn't to ship this but to quickly explore in order to figure out concrete challenges, to inform a "real" implementation.

Some observations:

  • ReactiveScopeDependency/Declaration now need types. We use the type of their identifier currently, so we'd have to populate a type for them instead. But if we do flow-sensitive types, there won't be one obvious correct type to use! Consider a scope that uses x twice, once where we can infer its a primitive and one where we can't. We should treat this like phi typing and only infer a precise type for the dep/decl if all references have the same type.

  • InferMutableRanges's aliasing logic uses a DisjointSet<Identifier> and checks the types for some things (refs in particular). So the obvious approach is to replace that with a DisjointSet<Place>. While doing that I was reminded that the way we handle aliasing for phis is kind of weird. We currently delay creating an alias until we know the phi is mutated later, but we don't do the same thing for things like x = y (ie we eagerly alias). Switching to paths is a good chance to revisit the aliasing.

  • InferTypes gets tricky because we still want different places with the same identifier to get the same type (for now, until we introduce flow-sensitive typing). But every Place has its own type instance. So for now we can basically keep a mapping of IdentifierId to a canonical Type and use this for all the inference. The actual implementation in the PR is messier than that since i started with a variant of flow-sensitive typing and then rolled it back.

For actual flow-sensitive typing (not implemented here) there's a sort of inverse phi situation. Consider a variant of @mofeiZ's recent find:

function Component({y}) {
  let x = makeValue(y);
  let result;
  if (...cond...) {
    result = ...x... // do something with x
  } else {
    result = ...x... // do something else with x
  }
  return result;
}

If both branches of the if can infer x as a number, then it's sound to infer makeValue(y) as producing a number. However, if you take away the else branch then it might not be, since now there's a code path (the fallthrough) in which we're not sure of the type. This is just like a phi for variable reassignment, but at the type level. And it's also happening in reverse — the later "operands" (usages of x) flow backwards into the "phi" that is the type of x before the if/else. We'd have to build up a mapping like this and build the appropriate type equations.

This is a quick sketch of moving types from Identifier to Places so that we can have flow-sensitive types. The intent isn't to ship this but to quickly explore in order to figure out concrete challenges, to inform a "real" implementation.

Some observations:

* ReactiveScopeDependency/Declaration now need types. We use the type of their identifier currently, so we'd have to populate a type for them instead. But if we do flow-sensitive types, there won't be one obvious correct type to use! Consider a scope that uses `x` twice, once where we can infer its a primitive and one where we can't. We should treat this like phi typing and only infer a precise type for the dep/decl if all references have the same type.

* InferMutableRanges's aliasing logic uses a `DisjointSet<Identifier>` and checks the types for some things (refs in particular). So the obvious approach is to replace that with a `DisjointSet<Place>`. While doing that I was reminded that the way we handle aliasing for phis is kind of weird. We currently delay creating an alias until we know the phi is mutated later, but we don't do the same thing for things like `x = y` (ie we eagerly alias). Switching to paths is a good chance to revisit the aliasing.

* InferTypes gets tricky because we still want different places with the same identifier to get the same type (for now, until we introduce flow-sensitive typing). But every Place has its own type instance. So for now we can basically keep a mapping of IdentifierId to a canonical Type and use this for all the inference. The actual implementation in the PR is messier than that since i started with a variant of flow-sensitive typing and then rolled it back.

For actual flow-sensitive typing (not implemented here) there's a sort of inverse phi situation. Consider a variant of @mofeiZ's recent find:

```
function Component({y}) {
  let x = makeValue(y);
  let result;
  if (...cond...) {
    result = ...x... // do something with x
  } else {
    result = ...x... // do something else with x
  }
  return result;
}
```

If both branches of the if can infer `x` as a number, then it's sound to infer `makeValue(y)` as producing a number. However, if you take away the else branch then it might not be, since now there's a code path (the fallthrough) in which we're not sure of the type. This is just like a phi for variable reassignment, but at the type level. And it's also happening in reverse — the later "operands" (usages of x) flow backwards into the "phi" that is the type of x before the if/else. We'd have to build up a mapping like this and build the appropriate type equations.

[ghstack-poisoned]
Copy link

vercel bot commented Nov 19, 2024

Deployment failed with the following error:

You don't have permission to create a Preview Deployment for this project.

View Documentation: https://vercel.com/docs/accounts/team-members-and-roles

josephsavona added a commit that referenced this pull request Nov 19, 2024
This is a quick sketch of moving types from Identifier to Places so that we can have flow-sensitive types. The intent isn't to ship this but to quickly explore in order to figure out concrete challenges, to inform a "real" implementation.

Some observations:

* ReactiveScopeDependency/Declaration now need types. We use the type of their identifier currently, so we'd have to populate a type for them instead. But if we do flow-sensitive types, there won't be one obvious correct type to use! Consider a scope that uses `x` twice, once where we can infer its a primitive and one where we can't. We should treat this like phi typing and only infer a precise type for the dep/decl if all references have the same type.

* InferMutableRanges's aliasing logic uses a `DisjointSet<Identifier>` and checks the types for some things (refs in particular). So the obvious approach is to replace that with a `DisjointSet<Place>`. While doing that I was reminded that the way we handle aliasing for phis is kind of weird. We currently delay creating an alias until we know the phi is mutated later, but we don't do the same thing for things like `x = y` (ie we eagerly alias). Switching to paths is a good chance to revisit the aliasing.

* InferTypes gets tricky because we still want different places with the same identifier to get the same type (for now, until we introduce flow-sensitive typing). But every Place has its own type instance. So for now we can basically keep a mapping of IdentifierId to a canonical Type and use this for all the inference. The actual implementation in the PR is messier than that since i started with a variant of flow-sensitive typing and then rolled it back.

For actual flow-sensitive typing (not implemented here) there's a sort of inverse phi situation. Consider a variant of mofeiZ's recent find:

```
function Component({y}) {
  let x = makeValue(y);
  let result;
  if (...cond...) {
    result = ...x... // do something with x
  } else {
    result = ...x... // do something else with x
  }
  return result;
}
```

If both branches of the if can infer `x` as a number, then it's sound to infer `makeValue(y)` as producing a number. However, if you take away the else branch then it might not be, since now there's a code path (the fallthrough) in which we're not sure of the type. This is just like a phi for variable reassignment, but at the type level. And it's also happening in reverse — the later "operands" (usages of x) flow backwards into the "phi" that is the type of x before the if/else. We'd have to build up a mapping like this and build the appropriate type equations.

ghstack-source-id: f5a01fd1836edf20e2a56cb3cbed8470310f2b2f
Pull Request resolved: #31575
@facebook-github-bot facebook-github-bot added CLA Signed React Core Team Opened by a member of the React Core Team labels Nov 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed React Core Team Opened by a member of the React Core Team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants