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(ts): parse multiline declarations correctly #12776
fix(ts): parse multiline declarations correctly #12776
Conversation
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:
|
Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/40507/ |
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 also add this test? https://www.typescriptlang.org/play?ssl=1&ssc=1&pln=3&pc=3#code/CYUwxgNghgTiAEBbA9sArhEBYAUAIgDNlk9cBvAXyA
is this going to also work with namespaces? this seem to be parsed as namespace and should not declare namespace
oo
{ } |
@armano2 yes it is parsed as a declaration, it needs to be fixed |
should i raise new ticket for this? |
It's not needed, since those are practically two instances of the same bug @fedeci can fix it in this PR if he wants. |
packages/babel-parser/test/fixtures/typescript/module-namespace/head-declare/output.json
Outdated
Show resolved
Hide resolved
return this.tsParseDeclaration(nany, value, /* next */ true); | ||
const oldState = this.state.clone(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.
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
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 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.
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.
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),
);
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.
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 = ...
.
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.
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.
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.
#12771 (comment) might be useful for this, if the goal is to handle the [no LineTerminator here].
874e203
to
953bda2
Compare
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 have a comment on the coding style, otherwise the PR looks good!
ee05451
to
aca0a8d
Compare
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 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
.
// tsc converts this to an enum declaration according to AST explorer, but all of the others require a fix :)
declare enum
e
{} |
Co-authored-by: Nicolò Ribaudo <nicolo.ribaudo@gmail.com>
267be70
to
df2f32c
Compare
df2f32c
to
f8e3472
Compare
ff7af7b
to
cb2c0ec
Compare
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 executed alignment tests between typescript and your pr, and it looks like its parsed correctly now, thanks 👍
Thanks! |
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.