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

Add TS support to @babel/parser's Scope #9766

Merged

Conversation

@nicolo-ribaudo
Copy link
Member

commented Mar 25, 2019

Q                       A
Fixed Issues? Fixes #9763, fixes #9480
Patch: Bug Fix? Yes
Major: Breaking Change?
Minor: New Feature?
Tests Added + Pass? Yes
Documentation PR Link
Any Dependency Changes?
License MIT
@babel-bot

This comment has been minimized.

Copy link
Collaborator

commented Mar 25, 2019

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

@nicolo-ribaudo nicolo-ribaudo marked this pull request as ready for review Mar 25, 2019

@nicolo-ribaudo nicolo-ribaudo force-pushed the nicolo-ribaudo:parser-typescript-scope-enum branch 3 times, most recently from 0f7a245 to ed7b6d6 Mar 25, 2019

const scope = this.currentScope();
scope.lexical.push(name);
} else if (bindingType === BIND_FUNCTION) {
if (

This comment has been minimized.

Copy link
@nicolo-ribaudo

nicolo-ribaudo Mar 26, 2019

Author Member

The changes in this file are mostly refactoring things in different functions, to make it possible to overwrite them.

@@ -41,12 +41,14 @@ export const BIND_NONE = 0, // Not a binding
BIND_LEXICAL = 2, // Let- or const-style binding
BIND_FUNCTION = 3, // Function declaration
BIND_SIMPLE_CATCH = 4, // Simple (identifier pattern) catch binding
BIND_OUTSIDE = 5; // Special case for function names as bound inside the function
BIND_OUTSIDE = 5, // Special case for function names as bound inside the function
BIND_TS_ENUM = 6; // TypeScript enums (block-scoped, but can be re-declared using var/function)

This comment has been minimized.

Copy link
@Jessidhia

Jessidhia Mar 26, 2019

Member

A complete fix for #9763 would also need to keep track of other typescript declarations.

Also, this is not quite how TS Enums work. enum can be declared multiple times, but only with enum or namespace. It might be useful to keep track of which types are in "type space" or "value space".

For example, type A = {}; function A() {} are completely different and unrelated -- they are two separate bindings (and both are hoisted!); they just share the same name.

Here's an attempt to explain how the declarations (are supposed to) work. The "type" part of declarations is always hoisted.

\ value type
namespace maybe yes, can hold other declarations
enum yes, let-like yes, namespace-like
class yes, let-like yes (refers to the instance)
interface no yes
type no yes
var yes no
let yes no
const yes no
function yes no

Generic arguments aren't here but they are type-like.

namespace and enum (besides an ES module itself) are the only kinds of type that can hold other types or declarations inside them. class "looks like" it would behave like that but they are interface-like instead of namespace-like -- but class can merge with a namespace, which affects the value portion class (i.e. they're part of the constructor, not the instance). class can also merge with interface, which affects the type portion of class (the instance type).

= merges
🔴 = invalid
⚪️ = no merging and no conflict; they are separate entities
⚠️ = namespaces are weird (as long as they do not contain values they don't conflict; if they do they can't merge)
❗️ = no conflict if and only if the type annotations are identical

Merging namespace enum class interface type var let const function
namespace 🔴 ⚠️
enum 🔴 🔴 🔴 🔴 🔴 🔴 🔴
class 🔴 🔴 🔴 🔴 🔴 🔴 🔴
interface 🔴 🔴 ⚪️ ⚪️ ⚪️ ⚪️
type 🔴 🔴 🔴 🔴 🔴 ⚪️ ⚪️ ⚪️ ⚪️
var 🔴 🔴 ⚪️ ⚪️ ❗️ 🔴 🔴 🔴
let 🔴 🔴 ⚪️ ⚪️ 🔴 🔴 🔴 🔴
const 🔴 🔴 ⚪️ ⚪️ 🔴 🔴 🔴 🔴
function 🔴 🔴 ⚪️ ⚪️ 🔴 🔴 🔴 *

*: multiple declarations of a function / method on the same scope are only valid if the declarations are overloads; only one of the declarations is allowed to have an implementation (the curly braces block), and it must be the last one.

type can maybe be understood as the type-only const equivalent, as it is block-scoped and it can't merge with anything or be reassigned.

This comment has been minimized.

Copy link
@nicolo-ribaudo

nicolo-ribaudo Mar 26, 2019

Author Member

Thanks you for the detailed table!
I think that we for the (var...functon)x(var...functon) cells, we should follow JavaScript's behavior, since we can't rely on type info in Babel.

As for ⚠️, I think that I won't implement redeclaration checks for them in this PR since they are only needed when transpiling namespaces to IIFEs, but Babel doesn't support doing so. It could be added later but it will be more difficult.

This comment has been minimized.

Copy link
@nicolo-ribaudo

nicolo-ribaudo Mar 26, 2019

Author Member

The "type" part of declarations is always hoisted.

Always to the block, not to the function, right?

@nicolo-ribaudo nicolo-ribaudo requested a review from Jessidhia Mar 26, 2019

@nicolo-ribaudo nicolo-ribaudo changed the title Add TS enums support to @babel/parser's Scope Add TS support to @babel/parser's Scope Mar 26, 2019

@airato

This comment has been minimized.

Copy link
Contributor

commented Apr 2, 2019

today it seems we have 3 issues with named exports in typescript:

  1. parser fails with undefined exports when we try to export TS types within named exports. this happens because TS type declarations are not counted as a declaration when checking exports (#9763)
  2. the scope for bodyless TS function declarations is getting opened, but not getting closed. This causes issues with function overloads
  3. the typescript transform does not erase named type exports.

this PR should fix 1) and maybe 2). Issue 3 is hidden today since parser fails before transform.

I have a branch attempting to fix these, but I'm doing so with existing scope module, and it's pretty dirty.
Since this PR is doing a refactor of the scope module for TS, I'll wait for this to land and then will create a PR to fix issue 3.

Here is one of the tests from my branch
https://github.com/airato/babel/blob/2b6fe025ace1038b5d2e77420f50692a0b1bd22d/packages/babel-plugin-transform-typescript/test/fixtures/declarations/erased/input.mjs

The parser should be able to parse this with no errors

@danez

This comment has been minimized.

Copy link
Member

commented Apr 3, 2019

This example now fails although it shouldn't, and may be because the SIMPLE_CATCH was removed?:

try {} catch (a) { 
  if(1) function a(){} 
}

https://tc39.github.io/ecma262/#sec-variablestatements-in-catch-blocks

Although I just noticed we already have a testcase for this which was untouched. So not sure why it is failing in the repl.

@nicolo-ribaudo

This comment has been minimized.

Copy link
Member Author

commented Apr 4, 2019

@danez

This comment has been minimized.

Copy link
Member

commented Apr 4, 2019

@nicolo-ribaudo

This comment has been minimized.

Copy link
Member Author

commented Apr 4, 2019

Because that tests doesn't run @babel/traverse, only @babel/parser.

@danez

This comment has been minimized.

Copy link
Member

commented Apr 4, 2019

🤦‍♂️I thought the error comes from the parser.

@danez
danez approved these changes Apr 4, 2019
@nicolo-ribaudo

This comment has been minimized.

Copy link
Member Author

commented Apr 4, 2019

@danez I will add a few more commits to address the link in #9766 (comment)

@nicolo-ribaudo

This comment has been minimized.

Copy link
Member Author

commented Apr 4, 2019

@airato Your test is now correctly parsed.

@danez
danez approved these changes Apr 4, 2019
@henderea

This comment has been minimized.

Copy link

commented Apr 17, 2019

Hey, is this PR going anywhere? It doesn't seem to have much activity in the past couple of weeks. What all is it waiting on?

@Jessidhia
Copy link
Member

left a comment

Looks good 👍

Sorry for taking so long to review this 🙇‍♀️


It could be good to have const enum tests as well, just in case. TypeScript requires the constness to be the same (but the error message it emits when they differ is nonsense).

): boolean {
if (scope.enums.indexOf(name) > -1) {
// Enums can be merged with other enums
return !(bindingType & BIND_FLAGS_TS_ENUM);

This comment has been minimized.

Copy link
@Jessidhia

Jessidhia Apr 22, 2019

Member

Enums can merge with namespaces too; I guess the error is avoided by not calling super.declareName for namespaces?

This comment has been minimized.

Copy link
@nicolo-ribaudo

nicolo-ribaudo Apr 26, 2019

Author Member

Yeah exactly.

Parser: Spec Compliance, Refactoring and Performance automation moved this from In progress to Reviewed Apr 22, 2019

Fix
Remove BIND_SIMPLE_CATCH
SCOPE_SIMPLE_CATCH was used instead
Fix body-less functions and namespaces
1) Move this.scope.exit() for functions from parseFunctionBody to the callers.
    Otherwise the scope of body-less functions was never closed.
    Also, it is easier to track scope.exit() if it is near to scope.enter()
2) Register namespace ids for export

@nicolo-ribaudo nicolo-ribaudo force-pushed the nicolo-ribaudo:parser-typescript-scope-enum branch from 89fc433 to be8e163 Apr 26, 2019

@nicolo-ribaudo nicolo-ribaudo merged commit 30d507c into babel:master Apr 26, 2019

0 of 2 checks passed

continuous-integration/travis-ci/pr The Travis CI build could not complete due to an error
Details
ci/circleci Your tests failed on CircleCI
Details

Parser: Spec Compliance, Refactoring and Performance automation moved this from Reviewed to Done Apr 26, 2019

@nicolo-ribaudo nicolo-ribaudo deleted the nicolo-ribaudo:parser-typescript-scope-enum branch Apr 26, 2019

Klaster1 pushed a commit to gridgain/gridgain that referenced this pull request May 8, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.