Skip to content

Commit

Permalink
Fix JS merge crashes from lovefield (#27989)
Browse files Browse the repository at this point in the history
1. Merge enum with expando.
2. Merge enum member with property assignment.
3. Merge interface-declared method declaration with
prototype-property-assignment method declaration.

The reason that the enum merges crash is that getTypeOfSymbol assumes
that symbol flags are (basically) mutually exclusive. This assumption is
shredded, badly, for JS merges.

One fix is to drop the assumption of exclusivity and instead order cases
by least to most likely. This has the highest chance of working, but is
also slow, since you would prefer to order cases by most likely *first*,
not *last*.

The other fix, which is what I did here, is to add a last-chance
re-dispatch at the bottom of
`getTypeOfVariableOrParameterOrPropertyWorker`. This dispatch uses the
valueDeclaration instead of the symbol flags.
  • Loading branch information
sandersn authored Oct 19, 2018
1 parent a3c2268 commit 8779d4c
Show file tree
Hide file tree
Showing 9 changed files with 287 additions and 0 deletions.
11 changes: 11 additions & 0 deletions src/compiler/checker.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2214,6 +2214,9 @@ namespace ts {

function getDeclarationOfJSPrototypeContainer(symbol: Symbol) {
const decl = symbol.parent!.valueDeclaration;
if (!decl) {
return undefined;
}
const initializer = isAssignmentDeclaration(decl) ? getAssignedExpandoInitializer(decl) :
hasOnlyExpressionInitializer(decl) ? getDeclaredExpandoInitializer(decl) :
undefined;
Expand Down Expand Up @@ -5241,6 +5244,14 @@ namespace ts {
|| isBindingElement(declaration)) {
type = getWidenedTypeForVariableLikeDeclaration(declaration, /*includeOptionality*/ true);
}
// getTypeOfSymbol dispatches some JS merges incorrectly because their symbol flags are not mutually exclusive.
// Re-dispatch based on valueDeclaration.kind instead.
else if (isEnumDeclaration(declaration)) {
type = getTypeOfFuncClassEnumModule(symbol);
}
else if (isEnumMember(declaration)) {
type = getTypeOfEnumMember(symbol);
}
else {
return Debug.fail("Unhandled declaration kind! " + Debug.showSyntaxKind(declaration) + " for " + Debug.showSymbol(symbol));
}
Expand Down
19 changes: 19 additions & 0 deletions tests/baselines/reference/enumMergeWithExpando.errors.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
tests/cases/conformance/salsa/enums.js(2,10): error TS2540: Cannot assign to 'DESC' because it is a constant or a read-only property.
tests/cases/conformance/salsa/enums.js(3,10): error TS2540: Cannot assign to 'ASC' because it is a constant or a read-only property.


==== tests/cases/conformance/salsa/lovefield-ts.d.ts (0 errors) ====
// bug #27352, crashes from github.com/google/lovefield
declare namespace lf {
export enum Order { ASC, DESC }
}

==== tests/cases/conformance/salsa/enums.js (2 errors) ====
lf.Order = {}
lf.Order.DESC = 0;
~~~~
!!! error TS2540: Cannot assign to 'DESC' because it is a constant or a read-only property.
lf.Order.ASC = 1;
~~~
!!! error TS2540: Cannot assign to 'ASC' because it is a constant or a read-only property.

31 changes: 31 additions & 0 deletions tests/baselines/reference/enumMergeWithExpando.symbols
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
=== tests/cases/conformance/salsa/lovefield-ts.d.ts ===
// bug #27352, crashes from github.com/google/lovefield
declare namespace lf {
>lf : Symbol(lf, Decl(lovefield-ts.d.ts, 0, 0), Decl(enums.js, 0, 0), Decl(enums.js, 0, 13))

export enum Order { ASC, DESC }
>Order : Symbol(Order, Decl(lovefield-ts.d.ts, 1, 22), Decl(enums.js, 0, 0), Decl(enums.js, 1, 3))
>ASC : Symbol(Order.ASC, Decl(lovefield-ts.d.ts, 2, 23), Decl(enums.js, 1, 18))
>DESC : Symbol(Order.DESC, Decl(lovefield-ts.d.ts, 2, 28), Decl(enums.js, 0, 13))
}

=== tests/cases/conformance/salsa/enums.js ===
lf.Order = {}
>lf.Order : Symbol(lf.Order, Decl(lovefield-ts.d.ts, 1, 22), Decl(enums.js, 0, 0), Decl(enums.js, 1, 3))
>lf : Symbol(lf, Decl(lovefield-ts.d.ts, 0, 0), Decl(enums.js, 0, 0), Decl(enums.js, 0, 13))
>Order : Symbol(lf.Order, Decl(lovefield-ts.d.ts, 1, 22), Decl(enums.js, 0, 0), Decl(enums.js, 1, 3))

lf.Order.DESC = 0;
>lf.Order.DESC : Symbol(lf.Order.DESC, Decl(lovefield-ts.d.ts, 2, 28), Decl(enums.js, 0, 13))
>lf.Order : Symbol(lf.Order, Decl(lovefield-ts.d.ts, 1, 22), Decl(enums.js, 0, 0), Decl(enums.js, 1, 3))
>lf : Symbol(lf, Decl(lovefield-ts.d.ts, 0, 0), Decl(enums.js, 0, 0), Decl(enums.js, 0, 13))
>Order : Symbol(lf.Order, Decl(lovefield-ts.d.ts, 1, 22), Decl(enums.js, 0, 0), Decl(enums.js, 1, 3))
>DESC : Symbol(lf.Order.DESC, Decl(lovefield-ts.d.ts, 2, 28), Decl(enums.js, 0, 13))

lf.Order.ASC = 1;
>lf.Order.ASC : Symbol(lf.Order.ASC, Decl(lovefield-ts.d.ts, 2, 23), Decl(enums.js, 1, 18))
>lf.Order : Symbol(lf.Order, Decl(lovefield-ts.d.ts, 1, 22), Decl(enums.js, 0, 0), Decl(enums.js, 1, 3))
>lf : Symbol(lf, Decl(lovefield-ts.d.ts, 0, 0), Decl(enums.js, 0, 0), Decl(enums.js, 0, 13))
>Order : Symbol(lf.Order, Decl(lovefield-ts.d.ts, 1, 22), Decl(enums.js, 0, 0), Decl(enums.js, 1, 3))
>ASC : Symbol(lf.Order.ASC, Decl(lovefield-ts.d.ts, 2, 23), Decl(enums.js, 1, 18))

37 changes: 37 additions & 0 deletions tests/baselines/reference/enumMergeWithExpando.types
Original file line number Diff line number Diff line change
@@ -0,0 +1,37 @@
=== tests/cases/conformance/salsa/lovefield-ts.d.ts ===
// bug #27352, crashes from github.com/google/lovefield
declare namespace lf {
>lf : typeof lf

export enum Order { ASC, DESC }
>Order : Order
>ASC : Order
>DESC : Order
}

=== tests/cases/conformance/salsa/enums.js ===
lf.Order = {}
>lf.Order = {} : typeof lf.Order
>lf.Order : typeof lf.Order
>lf : typeof lf
>Order : typeof lf.Order
>{} : {}

lf.Order.DESC = 0;
>lf.Order.DESC = 0 : 0
>lf.Order.DESC : any
>lf.Order : typeof lf.Order
>lf : typeof lf
>Order : typeof lf.Order
>DESC : any
>0 : 0

lf.Order.ASC = 1;
>lf.Order.ASC = 1 : 1
>lf.Order.ASC : any
>lf.Order : typeof lf.Order
>lf : typeof lf
>Order : typeof lf.Order
>ASC : any
>1 : 1

Original file line number Diff line number Diff line change
@@ -0,0 +1,40 @@
tests/cases/conformance/salsa/lovefield-ts.d.ts(4,19): error TS2503: Cannot find namespace 'query'.
tests/cases/conformance/salsa/lovefield-ts.d.ts(5,24): error TS2503: Cannot find namespace 'schema'.
tests/cases/conformance/salsa/lovefield-ts.d.ts(7,25): error TS2503: Cannot find namespace 'query'.
tests/cases/conformance/salsa/lovefield-ts.d.ts(9,14): error TS2304: Cannot find name 'TransactionStats'.
tests/cases/conformance/salsa/lovefield.js(3,23): error TS2694: Namespace 'lf' has no exported member 'schema'.
tests/cases/conformance/salsa/lovefield.js(4,14): error TS2304: Cannot find name 'IThenable'.


==== tests/cases/conformance/salsa/lovefield-ts.d.ts (4 errors) ====
// bug #27352, crashes from github.com/google/lovefield
declare namespace lf {
export interface Transaction {
attach(query: query.Builder): Promise<Array<Object>>
~~~~~
!!! error TS2503: Cannot find namespace 'query'.
begin(scope: Array<schema.Table>): Promise<void>
~~~~~~
!!! error TS2503: Cannot find namespace 'schema'.
commit(): Promise<void>
exec(queries: Array<query.Builder>): Promise<Array<Array<Object>>>
~~~~~
!!! error TS2503: Cannot find namespace 'query'.
rollback(): Promise<void>
stats(): TransactionStats
~~~~~~~~~~~~~~~~
!!! error TS2304: Cannot find name 'TransactionStats'.
}
}
==== tests/cases/conformance/salsa/lovefield.js (2 errors) ====
lf.Transaction = function() {};
/**
* @param {!Array<!lf.schema.Table>} scope
~~~~~~
!!! error TS2694: Namespace 'lf' has no exported member 'schema'.
* @return {!IThenable}
~~~~~~~~~
!!! error TS2304: Cannot find name 'IThenable'.
*/
lf.Transaction.prototype.begin = function(scope) {};

Original file line number Diff line number Diff line change
@@ -0,0 +1,61 @@
=== tests/cases/conformance/salsa/lovefield-ts.d.ts ===
// bug #27352, crashes from github.com/google/lovefield
declare namespace lf {
>lf : Symbol(lf, Decl(lovefield-ts.d.ts, 0, 0), Decl(lovefield.js, 0, 0))

export interface Transaction {
>Transaction : Symbol(Transaction, Decl(lovefield-ts.d.ts, 1, 22), Decl(lovefield.js, 0, 0))

attach(query: query.Builder): Promise<Array<Object>>
>attach : Symbol(Transaction.attach, Decl(lovefield-ts.d.ts, 2, 32))
>query : Symbol(query, Decl(lovefield-ts.d.ts, 3, 11))
>Promise : Symbol(Promise, Decl(lib.es5.d.ts, --, --))
>Array : Symbol(Array, Decl(lib.es5.d.ts, --, --), Decl(lib.es5.d.ts, --, --))
>Object : Symbol(Object, Decl(lib.es5.d.ts, --, --), Decl(lib.es5.d.ts, --, --))

begin(scope: Array<schema.Table>): Promise<void>
>begin : Symbol(Transaction.begin, Decl(lovefield-ts.d.ts, 3, 56), Decl(lovefield.js, 0, 31))
>scope : Symbol(scope, Decl(lovefield-ts.d.ts, 4, 10))
>Array : Symbol(Array, Decl(lib.es5.d.ts, --, --), Decl(lib.es5.d.ts, --, --))
>Promise : Symbol(Promise, Decl(lib.es5.d.ts, --, --))

commit(): Promise<void>
>commit : Symbol(Transaction.commit, Decl(lovefield-ts.d.ts, 4, 52))
>Promise : Symbol(Promise, Decl(lib.es5.d.ts, --, --))

exec(queries: Array<query.Builder>): Promise<Array<Array<Object>>>
>exec : Symbol(Transaction.exec, Decl(lovefield-ts.d.ts, 5, 27))
>queries : Symbol(queries, Decl(lovefield-ts.d.ts, 6, 9))
>Array : Symbol(Array, Decl(lib.es5.d.ts, --, --), Decl(lib.es5.d.ts, --, --))
>Promise : Symbol(Promise, Decl(lib.es5.d.ts, --, --))
>Array : Symbol(Array, Decl(lib.es5.d.ts, --, --), Decl(lib.es5.d.ts, --, --))
>Array : Symbol(Array, Decl(lib.es5.d.ts, --, --), Decl(lib.es5.d.ts, --, --))
>Object : Symbol(Object, Decl(lib.es5.d.ts, --, --), Decl(lib.es5.d.ts, --, --))

rollback(): Promise<void>
>rollback : Symbol(Transaction.rollback, Decl(lovefield-ts.d.ts, 6, 70))
>Promise : Symbol(Promise, Decl(lib.es5.d.ts, --, --))

stats(): TransactionStats
>stats : Symbol(Transaction.stats, Decl(lovefield-ts.d.ts, 7, 29))
}
}
=== tests/cases/conformance/salsa/lovefield.js ===
lf.Transaction = function() {};
>lf.Transaction : Symbol(lf.Transaction, Decl(lovefield-ts.d.ts, 1, 22), Decl(lovefield.js, 0, 0))
>lf : Symbol(lf, Decl(lovefield-ts.d.ts, 0, 0), Decl(lovefield.js, 0, 0))
>Transaction : Symbol(lf.Transaction, Decl(lovefield-ts.d.ts, 1, 22), Decl(lovefield.js, 0, 0))

/**
* @param {!Array<!lf.schema.Table>} scope
* @return {!IThenable}
*/
lf.Transaction.prototype.begin = function(scope) {};
>lf.Transaction.prototype : Symbol(lf.Transaction.begin, Decl(lovefield-ts.d.ts, 3, 56), Decl(lovefield.js, 0, 31))
>lf.Transaction : Symbol(lf.Transaction, Decl(lovefield-ts.d.ts, 1, 22), Decl(lovefield.js, 0, 0))
>lf : Symbol(lf, Decl(lovefield-ts.d.ts, 0, 0), Decl(lovefield.js, 0, 0))
>Transaction : Symbol(lf.Transaction, Decl(lovefield-ts.d.ts, 1, 22), Decl(lovefield.js, 0, 0))
>prototype : Symbol(Function.prototype, Decl(lib.es5.d.ts, --, --))
>begin : Symbol(lf.Transaction.begin, Decl(lovefield-ts.d.ts, 3, 56), Decl(lovefield.js, 0, 31))
>scope : Symbol(scope, Decl(lovefield.js, 5, 42))

Original file line number Diff line number Diff line change
@@ -0,0 +1,53 @@
=== tests/cases/conformance/salsa/lovefield-ts.d.ts ===
// bug #27352, crashes from github.com/google/lovefield
declare namespace lf {
export interface Transaction {
attach(query: query.Builder): Promise<Array<Object>>
>attach : (query: any) => Promise<Object[]>
>query : any
>query : any

begin(scope: Array<schema.Table>): Promise<void>
>begin : (scope: any[]) => Promise<void>
>scope : any[]
>schema : any

commit(): Promise<void>
>commit : () => Promise<void>

exec(queries: Array<query.Builder>): Promise<Array<Array<Object>>>
>exec : (queries: any[]) => Promise<Object[][]>
>queries : any[]
>query : any

rollback(): Promise<void>
>rollback : () => Promise<void>

stats(): TransactionStats
>stats : () => any
}
}
=== tests/cases/conformance/salsa/lovefield.js ===
lf.Transaction = function() {};
>lf.Transaction = function() {} : typeof Transaction
>lf.Transaction : typeof Transaction
>lf : typeof lf
>Transaction : typeof Transaction
>function() {} : typeof Transaction

/**
* @param {!Array<!lf.schema.Table>} scope
* @return {!IThenable}
*/
lf.Transaction.prototype.begin = function(scope) {};
>lf.Transaction.prototype.begin = function(scope) {} : (scope: any[]) => any
>lf.Transaction.prototype.begin : any
>lf.Transaction.prototype : any
>lf.Transaction : typeof Transaction
>lf : typeof lf
>Transaction : typeof Transaction
>prototype : any
>begin : any
>function(scope) {} : (scope: any[]) => any
>scope : any[]

13 changes: 13 additions & 0 deletions tests/cases/conformance/salsa/enumMergeWithExpando.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
// @noEmit: true
// @allowJs: true
// @checkJs: true
// @Filename: lovefield-ts.d.ts
// bug #27352, crashes from github.com/google/lovefield
declare namespace lf {
export enum Order { ASC, DESC }
}

// @Filename: enums.js
lf.Order = {}
lf.Order.DESC = 0;
lf.Order.ASC = 1;
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
// @noEmit: true
// @allowJs: true
// @checkJs: true
// @Filename: lovefield-ts.d.ts
// bug #27352, crashes from github.com/google/lovefield
declare namespace lf {
export interface Transaction {
attach(query: query.Builder): Promise<Array<Object>>
begin(scope: Array<schema.Table>): Promise<void>
commit(): Promise<void>
exec(queries: Array<query.Builder>): Promise<Array<Array<Object>>>
rollback(): Promise<void>
stats(): TransactionStats
}
}
// @Filename: lovefield.js
lf.Transaction = function() {};
/**
* @param {!Array<!lf.schema.Table>} scope
* @return {!IThenable}
*/
lf.Transaction.prototype.begin = function(scope) {};

0 comments on commit 8779d4c

Please sign in to comment.