fix(ts): parse multiline declarations correctly

[fedeci]

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

We check that the next token does not have a previous line break (only if we want to run next()). This is necessary to avoid skipping a token that will not be parsed.

[codesandbox-ci]

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit cb2c0ec:

Sandbox	Source
babel-repl-custom-plugin	Configuration
babel-plugin-multi-config	Configuration

[babel-bot]

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

[nicolo-ribaudo]

Can you also add this test? https://www.typescriptlang.org/play?ssl=1&ssc=1&pln=3&pc=3#code/CYUwxgNghgTiAEBbA9sArhEBYAUAIgDNlk9cBvAXyA

[armano2]

is this going to also work with namespaces?

this seem to be parsed as namespace and should not

declare namespace 
oo 
{ }

https://www.typescriptlang.org/play?ssl=3&ssc=4&pln=1&pc=1#code/CYUwxgNghgTiAEA7KBbEBnADlMCCwAUAPZHyEDe8AvkA

[fedeci]

@armano2 yes it is parsed as a declaration, it needs to be fixed

[armano2]

should i raise new ticket for this?

[nicolo-ribaudo]

It's not needed, since those are practically two instances of the same bug @fedeci can fix it in this PR if he wants.

[fedeci]

Not 100% happy with this, if there was a way to check that the next token is a line terminator it would be way better.

---

edit: maybe the oldState should be saved and applied directly in tsParseDeclaration

[JLHwung]

We can clone state in tsTryParseDeclare. Since it is a try parse method, if any error is thrown in its subroutine, it should be ignored and the parser state should be restored.

[fedeci]

Isn't it already cloned in tsTryParseDeclare subroutine? Or you just mean tsTryParse should be used?

return this.tsTryParse(() =>
  this.tsParseDeclaration(nany, value, /* next */ true),
);

[JLHwung]

Although it was named tsTryParseDeclare, it does not restore state when error is thrown:

babel/packages/babel-parser/src/plugins/typescript/index.js

Lines 1487 to 1538 in 759a82a

	tsTryParseDeclare(nany: any): ?N.Declaration {
	if (this.isLineTerminator()) {
	return;
	}
	let starttype = this.state.type;
	let kind;
	if (this.isContextual("let")) {
	starttype = tt._var;
	kind = "let";
	}
	return this.tsInDeclareContext(() => {
	switch (starttype) {
	case tt._function:
	nany.declare = true;
	return this.parseFunctionStatement(
	nany,
	/* async */ false,
	/* declarationPosition */ true,
	);
	case tt._class:
	// While this is also set by tsParseExpressionStatement, we need to set it
	// before parsing the class declaration to now how to register it in the scope.
	nany.declare = true;
	return this.parseClass(
	nany,
	/* isStatement */ true,
	/* optionalId */ false,
	);
	case tt._const:
	if (this.match(tt._const) && this.isLookaheadContextual("enum")) {
	// `const enum = 0;` not allowed because "enum" is a strict mode reserved word.
	this.expect(tt._const);
	this.expectContextual("enum");
	return this.tsParseEnumDeclaration(nany, /* isConst */ true);
	}
	// falls through
	case tt._var:
	kind = kind || this.state.value;
	return this.parseVarStatement(nany, kind);
	case tt.name: {
	const value = this.state.value;
	if (value === "global") {
	return this.tsParseAmbientExternalModuleDeclaration(nany);
	} else {
	return this.tsParseDeclaration(nany, value, /* next */ true);
	}
	}
	}
	});
	}

you just mean tsTryParse should be used?

That could work, just note that tsTryParseDeclare does not equal to tryParse(() => tsParseDeclaration): The former addresses declare ... parsing, the latter handles general declaration, e.g.type X = ....

[fedeci]

Definitely, but I am not sure if the entire tsTryParseDeclare method should be wrapped with tsTryParse because it is quite expensive. For the moment I will just update the current fix which is working fine.

[nicolo-ribaudo]

#12771 (comment) might be useful for this, if the goal is to handle the [no LineTerminator here].

[nicolo-ribaudo]

I have a comment on the coding style, otherwise the PR looks good!

[JLHwung]

Can you add the following cases? All of them should throw

declare interface
i
{}

declare enum
e
{}

declare abstract
class A {
}

declare type
A = number

The idea is that the line break check should be applied universally to the keyword after declare.

[fedeci]

// tsc converts this to an enum declaration according to AST explorer, but all of the others require a fix :)
declare enum
e
{}

[armano2]

i executed alignment tests between typescript and your pr, and it looks like its parsed correctly now, thanks 👍

[nicolo-ribaudo]

Thanks!