Skip to content

[compiler] Override type provider for hook-like names #30868

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

Closed
wants to merge 5 commits into from

Conversation

josephsavona
Copy link
Member

@josephsavona josephsavona commented Sep 3, 2024

Stack from ghstack (oldest at bottom):

If an imported name is hook-like but a type provider provides a non-hook type, we now override the returned value and treat it as a generic custom hook. This is meant as an extra check to prevent miscompilation.

If an imported name is hook-like but a type provider provides a non-hook type, we now override the returned value and treat it as a generic custom hook. This is meant as an extra check to prevent miscompilation.

[ghstack-poisoned]
Copy link

vercel bot commented Sep 3, 2024

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

Name Status Preview Comments Updated (UTC)
react-compiler-playground ✅ Ready (Inspect) Visit Preview 💬 Add feedback Sep 5, 2024 4:19pm

josephsavona added a commit that referenced this pull request Sep 3, 2024
If an imported name is hook-like but a type provider provides a non-hook type, we now override the returned value and treat it as a generic custom hook. This is meant as an extra check to prevent miscompilation.

ghstack-source-id: 28539db
Pull Request resolved: #30868
@facebook-github-bot facebook-github-bot added CLA Signed React Core Team Opened by a member of the React Core Team labels Sep 3, 2024
If an imported name is hook-like but a type provider provides a non-hook type, we now override the returned value and treat it as a generic custom hook. This is meant as an extra check to prevent miscompilation.

[ghstack-poisoned]
josephsavona added a commit that referenced this pull request Sep 3, 2024
If an imported name is hook-like but a type provider provides a non-hook type, we now override the returned value and treat it as a generic custom hook. This is meant as an extra check to prevent miscompilation.

ghstack-source-id: 740a7cc
Pull Request resolved: #30868
If an imported name is hook-like but a type provider provides a non-hook type, we now override the returned value and treat it as a generic custom hook. This is meant as an extra check to prevent miscompilation.

[ghstack-poisoned]
josephsavona added a commit that referenced this pull request Sep 4, 2024
If an imported name is hook-like but a type provider provides a non-hook type, we now override the returned value and treat it as a generic custom hook. This is meant as an extra check to prevent miscompilation.

ghstack-source-id: d001639
Pull Request resolved: #30868
) {
return propertyType;
}
return this.#getCustomHookType();
Copy link
Contributor

Choose a reason for hiding this comment

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

We might produce invalid compilation output on false positives (i.e. functions that are hook-named but not hooks) as hook calls freeze args and returned results whereas regular function calls don't. Imports being renamed nonhook <--> hook look a bit suspicious, but we haven't had any practical issues.

import {foo as useHookNameForNonHook} ...

Seems reasonable as our internal use is currently pretty limited. It might be a good idea to warn / bailout here in the future if we find cases of calls being incorrectly inferred as hooks.

@@ -48,6 +48,14 @@ export function makeSharedRuntimeTypeProvider({
returnType: {kind: 'type', name: 'Primitive'},
returnValueKind: ValueKindEnum.Primitive,
},
useArrayConcatNotTypedAsHook: {
Copy link
Contributor

@mofeiZ mofeiZ Sep 4, 2024

Choose a reason for hiding this comment

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

This feels a bit like a InvalidConfigError (and something we can validate when parsing out EnvironmentConfigs).

It's one thing to have assume that unknown properties are hooks based on naming, but, if the type provider lists a hook-named property as a function, I think we should either respect that or throw with an error

Copy link
Contributor

@mofeiZ mofeiZ left a comment

Choose a reason for hiding this comment

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

Overall makes sense! A few non-blocking suggestions / questions but feel free to land without any changes

If an imported name is hook-like but a type provider provides a non-hook type, we now override the returned value and treat it as a generic custom hook. This is meant as an extra check to prevent miscompilation.

[ghstack-poisoned]
josephsavona added a commit that referenced this pull request Sep 4, 2024
If an imported name is hook-like but a type provider provides a non-hook type, we now override the returned value and treat it as a generic custom hook. This is meant as an extra check to prevent miscompilation.

ghstack-source-id: ac69abc
Pull Request resolved: #30868
If an imported name is hook-like but a type provider provides a non-hook type, we now override the returned value and treat it as a generic custom hook. This is meant as an extra check to prevent miscompilation.

[ghstack-poisoned]
josephsavona added a commit that referenced this pull request Sep 5, 2024
If an imported name is hook-like but a type provider provides a non-hook type, we now override the returned value and treat it as a generic custom hook. This is meant as an extra check to prevent miscompilation.

ghstack-source-id: 113ddf3
Pull Request resolved: #30868
@josephsavona
Copy link
Member Author

Yeah, good feedback. I was torn on whether to go the validation route or this approach, i'm gonna switch to just validating that the config was correct.

josephsavona added a commit that referenced this pull request Sep 5, 2024
Alternative to #30868. The goal is to ensure that the types coming out of moduleTypeProvider are valid wrt to hook typing. If something is named like a hook, then it must be typed as a hook (or don't type it).

[ghstack-poisoned]
josephsavona added a commit that referenced this pull request Sep 5, 2024
Alternative to #30868. The goal is to ensure that the types coming out of moduleTypeProvider are valid wrt to hook typing. If something is named like a hook, then it must be typed as a hook (or don't type it).

ghstack-source-id: 787575b
Pull Request resolved: #30888
josephsavona added a commit that referenced this pull request Sep 5, 2024
Alternative to #30868. The goal is to ensure that the types coming out of moduleTypeProvider are valid wrt to hook typing. If something is named like a hook, then it must be typed as a hook (or don't type it).

[ghstack-poisoned]
josephsavona added a commit that referenced this pull request Sep 5, 2024
Alternative to #30868. The goal is to ensure that the types coming out of moduleTypeProvider are valid wrt to hook typing. If something is named like a hook, then it must be typed as a hook (or don't type it).

ghstack-source-id: ebde772
Pull Request resolved: #30888
josephsavona added a commit that referenced this pull request Sep 5, 2024
Alternative to #30868. The goal is to ensure that the types coming out of moduleTypeProvider are valid wrt to hook typing. If something is named like a hook, then it must be typed as a hook (or don't type it).

[ghstack-poisoned]
josephsavona added a commit that referenced this pull request Sep 5, 2024
Alternative to #30868. The goal is to ensure that the types coming out of moduleTypeProvider are valid wrt to hook typing. If something is named like a hook, then it must be typed as a hook (or don't type it).

ghstack-source-id: ea413d6
Pull Request resolved: #30888
josephsavona added a commit that referenced this pull request Sep 5, 2024
Alternative to #30868. The goal is to ensure that the types coming out of moduleTypeProvider are valid wrt to hook typing. If something is named like a hook, then it must be typed as a hook (or don't type it).

[ghstack-poisoned]
josephsavona added a commit that referenced this pull request Sep 5, 2024
Alternative to #30868. The goal is to ensure that the types coming out of moduleTypeProvider are valid wrt to hook typing. If something is named like a hook, then it must be typed as a hook (or don't type it).

ghstack-source-id: 0d72aa5
Pull Request resolved: #30888
josephsavona added a commit that referenced this pull request Sep 5, 2024
Alternative to #30868. The goal is to ensure that the types coming out of moduleTypeProvider are valid wrt to hook typing. If something is named like a hook, then it must be typed as a hook (or don't type it).

[ghstack-poisoned]
josephsavona added a commit that referenced this pull request Sep 5, 2024
Alternative to #30868. The goal is to ensure that the types coming out of moduleTypeProvider are valid wrt to hook typing. If something is named like a hook, then it must be typed as a hook (or don't type it).

ghstack-source-id: fc389f3
Pull Request resolved: #30888
josephsavona added a commit that referenced this pull request Sep 5, 2024
Alternative to #30868. The goal is to ensure that the types coming out of moduleTypeProvider are valid wrt to hook typing. If something is named like a hook, then it must be typed as a hook (or don't type it).

[ghstack-poisoned]
josephsavona added a commit that referenced this pull request Sep 5, 2024
Alternative to #30868. The goal is to ensure that the types coming out of moduleTypeProvider are valid wrt to hook typing. If something is named like a hook, then it must be typed as a hook (or don't type it).

ghstack-source-id: 3e8b5a0
Pull Request resolved: #30888
josephsavona added a commit that referenced this pull request Sep 10, 2024
Alternative to #30868. The goal is to ensure that the types coming out of moduleTypeProvider are valid wrt to hook typing. If something is named like a hook, then it must be typed as a hook (or don't type it).

ghstack-source-id: 3e8b5a0
Pull Request resolved: #30888
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.

3 participants