Skip to content

compiler: Fixtures to show challenge with setters#29593

Closed
josephsavona wants to merge 1 commit intogh/josephsavona/18/basefrom
gh/josephsavona/18/head
Closed

compiler: Fixtures to show challenge with setters#29593
josephsavona wants to merge 1 commit intogh/josephsavona/18/basefrom
gh/josephsavona/18/head

Conversation

@josephsavona
Copy link
Copy Markdown
Member

@josephsavona josephsavona commented May 25, 2024

Stack from ghstack (oldest at bottom):

I quickly experimented with enabling support for get/set syntax. This confirmed my suspicion that these won't be safe without additional work. The "set" fixture example shows how we assume PropertyStore is only a Store effect on the object, but with a setter it needs to be modeled as an arbitrary mutation.

If we were to support getters and setters I think we'd need to:

  • Support this syntax (maybe limited to just within object methods)
  • Enforce that getters cannot mutate any free variables, cannot mutate this
  • Enforce that setters cannot mutate any free variables (but allow mutating this).

In other words, enforce that you're using getters/setters in a reasonable way that matches our modeling (get is a read, set is a store to the object).

I quickly experimented with enabling support for get/set syntax. This confirmed my suspicion that these won't be safe without additional work. The "set" fixture example shows how we assume PropertyStore is only a Store effect on the object, but with a setter it needs to be modeled as an arbitrary mutation.

If we were to support getters and setters I think we'd need to:
* Support `this` syntax (maybe limited to just within object methods)
* Enforce that getters cannot mutate any free variables, cannot mutate `this`
* Enforce that setters cannot mutate any free variables (but allow mutating `this`).

In other words, enforce that you're using getters/setters in a reasonable way that matches our modeling (get is a read, set is a store to the object).

[ghstack-poisoned]
@vercel
Copy link
Copy Markdown

vercel bot commented May 25, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
react-compiler-playground 🔄 Building (Inspect) Visit Preview 💬 Add feedback May 25, 2024 10:19pm

@facebook-github-bot facebook-github-bot added the React Core Team Opened by a member of the React Core Team label May 25, 2024
josephsavona added a commit that referenced this pull request May 25, 2024
I quickly experimented with enabling support for get/set syntax. This confirmed my suspicion that these won't be safe without additional work. The "set" fixture example shows how we assume PropertyStore is only a Store effect on the object, but with a setter it needs to be modeled as an arbitrary mutation.

If we were to support getters and setters I think we'd need to:
* Support `this` syntax (maybe limited to just within object methods)
* Enforce that getters cannot mutate any free variables, cannot mutate `this`
* Enforce that setters cannot mutate any free variables (but allow mutating `this`).

In other words, enforce that you're using getters/setters in a reasonable way that matches our modeling (get is a read, set is a store to the object).

ghstack-source-id: ca6337f
Pull Request resolved: #29593
@react-sizebot
Copy link
Copy Markdown

Comparing: b078c81...6d75e61

Critical size changes

Includes critical production bundles, as well as any change greater than 2%:

Name +/- Base Current +/- gzip Base gzip Current gzip
oss-stable/react-dom/cjs/react-dom.production.js = 6.66 kB 6.66 kB = 1.82 kB 1.82 kB
oss-stable/react-dom/cjs/react-dom-client.production.js = 496.04 kB 496.04 kB = 88.77 kB 88.77 kB
oss-experimental/react-dom/cjs/react-dom.production.js = 6.67 kB 6.67 kB = 1.83 kB 1.83 kB
oss-experimental/react-dom/cjs/react-dom-client.production.js = 500.84 kB 500.84 kB = 89.46 kB 89.46 kB
facebook-www/ReactDOM-prod.classic.js = 593.48 kB 593.48 kB = 104.38 kB 104.38 kB
facebook-www/ReactDOM-prod.modern.js = 569.87 kB 569.87 kB = 100.77 kB 100.77 kB
test_utils/ReactAllWarnings.js Deleted 63.82 kB 0.00 kB Deleted 15.95 kB 0.00 kB

Significant size changes

Includes any change greater than 0.2%:

Expand to show
Name +/- Base Current +/- gzip Base gzip Current gzip
test_utils/ReactAllWarnings.js Deleted 63.82 kB 0.00 kB Deleted 15.95 kB 0.00 kB

Generated by 🚫 dangerJS against 6d75e61

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CLA Signed Component: React Compiler 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.

3 participants