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 Bug]: null reference exception if you assume a value is not null in a callback (which can be valid) #31551

Open
1 of 4 tasks
lukpsaxo opened this issue Nov 15, 2024 · 4 comments
Assignees
Labels
Component: Optimizing Compiler Status: Unconfirmed A potential issue that we haven't yet confirmed as a bug Type: Bug

Comments

@lukpsaxo
Copy link

What kind of issue is this?

  • React Compiler core (the JS output is incorrect, or your app works incorrectly after optimization)
  • babel-plugin-react-compiler (build issue installing or using the Babel plugin)
  • eslint-plugin-react-compiler (build issue installing or using the eslint plugin)
  • react-compiler-healthcheck (build issue installing or using the healthcheck script)

Link to repro

https://playground.react.dev/#N4Igzg9grgTgxgUxALhAMygOzgFwJYSYAEAKgmDgBTBEC2AhgJ4BGCAclADacA0RAvgEoiwADrEicQhUnMiAXiJQwCAML1uzenADWlSsPkA+EeKLmiaTNSIAPZHSasO3AHS2BggNxmBfANoMLOxcvAC63uK+MAg4sMRBzqFEAPxEADxgAA70xAD0Jg7pACZ4AG5EhKqceLrywHDM-EQFPpj8IPxAA

Repro steps

Similiar to #31550 but with useCallback and no external deps.

You seem to assume that accessing x.y is safe in the render method if it is safe in a callback.

How often does this bug happen?

Every time

What version of React are you using?

18.3.1

What version of React Compiler are you using?

19.0.0-beta-a7bf2bd-20241110

@lukpsaxo lukpsaxo added Component: Optimizing Compiler Status: Unconfirmed A potential issue that we haven't yet confirmed as a bug Type: Bug labels Nov 15, 2024
@lukpsaxo
Copy link
Author

Similiar to #31269 but that is fixed on the playground and this is not.

@mofeiZ
Copy link
Contributor

mofeiZ commented Nov 15, 2024

Thanks for the report! The compiler is making this optimization assuming that react code is using a type system (e.g. typescript / flow) which would error on unguarded accesses to maybe-null variables (see typescript playground).
image

We recognize that not everyone is using a type system, and that even typescript / flow developers may have unsound casts (e.g. (maybeNull as any).x) in their code.

While we work on adding type-system aware configs, you can manually configure the compiler to not make these assumptions by adding enableTreatFunctionDepsAsConditional to your config (see playground). See our docs for full instructions on editing your compiler config.

// babel.config.js
const ReactCompilerConfig = {
  /* ... */
  environment: {
    enableTreatFunctionDepsAsConditional: true,
  }
};

@lukeapage
Copy link

Thanks - we do use typescript and don’t have any suppressions here - will check what’s happening. I think on our codebase we won’t take the risk and disable the optimization as you suggest, thanks.

@lukpsaxo
Copy link
Author

lukpsaxo commented Nov 18, 2024

The issue is typescript is not faultless - they make decisions for the best ergonomics but not always accurate.

e.g. my issue was this case:

https://www.typescriptlang.org/play/?#code/MYewdgzgLgBAHgLhgQQE6oIYE8A8BvGDJAIxBABsBTDMGAXwD4YBeGAbQF0BuAKB9EiwAllEoBbFvDYAGbnwHRCkkeIB0GXjyA

notice there is no typescript error but the object will clearly be undefined.

I've abstracted this from a 400 line component - in that component the object is referenced in a callback that only ends up being attached if the array has items in it. but initially it has no items, but react compiler pulls up access to this property. So... without react compiler it will never error. With react compiler it will error on a empty array...

even if react compiler understands typescript, this optimization is always going to lead to occasional issues, some of which might not be detectable until production. I would love to see this optimization and any other inherently unsafe optimizations opt-in.

mofeiZ added a commit that referenced this issue Nov 19, 2024
See #31551 for context

We've rolled out internally with `enableTreatFunctionDepsAsConditional: false` and encountered no issues, but this is technically an unsound optimization as both flow and typescript have sources of unsoundness:
- typing array accesses / other unknown keys as `TValue | undefined`  (typescript's noUncheckedIndexedAccess)
- explicit and inferred `any`
- unsound typecasts

Note that removing this optimization results in less granular dependencies for ~3% of files on a large Meta project ([link](https://www.internalfb.com/phabricator/paste/view/P1681742214)).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component: Optimizing Compiler Status: Unconfirmed A potential issue that we haven't yet confirmed as a bug Type: Bug
Projects
None yet
Development

No branches or pull requests

3 participants