-
Notifications
You must be signed in to change notification settings - Fork 12.6k
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
[Wip] master stateless overload #13640
Conversation
… array # Conflicts: # src/compiler/visitor.ts
Add language service support for JSXAttributes Add completion support Add find-all-references support Add goto-definition support
…alType property directly
Address comment: using ternary operator, fix comments
…AttributesTypeFromAttributesProperty
Address comment: fix spelling mistakes Address comment: calling `checkApplicableSignatureForJsxOpeningLikeElement` from inside `checkApplicableSignature` Address comment: fix spelling, rename function to be more consistent Address comment: minor fix indentation, fix function name isObjectLiteralPropertyDeclaration => isObjectLiteralElement Address PR: gotoDefinition return the last signature when there is an error in statelss function component Address PR: convert Foreach to for...of Address comment: fix type, inline code, clarify name of variables
… failure to resolve signature cases
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few comments before looking at the checker and services code.
src/compiler/types.ts
Outdated
@@ -2697,6 +2702,7 @@ | |||
/* @internal */ isReferenced?: boolean; // True if the symbol is referenced elsewhere | |||
/* @internal */ isReplaceableByMethod?: boolean; // Can this Javascript class property be replaced by a method symbol? | |||
/* @internal */ isAssigned?: boolean; // True if the symbol is a parameter with assignments | |||
/* @internal */ syntheticKind?: SyntheticSymbolKind; // Synthetic symbols are either spread or union/intersection |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This isn't used anymore, so you should remove this line. I forgot to remove SyntheticSymbolKind
itself, but I'll do that in the spread-type PR. #Closed
@@ -1824,15 +1826,21 @@ namespace ts { | |||
write("<"); | |||
emitJsxTagName(node.tagName); | |||
write(" "); | |||
emitList(node, node.attributes, ListFormat.JsxElementAttributes); | |||
// We are checking here so we won't re-enter the emiting pipeline and emit extra sourcemap |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
typo: emiting → emitting
src/compiler/emitter.ts
Outdated
writeIfAny(node.attributes, " "); | ||
emitList(node, node.attributes, ListFormat.JsxElementAttributes); | ||
writeIfAny(node.attributes.properties, " "); | ||
// We are checking here so we won't re-enter the emiting pipeline and emit extra sourcemap |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you give an example when this would happen?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will confirm with Ron. My understanding is that we only want to call emit if the attributes are not empty (as this can cause extra sourcemap)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Comments from reviewing checker
src/compiler/checker.ts
Outdated
if ((relation === assignableRelation || relation === comparableRelation) && (type === globalObjectType || isEmptyObjectType(resolved)) || | ||
resolved.stringIndexInfo || | ||
(resolved.numberIndexInfo && isNumericLiteralName(name)) || | ||
getPropertyOfType(type, name)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it be equivalent just to add a line || (isComparingJsxAttributes && !isUnhyphenatedJsxName(name)))
to the original code? If that's the case, I'd just do that. I think it's easier to read as a compact series of cases that return true.
reportError(Diagnostics.Property_0_does_not_exist_on_type_1, symbolToString(prop), typeToString(target)); | ||
} | ||
else { | ||
errorNode = prop.valueDeclaration; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do errors get more precise if you set errorNode = prop.valueDeclaration
? I noticed that the error spans the entire JSX literal currently.
If that change works, then I would also suggest pushing the isJsxAttributes
predicate down inside the reportError
call since the other arguments are the same for both branches. #Resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Talk offline, the error message helps elaborate this further so it should be ok for now.
src/compiler/checker.ts
Outdated
// When we trying to resolve JsxOpeningLikeElement as a stateless function element, we will already give its attributes a contextual type | ||
// which is a type of the parameter of the signature we are trying out. | ||
// If there is no contextual type (e.g. we are trying to resolve stateful component), get attributes type from resolving element's tagName | ||
const attributesType = getContextualType(<Expression>attribute.parent) || getAttributesTypeFromJsxOpeningLikeElement(<JsxOpeningLikeElement>attribute.parent.parent); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
getContextualType
calls getAttributesTypeFromJsxOpeningLikeElement(parent)
for JsxOpeningLikeElement
, so when you call getContextualType(attribute.parent)
shouldn't it be equivalent to getAttributesTypeFromJsxOpeningLikeElement(<JsxOpeningLikeElement>attribute.parent.parent)
? If so, you can call only getContextualType
here. #Closed
src/compiler/checker.ts
Outdated
} | ||
|
||
/** | ||
* Get the attributes type which is the type that indicate which attributes are valid on the given JSXOpeningLikeElement. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- The jsdoc tags here are low-value -- they just restate the type and function names.
- I would rewrite the description as "Get the attributes type, which indicates the attributes that are valid on the given JSXOpeningLikeElement." #Closed
/** | ||
* Get attributes type of the given custom opening-like JSX element. | ||
* This function is intended to be called from a caller that handles intrinsic JSX element already. | ||
* @param node a custom JSX opening-like element |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same here, the param tag doesn't have interesting information and there is no tag for shouldIncludeAllStatelessAttributesType, which is the more mysterious one since its type is just boolean. #Closed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The comment above the call from getAllAttributesType...
is a good candidate for explaining the boolean. #Closed
src/compiler/checker.ts
Outdated
@@ -12218,7 +12443,7 @@ namespace ts { | |||
return jsxElementClassType; | |||
} | |||
|
|||
/// Returns all the properties of the Jsx.IntrinsicElements interface | |||
// Returns all the properties of the Jsx.IntrinsicElements interface |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
might as well turn this into jsdoc while you're here #Closed
src/compiler/checker.ts
Outdated
} | ||
} | ||
} | ||
checkJsxAttributesAssignableToTagNameAttributes(node); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you move the definition of this function closer to its call? #Closed
src/compiler/checker.ts
Outdated
@@ -12744,6 +12940,13 @@ namespace ts { | |||
let isDecorator: boolean; | |||
let spreadArgIndex = -1; | |||
|
|||
if (isJsxOpeningLikeElement(node)) { | |||
// For JSX opening-like element, we will ignore regular arity check (which is what is done here). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you make this comment shorter? Basically just give a link to the function that does the work instead: "The arity check will be done in checkApplicableSignatureForJsxOpeningLikeElement
instead". #Closed
src/compiler/checker.ts
Outdated
// However "context" and "updater" are implicit and can't be specify by users. Only the first parameter, props, | ||
// can be specified by users through attributes property. | ||
const paramType = getTypeAtPosition(signature, 0); | ||
const argType = checkExpressionWithContextualType(node.attributes, paramType, /*contextualMapper*/ undefined); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think attributesType
is a better name. #Closed
src/compiler/checker.ts
Outdated
@@ -13422,6 +13669,11 @@ namespace ts { | |||
// in arguments too early. If possible, we'd like to only type them once we know the correct | |||
// overload. However, this matters for the case where the call is correct. When the call is | |||
// an error, we don't need to exclude any arguments, although it would cause no harm to do so. | |||
if (isJsxOpeningOrSelfClosingElement) { | |||
// If there is not result, just return the last one we try as a candidate. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- It looks like this
if
should go before the preceding comment block that talks aboutexcludeArgument
. - I would delete the first line of the comment because it just repeats what the code itself says. #Closed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Comments from services side.
src/services/goToDefinition.ts
Outdated
// /*secondSource*/declare function MainButton(linkProps: LinkProps): JSX.Element; | ||
// /*thirdSource*/declare function MainButton(props: ButtonProps | LinkProps): JSX.Element; | ||
// let opt = <Main/*firstTarget*/Button />; // We get undefined for resolved signature indicating an error, then just return the first declaration | ||
const {symbolName, symbolKind, containerName} = getSymbolInfo(typeChecker, symbol, node); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
double-space before the equals sign. Does the linter catch this? #Closed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nop, the linter doesn't catch it .we should fix the linter though. I fix this space in my commit
src/services/goToDefinition.ts
Outdated
// For JSX opening-like element, the tag can be resolved either as stateful component (e.g class) or stateless function component. | ||
// Because if it is a stateless function component with an error while trying to resolve the signature, we don't want to return all | ||
// possible overloads but just the first one. | ||
// For example: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This example didn't help my understanding. #Closed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From in-person discussion, the comment is wrong now and needs to be updated. #Closed
src/services/symbolDisplay.ts
Outdated
if (!signature && candidateSignatures.length) { | ||
// Use the first candidate: | ||
signature = candidateSignatures[0]; | ||
} | ||
|
||
const useConstructSignatures = callExpression.kind === SyntaxKind.NewExpression || callExpression.expression.kind === SyntaxKind.SuperKeyword; | ||
let useConstructSignatures = false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- I liked the single-expression form better.
- Did you add
isCallExpression
because of this change? super doesn't seem related to stateless JSX overload resolution. #Closed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep because callExpressionLike include JSXOpeningElement as well. I did fix it up to use single-expression
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Comments from the test
return this._buildMainButton(props); | ||
} | ||
|
||
const b0 = <MainButton {...{onClick: (k) => {console.log(k)}}} extra />; // k has type any |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think the comment is correct; k should have the type 'left' | 'right'
#Closed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah. I fix the contextual type and forgot to update the comment. Thanks
~~~~~~~~~~~~~~~~ | ||
!!! error TS2322: Type '{ x: number; y: number; }' is not assignable to type 'Attribs1'. | ||
!!! error TS2322: Types of property 'x' are incompatible. | ||
!!! error TS2322: Type 'number' is not assignable to type 'string'. | ||
|
||
// OK |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this should not be Error (extras not allowed)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what do you mean? Do you mean on <test1 {...obj6} />
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I meant should be. Right the comment is // OK
but <test1 {...obj6} />
actually causes an error.
@@ -23,26 +23,26 @@ tests/cases/conformance/jsx/file.tsx(30,1): error TS2324: Property 'toString' is | |||
|
|||
function make1<T extends {x: string}> (obj: T) { | |||
return <test1 {...obj} />; // OK | |||
~~~~~~~~ | |||
!!! error TS2698: Spread types may only be created from object types. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
these baselines will change back to OK after spread types are merged, which might be before this PR or might be after.
Type '42' is not assignable to type 'string'. | ||
tests/cases/conformance/jsx/file.tsx(26,15): error TS2322: Type '{ naaaaaaame: "no"; }' is not assignable to type 'IntrinsicAttributes & { name?: string; }'. | ||
Property 'naaaaaaame' does not exist on type 'IntrinsicAttributes & { name?: string; }'. | ||
tests/cases/conformance/jsx/file.tsx(31,23): error TS2322: Type '{}' is not assignable to type 'IntrinsicAttributes & { "prop-name": string; }'. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The reason the error is missing extra-prop-name
is that it's a hyphenated name, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yep. So the "extra-prop-name" doesn't exist in attribute type so it doesn't participate in the type check and get filter out
|
||
// Error | ||
let x = <TextComponent editable={true} /> | ||
~~~~~~~~~~~~~ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This particular error should go away when spread types are available, I think.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah this error is related to this bug #13526
|
||
let y1 = <TextComponent {...textPropsFalse} /> | ||
~~~~~~~~~~~~~ | ||
!!! error TS2600: JSX element attributes type '({ editable: false; } & { children?: ReactNode; }) | ({ editable: true; onEdit: (newText: string) => void; } & { children?: ReactNode; })' may not be a union type. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same here; after spread types are available, these errors will go away and may expose others.
@@ -0,0 +1,62 @@ | |||
=== tests/cases/conformance/jsx/file.tsx === |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
there shouldn't be an *.errors.txt and a *.symbols + *.js. Which is correct?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
errors.txt. I am surprised the test doesn't fail. Remove the symbols and types
|
||
verify.quickInfos({ | ||
1: "function ComponentSpecific<{}>(l: {\n prop: {};\n}): any", | ||
2: "function ComponentSpecific<{}>(l: {\n prop: {};\n}): any" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is U={}
and the return type any
instead of JSX.Element
? Is the lack of spread types causing errors in the code?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The reason it is "any" because we will fail to get JSX.Element (as there is no react library loaded here)....I believe that U={} is due to lack of spread types.
//// export function MainButton(props: ButtonProps): JSX.Element { | ||
//// return this._buildMainButton(props); | ||
//// } | ||
//// let e1 = <MainButton/*1*/ /*2*/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what is the difference between /*1*/
and /*2*/
here? They look like they should be the same, and in fact they are.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you are right. I fix that Just realize why I did that because I want to make sure we will show signature help both when we are on the tagName and empty attributes
//// let e1 = <MainButton/*1*/ /*2*/ | ||
|
||
goTo.marker("1"); | ||
// The second overload is chosen because the first doesn't get selected by choose overload as it has incorrect arity |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This comment seems wrong, because it looks like the first overload is chosen after all.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you are right. the comment is not. I must have forgot to update it when I was experimenting
🔔 @RyanCavanaugh if you would like to give another look :) |
Talk with @RyanCavanaugh, we should have better name for this one |
src/compiler/checker.ts
Outdated
@@ -590,7 +590,7 @@ namespace ts { | |||
return node.kind === SyntaxKind.SourceFile && !isExternalOrCommonJsModule(<SourceFile>node); | |||
} | |||
|
|||
function getSymbol(symbols: SymbolTable, name: string, meaning: SymbolFlags): Symbol { | |||
function getSymbol(symbols: SymbolTable, name: string, meaning: SymbolFlags): Symbol { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wrong indent?
src/compiler/checker.ts
Outdated
@@ -3362,6 +3362,11 @@ namespace ts { | |||
const type = checkDeclarationInitializer(declaration); | |||
return addOptionality(type, /*optional*/ declaration.questionToken && includeOptionality); | |||
} | |||
else if (isJsxAttribute(declaration)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No need for else
here
src/compiler/checker.ts
Outdated
return false; | ||
} | ||
} | ||
if (checkTypeRelatedTo(attributesType, paramType, relation, /*errorNode*/ undefined, headMessage)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can just return this value directly
src/compiler/checker.ts
Outdated
@@ -13105,8 +13349,11 @@ namespace ts { | |||
// `getEffectiveArgumentCount` and `getEffectiveArgumentType` below. | |||
return undefined; | |||
} | |||
else if (isJsxOpeningLikeElement(node)) { | |||
args = node.attributes.properties.length > 0 ? [node.attributes] : []; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Intentional to use []
instead of emptyArray
here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nop. Fix
src/compiler/checker.ts
Outdated
let result: Signature; | ||
for (const type of types) { | ||
// This is mainly to fill in all the candidates if there is one. | ||
result = result && resolveStatelessJsxOpeningLikeElement(openingLikeElement, type, candidatesOutArray); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't this be ||
? I would expect a failing test because of this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't have test around this surprisingly. I am adding one in
# Conflicts: # src/compiler/factory.ts
Update of the previous PR #12107
Fix: #11187 (see: tsxSpreadAttributesResolution1.tsx), #9703 (see : tsxStatelessFunctionComponentOverload6.tsx)
Overview of the change
JSXAttributes
similar toObjectLiteralExpression
. We change the AST structure and store JSXAttributes as a AST Node instead of NodeArrayNote
Currently when we see spread of any type, the entire attributes has any type which means that follow example is allowed even though we should give an error: