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

typescript: Support definite assignment assertion #7159

Merged
merged 5 commits into from Feb 24, 2018

Conversation

@andy-ms
Copy link
Contributor

andy-ms commented Jan 5, 2018

Q                       A
Fixed Issues? #7157
Patch: Bug Fix? No
Major: Breaking Change? No
Minor: New Feature? Yes
Tests Added + Pass? Yes
Documentation PR
Any Dependency Changes? No
License MIT

Supports parsing the ! postfix syntax on local variables and class properties. Also supports it in babel-generator and transforms it away in babel-plugin-transform-typescript.

CC @DanielRosenwasser and @JamesHenry for review.

@babel-bot

This comment has been minimized.

Copy link
Collaborator

babel-bot commented Jan 5, 2018

Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/6889/

@@ -631,7 +634,7 @@ export type ClassMemberBase = NodeBase &
// TypeScript only:
accessibility?: ?Accessibility,
abstract?: ?true,
optional?: ?true,
optional?: ?true, //!

This comment has been minimized.

Copy link
@nicolo-ribaudo

nicolo-ribaudo Jan 5, 2018

Member

Did you forgot exclamation?

This comment has been minimized.

Copy link
@andy-ms

andy-ms Jan 5, 2018

Author Contributor

Actually that properly goes on ClassProperty only, not on any other class members. Left a comment behind though.

Copy link
Member

nicolo-ribaudo left a comment

Are these examples allowed or disallowed? Can you add tests for them?

var x!: number = 2;
var { foo }!: { foo: number } = { foo: number }
class Class {
  a?!: number;
}
@@ -836,6 +836,10 @@ defineType("VariableDeclarator", {
id: {
validate: assertNodeType("LVal"),
},
exclamation: {

This comment has been minimized.

Copy link
@nicolo-ribaudo

nicolo-ribaudo Jan 6, 2018

Member

Maybe we should call it definiteAssertion (or assertDefined) instead of exclamation? I saw that tsc uses node.exclamationToken, but it also uses node.questionToken instead of node.optional.

This comment has been minimized.

Copy link
@sgoll

sgoll Jan 12, 2018

In theory, I'd stick to the syntactic characteristics of a token to ease later semantic changes to its meaning, i.e. name it exclamation[Token] instead of definite since token definitions may later be broadened to include more than the original intentions—and the parser should add as little semantic overlay/interpretation as possible, IMHO.

However, since there is precedent for using semantic descriptions even when the TS AST uses a syntactic naming scheme (questionTokenoptional), following that same path again may be the better way with regard to internal consistency.

In any case, my feelings are not very strong either way, so I'd be fine with both.

Copy link

DanielRosenwasser left a comment

LGTM though you may want to consider the appropriate change from exclamation that @nicolo-ribaudo suggested.

Copy link
Member

JamesHenry left a comment

LGTM, would just echo Daniel and Nicolo :)

Copy link
Member

nicolo-ribaudo left a comment

I tested the examples I posted in my previous review in the TS playground and they should all fail but work with this PR.

@DanielRosenwasser

This comment has been minimized.

Copy link

DanielRosenwasser commented Jan 10, 2018

@nicolo-ribaudo the TypeScript playground isn't on 2.7. Try npm install typescript@next.

@nicolo-ribaudo

This comment has been minimized.

Copy link
Member

nicolo-ribaudo commented Jan 10, 2018

@DanielRosenwasser The don't work, even if I'm using typescript@next:

~/Documenti/ts-demo via ⬢ v9.3.0 
➜ ./node_modules/.bin/tsc --version
Version 2.7.0-dev.20180110

~/Documenti/ts-demo via ⬢ v9.3.0 
➜ cat demo.ts 
var a!: number;
var x!: number = 2;
var { foo }!: { foo: number } = { foo: number }
class Class {
  a?!: number;
}

~/Documenti/ts-demo via ⬢ v9.3.0 
➜ ./node_modules/.bin/tsc demo.ts  
demo.ts(3,12): error TS1005: ',' expected.
demo.ts(3,13): error TS1109: Expression expected.
demo.ts(3,31): error TS1128: Declaration or statement expected.
demo.ts(5,5): error TS1005: ';' expected.
demo.ts(5,6): error TS1109: Expression expected.
demo.ts(6,1): error TS1128: Declaration or statement expected.

~/Documenti/ts-demo via ⬢ v9.3.0 
➜ cat demo.js 
var a;
var x = 2;
var foo = (void 0).foo;
!;
{
    foo: number;
}
{
    foo: number;
}
var Class = /** @class */ (function () {
    function Class() {
    }
    return Class;
}());
!;
number;
@@ -1917,7 +1925,9 @@ export default (superClass: Class<Parser>): Class<Parser> =>
}

isClassProperty(): boolean {
return this.match(tt.colon) || super.isClassProperty();
return (
this.match(tt.bang) || this.match(tt.colon) || super.isClassProperty()

This comment has been minimized.

Copy link
@DanielRosenwasser

DanielRosenwasser Jan 10, 2018

Weird, how does ? get parsed from here?

This comment has been minimized.

Copy link
@nicolo-ribaudo

nicolo-ribaudo Jan 10, 2018

Member

I think that it is done in parsePostMemberNameModifiers

@@ -1704,6 +1708,10 @@ export default (superClass: Class<Parser>): Class<Parser> =>
// `let x: number;`
parseVarHead(decl: N.VariableDeclarator): void {
super.parseVarHead(decl);
if (this.eat(tt.bang)) {

This comment has been minimized.

Copy link
@DanielRosenwasser

DanielRosenwasser Jan 10, 2018

Like @nicolo-ribaudo pointed out, it probably doesn't make sense to parse a ! here unless there's a way to issue an error later.

@DanielRosenwasser

This comment has been minimized.

Copy link

DanielRosenwasser commented Jan 16, 2018

@nicolo-ribaudo I would give it another shot. Several of those should now parse correctly. I think in general, we should just have the parsing behavior match what TypeScript does (i.e. certain things still parse, but don't necessarily lead to errors). Basically, the behavior for variable declarations is just to allow a ! after identifier names (i.e. not binding patterns). @andy-ms will send an update to restrict the current behavior to only identifier declarations.

@chrisgervang

This comment has been minimized.

Copy link

chrisgervang commented Jan 31, 2018

2.7 was released today, in case typescript@next was causing issues

@@ -836,6 +836,10 @@ defineType("VariableDeclarator", {
id: {
validate: assertNodeType("LVal"),
},
definiteAssignment: {

This comment has been minimized.

Copy link
@j-f1

j-f1 Feb 11, 2018

How about definite?

@nicolo-ribaudo

This comment has been minimized.

Copy link
Member

nicolo-ribaudo commented Feb 15, 2018

var x!: number = 2; is an error in TS, but is it a parser error or a type error? In the first case, it should be disallowed by Babylon.

@andy-ms

This comment has been minimized.

Copy link
Contributor Author

andy-ms commented Feb 15, 2018

TypeScript does not really distinguish between error kinds. It's a "grammar error", meaning that parsing completes without adding any diagnostics, but the TypeChecker will add a diagnostic on the node. But that's more an implementation detail than anything.

@nicolo-ribaudo

This comment has been minimized.

Copy link
Member

nicolo-ribaudo commented Feb 15, 2018

Ok. WDYT that Babylon should do then? I think that it should throw, but I don't know TS much.

@andy-ms

This comment has been minimized.

Copy link
Contributor Author

andy-ms commented Feb 16, 2018

I don't see any benefit to throwing here -- TypeScript users already need to run tsc to get all errors, so there's never really a time when we need to throw in the parser other than for convenience of implementation.

Copy link
Member

existentialism left a comment

LGTM

@nicolo-ribaudo any remaining concerns?

@existentialism existentialism merged commit 6f3be3a into babel:master Feb 24, 2018
4 checks passed
4 checks passed
babel/repl REPL preview is available
Details
ci/circleci Your tests passed on CircleCI!
Details
codecov/project 84.16% (target 80%)
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@andy-ms andy-ms deleted the andy-ms:exclamation branch Feb 26, 2018
This was referenced Mar 14, 2018
aminmarashi-binary added a commit to aminmarashi-binary/babel that referenced this pull request Mar 17, 2018
@lock lock bot added the outdated label Oct 5, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Oct 5, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.