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

Fix 8549: Using variable as Jsx tagname #9337

Merged
merged 11 commits into from
Jun 24, 2016
28 changes: 26 additions & 2 deletions src/compiler/checker.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9628,8 +9628,9 @@ namespace ts {
/**
* Returns true iff React would emit this tag name as a string rather than an identifier or qualified name
*/
function isJsxIntrinsicIdentifier(tagName: Identifier | QualifiedName) {
if (tagName.kind === SyntaxKind.QualifiedName) {
function isJsxIntrinsicIdentifier(tagName: JsxTagNameExpression) {
// TODO (yuisu): comment
if (tagName.kind === SyntaxKind.PropertyAccessExpression || tagName.kind === SyntaxKind.ThisKeyword) {
return false;
}
Copy link
Member

Choose a reason for hiding this comment

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

The else case here needs to be hardened since it assumes tagName: Identifier

else {
Expand Down Expand Up @@ -9825,6 +9826,29 @@ namespace ts {
}));
}

// If the elemType is a string type, we have to return anyType to prevent an error downstream as we will try to find construct or call signature of the type
if (elemType.flags & TypeFlags.String) {
return anyType;
}
else if (elemType.flags & TypeFlags.StringLiteral) {
Copy link
Member

Choose a reason for hiding this comment

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

Finish sentence in comment

// If the elemType is a stringLiteral type, we can then provide a check to make sure that the string literal type is one of the Jsx intrinsic element type
const intrinsicElementsType = getJsxType(JsxNames.IntrinsicElements);
if (intrinsicElementsType !== unknownType) {
const stringLiteralTypeName = (<StringLiteralType>elemType).text;
const intrinsicProp = getPropertyOfType(intrinsicElementsType, stringLiteralTypeName);
if (intrinsicProp) {
return getTypeOfSymbol(intrinsicProp);
}
const indexSignatureType = getIndexTypeOfType(intrinsicElementsType, IndexKind.String);
if (indexSignatureType) {
return indexSignatureType;
}
error(node, Diagnostics.Property_0_does_not_exist_on_type_1, stringLiteralTypeName, "JSX." + JsxNames.IntrinsicElements);
}
// If we need to report an error, we already done so here. So just return any to prevent any more error downstream
return anyType;
Copy link
Member

Choose a reason for hiding this comment

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

Doesn't look like you return anyType/unknownType if you couldn't find an intrinsicElementsType for a string literal.

Why not make it

if (intrinsicElementsType === unknownType) {
    return unknownType;
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

that is actually intended that if intrinsicElementsType === unknownType then you just fall through and check for the construct/call though, arguably we should actually return unknownType instead

}

// Get the element instance type (the result of newing or invoking this tag)
const elemInstanceType = getJsxElementInstanceType(node, elemType);

Expand Down
2 changes: 1 addition & 1 deletion src/compiler/emitter.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1217,7 +1217,7 @@ var __awaiter = (this && this.__awaiter) || function (thisArg, _arguments, P, ge
function jsxEmitReact(node: JsxElement | JsxSelfClosingElement) {
/// Emit a tag name, which is either '"div"' for lower-cased names, or
/// 'Div' for upper-cased or dotted names
function emitTagName(name: Identifier | QualifiedName) {
function emitTagName(name: LeftHandSideExpression) {
if (name.kind === SyntaxKind.Identifier && isIntrinsicJsxName((<Identifier>name).text)) {
write('"');
emit(name);
Expand Down
36 changes: 24 additions & 12 deletions src/compiler/parser.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3575,7 +3575,7 @@ namespace ts {
return finishNode(node);
}

function tagNamesAreEquivalent(lhs: EntityName, rhs: EntityName): boolean {
function tagNamesAreEquivalent(lhs: JsxTagNameExpression, rhs: JsxTagNameExpression): boolean {
if (lhs.kind !== rhs.kind) {
return false;
}
Expand All @@ -3584,8 +3584,15 @@ namespace ts {
return (<Identifier>lhs).text === (<Identifier>rhs).text;
}

return (<QualifiedName>lhs).right.text === (<QualifiedName>rhs).right.text &&
tagNamesAreEquivalent((<QualifiedName>lhs).left, (<QualifiedName>rhs).left);
if (lhs.kind === SyntaxKind.ThisKeyword) {
return true;
}

// If we are at this statement then we must have PropertyAccessExpression and because tag name in Jsx element can only
// take forms of JsxTagNameExpression which includes an identifier, "this" expression, or another propertyAccessExpression
// it is safe to case the expression property as such. See parseJsxElementName for how we parse tag name in Jsx element
return (<PropertyAccessExpression>lhs).name.text === (<PropertyAccessExpression>rhs).name.text &&
tagNamesAreEquivalent((<PropertyAccessExpression>lhs).expression as JsxTagNameExpression, (<PropertyAccessExpression>rhs).expression as JsxTagNameExpression);
}


Expand Down Expand Up @@ -3653,7 +3660,7 @@ namespace ts {
Debug.fail("Unknown JSX child kind " + token);
}

function parseJsxChildren(openingTagName: EntityName): NodeArray<JsxChild> {
function parseJsxChildren(openingTagName: LeftHandSideExpression): NodeArray<JsxChild> {
const result = <NodeArray<JsxChild>>[];
result.pos = scanner.getStartPos();
const saveParsingContext = parsingContext;
Expand Down Expand Up @@ -3716,17 +3723,22 @@ namespace ts {
return finishNode(node);
}

function parseJsxElementName(): EntityName {
function parseJsxElementName(): JsxTagNameExpression {
scanJsxIdentifier();
let elementName: EntityName = parseIdentifierName();
// JsxElement can have name in the form of
// propertyAccessExpression
// primaryExpression in the form of an identifier and "this" keyword
// We can't just simply use parseLeftHandSideExpressionOrHigher because then we will start consider class,function etc as a keyword
// We only want to consider "this" as a primaryExpression
let expression: JsxTagNameExpression = token === SyntaxKind.ThisKeyword ?
parseTokenNode<PrimaryExpression>() : parseIdentifierName();
while (parseOptional(SyntaxKind.DotToken)) {
scanJsxIdentifier();
const node: QualifiedName = <QualifiedName>createNode(SyntaxKind.QualifiedName, elementName.pos); // !!!
node.left = elementName;
node.right = parseIdentifierName();
elementName = finishNode(node);
const propertyAccess: PropertyAccessExpression = <PropertyAccessExpression>createNode(SyntaxKind.PropertyAccessExpression, expression.pos);
propertyAccess.expression = expression;
propertyAccess.name = parseRightSideOfDot(/*allowIdentifierNames*/ true);
expression = finishNode(propertyAccess);
}
return elementName;
return expression;
}

function parseJsxExpression(inExpressionContext: boolean): JsxExpression {
Expand Down
6 changes: 4 additions & 2 deletions src/compiler/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1042,11 +1042,13 @@ namespace ts {
closingElement: JsxClosingElement;
}

export type JsxTagNameExpression = PrimaryExpression | PropertyAccessExpression;

/// The opening element of a <Tag>...</Tag> JsxElement
// @kind(SyntaxKind.JsxOpeningElement)
export interface JsxOpeningElement extends Expression {
_openingElementBrand?: any;
tagName: EntityName;
tagName: JsxTagNameExpression;
attributes: NodeArray<JsxAttribute | JsxSpreadAttribute>;
}

Expand All @@ -1073,7 +1075,7 @@ namespace ts {

// @kind(SyntaxKind.JsxClosingElement)
export interface JsxClosingElement extends Node {
tagName: EntityName;
tagName: JsxTagNameExpression;
}

// @kind(SyntaxKind.JsxExpression)
Expand Down
8 changes: 8 additions & 0 deletions tests/baselines/reference/tsxDynamicTagName1.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
//// [tsxDynamicTagName1.tsx]

var CustomTag = "h1";
<CustomTag> Hello World </CustomTag> // No error

//// [tsxDynamicTagName1.jsx]
var CustomTag = "h1";
<CustomTag> Hello World </CustomTag>; // No error
9 changes: 9 additions & 0 deletions tests/baselines/reference/tsxDynamicTagName1.symbols
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
=== tests/cases/conformance/jsx/tsxDynamicTagName1.tsx ===

var CustomTag = "h1";
>CustomTag : Symbol(CustomTag, Decl(tsxDynamicTagName1.tsx, 1, 3))

<CustomTag> Hello World </CustomTag> // No error
>CustomTag : Symbol(CustomTag, Decl(tsxDynamicTagName1.tsx, 1, 3))
>CustomTag : Symbol(CustomTag, Decl(tsxDynamicTagName1.tsx, 1, 3))

11 changes: 11 additions & 0 deletions tests/baselines/reference/tsxDynamicTagName1.types
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
=== tests/cases/conformance/jsx/tsxDynamicTagName1.tsx ===

var CustomTag = "h1";
>CustomTag : string
>"h1" : string

<CustomTag> Hello World </CustomTag> // No error
><CustomTag> Hello World </CustomTag> : any
>CustomTag : string
>CustomTag : string

19 changes: 19 additions & 0 deletions tests/baselines/reference/tsxDynamicTagName2.errors.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
tests/cases/conformance/jsx/tsxDynamicTagName2.tsx(10,1): error TS2339: Property 'customTag' does not exist on type 'JSX.IntrinsicElements'.
tests/cases/conformance/jsx/tsxDynamicTagName2.tsx(10,25): error TS2339: Property 'customTag' does not exist on type 'JSX.IntrinsicElements'.


==== tests/cases/conformance/jsx/tsxDynamicTagName2.tsx (2 errors) ====

declare module JSX {
interface Element { }
interface IntrinsicElements {
div: any
}
}

var customTag = "h1";
<customTag> Hello World </customTag> // This should be an error. The lower-case is look up as an intrinsic element name
~~~~~~~~~~~
!!! error TS2339: Property 'customTag' does not exist on type 'JSX.IntrinsicElements'.
~~~~~~~~~~~~
!!! error TS2339: Property 'customTag' does not exist on type 'JSX.IntrinsicElements'.
15 changes: 15 additions & 0 deletions tests/baselines/reference/tsxDynamicTagName2.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
//// [tsxDynamicTagName2.tsx]

declare module JSX {
interface Element { }
interface IntrinsicElements {
div: any
}
}

var customTag = "h1";
<customTag> Hello World </customTag> // This should be an error. The lower-case is look up as an intrinsic element name

//// [tsxDynamicTagName2.jsx]
var customTag = "h1";
<customTag> Hello World </customTag>; // This should be an error. The lower-case is look up as an intrinsic element name
16 changes: 16 additions & 0 deletions tests/baselines/reference/tsxDynamicTagName3.errors.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
tests/cases/conformance/jsx/tsxDynamicTagName3.tsx(10,1): error TS2339: Property 'h1' does not exist on type 'JSX.IntrinsicElements'.


==== tests/cases/conformance/jsx/tsxDynamicTagName3.tsx (1 errors) ====

declare module JSX {
interface Element { }
interface IntrinsicElements {
div: any
}
}

var CustomTag: "h1" = "h1";
<CustomTag> Hello World </CustomTag> // This should be an error. we will try look up string literal type in JSX.IntrinsicElements
~~~~~~~~~~~
!!! error TS2339: Property 'h1' does not exist on type 'JSX.IntrinsicElements'.
15 changes: 15 additions & 0 deletions tests/baselines/reference/tsxDynamicTagName3.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
//// [tsxDynamicTagName3.tsx]

declare module JSX {
interface Element { }
interface IntrinsicElements {
div: any
}
}

var CustomTag: "h1" = "h1";
<CustomTag> Hello World </CustomTag> // This should be an error. we will try look up string literal type in JSX.IntrinsicElements

//// [tsxDynamicTagName3.jsx]
var CustomTag = "h1";
<CustomTag> Hello World </CustomTag>; // This should be an error. we will try look up string literal type in JSX.IntrinsicElements
16 changes: 16 additions & 0 deletions tests/baselines/reference/tsxDynamicTagName4.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
//// [tsxDynamicTagName4.tsx]

declare module JSX {
interface Element { }
interface IntrinsicElements {
div: any
h1: any
}
}

var CustomTag: "h1" = "h1";
<CustomTag> Hello World </CustomTag>

//// [tsxDynamicTagName4.jsx]
var CustomTag = "h1";
<CustomTag> Hello World </CustomTag>;
26 changes: 26 additions & 0 deletions tests/baselines/reference/tsxDynamicTagName4.symbols
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
=== tests/cases/conformance/jsx/tsxDynamicTagName4.tsx ===

declare module JSX {
>JSX : Symbol(JSX, Decl(tsxDynamicTagName4.tsx, 0, 0))

interface Element { }
>Element : Symbol(Element, Decl(tsxDynamicTagName4.tsx, 1, 20))

interface IntrinsicElements {
>IntrinsicElements : Symbol(IntrinsicElements, Decl(tsxDynamicTagName4.tsx, 2, 22))

div: any
>div : Symbol(IntrinsicElements.div, Decl(tsxDynamicTagName4.tsx, 3, 30))

h1: any
>h1 : Symbol(IntrinsicElements.h1, Decl(tsxDynamicTagName4.tsx, 4, 10))
}
}

var CustomTag: "h1" = "h1";
>CustomTag : Symbol(CustomTag, Decl(tsxDynamicTagName4.tsx, 9, 3))

<CustomTag> Hello World </CustomTag>
>CustomTag : Symbol(CustomTag, Decl(tsxDynamicTagName4.tsx, 9, 3))
>CustomTag : Symbol(CustomTag, Decl(tsxDynamicTagName4.tsx, 9, 3))

28 changes: 28 additions & 0 deletions tests/baselines/reference/tsxDynamicTagName4.types
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
=== tests/cases/conformance/jsx/tsxDynamicTagName4.tsx ===

declare module JSX {
>JSX : any

interface Element { }
>Element : Element

interface IntrinsicElements {
>IntrinsicElements : IntrinsicElements

div: any
>div : any

h1: any
>h1 : any
}
}

var CustomTag: "h1" = "h1";
>CustomTag : "h1"
>"h1" : "h1"

<CustomTag> Hello World </CustomTag>
><CustomTag> Hello World </CustomTag> : JSX.Element
>CustomTag : "h1"
>CustomTag : "h1"

41 changes: 41 additions & 0 deletions tests/baselines/reference/tsxDynamicTagName5.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
//// [tests/cases/conformance/jsx/tsxDynamicTagName5.tsx] ////

//// [react.d.ts]

declare module 'react' {
class Component<T, U> { }
}

//// [app.tsx]
import * as React from 'react';

export class Text extends React.Component<{}, {}> {
_tagName: string = 'div';

render() {
return (
<this._tagName />
);
}
}

//// [app.jsx]
"use strict";
var __extends = (this && this.__extends) || function (d, b) {
for (var p in b) if (b.hasOwnProperty(p)) d[p] = b[p];
function __() { this.constructor = d; }
d.prototype = b === null ? Object.create(b) : (__.prototype = b.prototype, new __());
};
var React = require('react');
var Text = (function (_super) {
__extends(Text, _super);
function Text() {
_super.apply(this, arguments);
this._tagName = 'div';
}
Text.prototype.render = function () {
return (<this._tagName />);
};
return Text;
}(React.Component));
exports.Text = Text;
34 changes: 34 additions & 0 deletions tests/baselines/reference/tsxDynamicTagName5.symbols
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
=== tests/cases/conformance/jsx/react.d.ts ===

declare module 'react' {
class Component<T, U> { }
>Component : Symbol(Component, Decl(react.d.ts, 1, 24))
>T : Symbol(T, Decl(react.d.ts, 2, 17))
>U : Symbol(U, Decl(react.d.ts, 2, 19))
}

=== tests/cases/conformance/jsx/app.tsx ===
import * as React from 'react';
>React : Symbol(React, Decl(app.tsx, 0, 6))

export class Text extends React.Component<{}, {}> {
>Text : Symbol(Text, Decl(app.tsx, 0, 31))
>React.Component : Symbol(React.Component, Decl(react.d.ts, 1, 24))
>React : Symbol(React, Decl(app.tsx, 0, 6))
>Component : Symbol(React.Component, Decl(react.d.ts, 1, 24))

_tagName: string = 'div';
>_tagName : Symbol(Text._tagName, Decl(app.tsx, 2, 51))

render() {
>render : Symbol(Text.render, Decl(app.tsx, 3, 27))

return (
<this._tagName />
>this._tagName : Symbol(Text._tagName, Decl(app.tsx, 2, 51))
>this : Symbol(Text, Decl(app.tsx, 0, 31))
>_tagName : Symbol(Text._tagName, Decl(app.tsx, 2, 51))

);
}
}
Loading