emit @implements on 'implements' of type+value #1073
Conversation
If tsickle gets input that contains a type-value conflict, e.g.
interface X { ... }
const X = 3;
then it drops the 'interface' declaration in the output because
at the Closure level, the definition of "X" clobbers the @record.
Because of this, if someone later attempts to *use* the interface, e.g.
class C implements X {}
tsickle would silently drop the '@implements' of that interface in
the Closure output.
But dropping @implements is dangerous in the presence of typed
optimzations; and this actually caused a real misoptimization in a
Google app.
The reason was that you actually can have legitimate type+value
interfaces in Closure:
/** @interface */ class I { ... }
I.someValue = 3;
and Clutz then dutifully converts this to the matching TypeScript,
and then if someone implements that interface, dropping the @implements
can cause a misoptimization.
In #1072 I convinced myself that we should instead never drop any
'implements' clause. If the user writes the above pattern with X
and C, we should just compile fail instead, because there is no
way we can safely emit it.
In the test case here I added some usage of 'export import', but it
turns out that was irrelevant to this bug; it's just that when Clutz
started using 'export import' it got better at correctly describing
the above 'class I' pattern, which cascaded into this tsickle failure.
The clutz 'export import' change is
angular/clutz@cc9c9e6
| @@ -0,0 +1,17 @@ | |||
| /** | |||
There was a problem hiding this comment.
nit: I usually name these export_import/export_import.ts so that we don't end up with a million files called test.ts. The repetition sucks too, not sure if it's substantially better...
| // Simple implementation of a type+value that is indirected through an | ||
| // 'export import' clause. | ||
| // We expect an @implements to show in the output. | ||
| class C2 implements export_import.Reexport.Reexported { |
There was a problem hiding this comment.
are you going to remove the export import stuff here before submission? It's distracting from what the test really tests, isn't it?
| @@ -90,6 +90,10 @@ export function maybeAddHeritageClauses( | |||
| type: heritage.parentName, | |||
| }); | |||
| } | |||
| // In the heritage==null case, we silently have dropped an @implements. | |||
| // In issue 1072 I advocate that we should never drop an @implements here, | |||
There was a problem hiding this comment.
I don't understand this comment. It sounds as if you advocate something, but then didn't act on it? Is that a follow up?
The way I read the code it suggests that we below fall through and do emit?
| @@ -0,0 +1,18 @@ | |||
| /** | |||
| * @fileoverview This file vaguely resembles the shape of the clutz output of | |||
There was a problem hiding this comment.
Can you narrow this down to a test that just creates the type/value conflict?
| // Because Zone is both a type and a value, the interface will be dropped | ||
| // when converting to Closure, so the "implements" should be ignored for | ||
| // both the direct use and the use via a typedef. | ||
| interface Zone { |
There was a problem hiding this comment.
You're removing coverage here. Can we keep a test that shows what happens when we have a type/value conflict in a regular .ts file?
|
I think we should keep a test and show what happens for a type/value conflict in |
|
Abandoning this in favor of ef80516 |
If tsickle gets input that contains a type-value conflict, e.g.
interface X { ... }
const X = 3;
then it drops the 'interface' declaration in the output because
at the Closure level, the definition of "X" clobbers the @record.
Because of this, if someone later attempts to use the interface, e.g.
class C implements X {}
tsickle would silently drop the '@implements' of that interface in
the Closure output.
But dropping @implements is dangerous in the presence of typed
optimzations; and this actually caused a real misoptimization in a
Google app.
The reason was that you actually can have legitimate type+value
interfaces in Closure:
/** @interface */ class I { ... }
I.someValue = 3;
and Clutz then dutifully converts this to the matching TypeScript,
and then if someone implements that interface, dropping the @implements
can cause a misoptimization.
In #1072 I convinced myself that we should instead never drop any
'implements' clause. If the user writes the above pattern with X
and C, we should just compile fail instead, because there is no
way we can safely emit it.
In the test case here I added some usage of 'export import', but it
turns out that was irrelevant to this bug; it's just that when Clutz
started using 'export import' it got better at correctly describing
the above 'class I' pattern, which cascaded into this tsickle failure.
The clutz 'export import' change is
angular/clutz@cc9c9e6