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.

Sign up for GitHub

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

Jump to bottom

Throw a syntax error for a constructor with type parameters #12065

Merged
merged 3 commits into from Sep 17, 2020
Merged

Throw a syntax error for a constructor with type parameters #12065

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
969ad50
Throw a syntax error for a constructor with type parameters
sosukesuzuki Sep 16, 2020
952ef30
Modify to match error location with TypeScript
sosukesuzuki Sep 16, 2020
7a02112
Update typescript parser tests
sosukesuzuki Sep 17, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
5 changes: 5 additions & 0 deletions packages/babel-parser/src/plugins/typescript/index.js
View file
Open in desktop
Expand Up @@ -65,6 +65,8 @@ type ParsingContext =
const TSErrors = Object.freeze({
ClassMethodHasDeclare: "Class methods cannot have the 'declare' modifier",
ClassMethodHasReadonly: "Class methods cannot have the 'readonly' modifier",
ConstructorHasTypeParameters:
"Type parameters cannot appear on a constructor declaration.",
DeclareClassFieldHasInitializer:
"'declare' class fields cannot have an initializer",
DuplicateModifier: "Duplicate modifier: '%0'",
Expand Down Expand Up @@ -2291,6 +2293,9 @@ export default (superClass: Class<Parser>): Class<Parser> =>
allowsDirectSuper: boolean,
): void {
const typeParameters = this.tsTryParseTypeParameters();
if (typeParameters && isConstructor) {
this.raise(typeParameters.start, TSErrors.ConstructorHasTypeParameters);
}
if (typeParameters) method.typeParameters = typeParameters;
super.pushClassMethod(
classBody,
Expand Down
3 changes: 3 additions & 0 deletions ...ges/babel-parser/test/fixtures/typescript/class/constructor-with-type-parameters/input.ts
View file
Open in desktop
@@ -0,0 +1,3 @@
class C {
constructor<T>(foo: T) {}
}
84 changes: 84 additions & 0 deletions .../babel-parser/test/fixtures/typescript/class/constructor-with-type-parameters/output.json
View file
Open in desktop
@@ -0,0 +1,84 @@
{
"type": "File",
"start":0,"end":39,"loc":{"start":{"line":1,"column":0},"end":{"line":3,"column":1}},
"errors": [
"SyntaxError: Type parameters cannot appear on a constructor declaration. (2:13)"
],
"program": {
"type": "Program",
"start":0,"end":39,"loc":{"start":{"line":1,"column":0},"end":{"line":3,"column":1}},
"sourceType": "module",
"interpreter": null,
"body": [
{
"type": "ClassDeclaration",
"start":0,"end":39,"loc":{"start":{"line":1,"column":0},"end":{"line":3,"column":1}},
"id": {
"type": "Identifier",
"start":6,"end":7,"loc":{"start":{"line":1,"column":6},"end":{"line":1,"column":7},"identifierName":"C"},
"name": "C"
},
"superClass": null,
"body": {
"type": "ClassBody",
"start":8,"end":39,"loc":{"start":{"line":1,"column":8},"end":{"line":3,"column":1}},
"body": [
{
"type": "ClassMethod",
"start":12,"end":37,"loc":{"start":{"line":2,"column":2},"end":{"line":2,"column":27}},
"static": false,
"key": {
"type": "Identifier",
"start":12,"end":23,"loc":{"start":{"line":2,"column":2},"end":{"line":2,"column":13},"identifierName":"constructor"},
"name": "constructor"
},
"computed": false,
"kind": "constructor",
"typeParameters": {
"type": "TSTypeParameterDeclaration",
"start":23,"end":26,"loc":{"start":{"line":2,"column":13},"end":{"line":2,"column":16}},
"params": [
{
"type": "TSTypeParameter",
"start":24,"end":25,"loc":{"start":{"line":2,"column":14},"end":{"line":2,"column":15}},
"name": "T"
}
]
},
"id": null,
"generator": false,
"async": false,
"params": [
{
"type": "Identifier",
"start":27,"end":33,"loc":{"start":{"line":2,"column":17},"end":{"line":2,"column":23},"identifierName":"foo"},
"name": "foo",
"typeAnnotation": {
"type": "TSTypeAnnotation",
"start":30,"end":33,"loc":{"start":{"line":2,"column":20},"end":{"line":2,"column":23}},
"typeAnnotation": {
"type": "TSTypeReference",
"start":32,"end":33,"loc":{"start":{"line":2,"column":22},"end":{"line":2,"column":23}},
"typeName": {
"type": "Identifier",
"start":32,"end":33,"loc":{"start":{"line":2,"column":22},"end":{"line":2,"column":23},"identifierName":"T"},
"name": "T"
}
}
}
}
],
"body": {
"type": "BlockStatement",
"start":35,"end":37,"loc":{"start":{"line":2,"column":25},"end":{"line":2,"column":27}},
"body": [],
"directives": []
}
}
]
}
}
],
"directives": []
}
}
1 change: 1 addition & 0 deletions scripts/parser-tests/typescript/allowlist.txt
View file
Open in desktop
Expand Up @@ -420,3 +420,4 @@ withStatement.ts
withStatementErrors.ts
withStatementInternalComments.ts
withStatementNestedScope.ts
parserConstructorDeclaration12.ts
Copy link
Member Author

Choose a reason for hiding this comment

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

https://github.com/microsoft/TypeScript/blob/master/tests/cases/compiler/parserConstructorDeclaration12.ts

class C {
  constructor<>() { }
  constructor<> () { }
  constructor <>() { }
  constructor <> () { }
  constructor< >() { }
  constructor< > () { }
  constructor < >() { }
  constructor < > () { }
}

Copy link
Member

Choose a reason for hiding this comment

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

Can you update this line:

babel/Makefile

Line 3 in bbe0cf0

TYPESCRIPT_COMMIT = ffa35d3272647fe48ddf173e1f0928f772c18630

to update the TS tests?
And then run make test-typescript-update-allowlist

Copy link
Member Author

Choose a reason for hiding this comment

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

I've fixed at 7a02112! But I'm not familiar with how babel-parser tests work, so I'm not sure if it's correct.

Copy link
Member

Choose a reason for hiding this comment

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

It's correct! Btw, allowlist contain tests that we don't pass and not tests that we are correctly handling.