Fix namespace expando merge #26690
Fix namespace expando merge #26690
Conversation
Expando functions marked with JSContainer previously failed to merge with namespaces. This change adds JSContainer to ValueModuleExcludes, allowing this kind of merge.
Calls to bindPropertyAssignment now provide which special assignment kind they originated from. This allows better symbol flags to be set: 1. Property assignments get the FunctionScopedVariable flag, since they are equivalent to a `namespace` exporting a `var`. 2. Prototype property assignments get the Method flag if the initialiser is functionlike, and Property otherwise. 3. Prototype assignments get the flag Property. (3) is still not entirely correct (it's missing the Prototype flag), but is what existed previously. I'll try adding the Prototype flag to see whether it changes any baselines.
|
@weswigham good idea. I added it. I also learned that But all the other correct errors are also there. |
| } | ||
| ==== tests/cases/conformance/salsa/b.js (1 errors) ==== | ||
| ==== tests/cases/conformance/salsa/b.js (2 errors) ==== |
There was a problem hiding this comment.
This seems to be going in the wrong direction - a declaration shouldn't block an assignment to that same property
There was a problem hiding this comment.
Hm, this is a tough one. This example is obscured by the type error, so here's a type-correct one:
// filename: a.d.ts
declare namespace C {
export let x = 2
}
// filename: work.js
C.x = 3The problem is that the binder doesn't have global knowledge, so it assumes that C.bar = 3 is a declaration. I guess the merge code in the checker could selectively Javascript declarations when certain non-JS declarations are around. Problem is, I'm not sure which non-JS declarations should 'win'. Let me think about this one a bit more.
|
Small thing about the declaration change. |
| @@ -188,13 +188,13 @@ declare const ExpandoExpr: { | |||
| y: string; | |||
| x?: undefined; | |||
| }; | |||
| m(n: number): number; | |||
| m: (n: number) => number; | |||
There was a problem hiding this comment.
So these are slightly different in strict mode - the method is bivariant while the callback property is not. Know what causes the change and if that's ok?
There was a problem hiding this comment.
Yes, the change is intended: expando properties on a function shouldn't give you static methods (like they used to). They give you exports from a merged namespace. If you want bivariance you'll have to use explicit method syntax.
This is easily available in Typescript, and I don't expect to see a lot of --strict adoption in JS*, so I think this is the right approach.
- Except by people willing to rewrite their entire source a la webpack.
There was a problem hiding this comment.
But prototype assignments still get you methods (right)? Since prototype assignments are instance methods. I guess because that's expected, the expanding assignments being different feels a bit odd.
There was a problem hiding this comment.
Yeah, this is tricky. Basically, we have to choose how we'd like to model property-assignment declarations. Like class statics:
class Ex {
static m = 1
}
namespace Ex {
export var m = 2
}Or namespace merges:
namespace Mr {
export var n = 3
}
namespace Mr {
export var n = 4
}Taken on their own, I think property-assignment declarations are actually more like namespace merges. And this model allows merges of properties of the same name, which is .. nice? I guess? You definitely can't merge Ex.m in the first example.
On the other hand, classic JS often uses prototype-property- and property-assignment declarations to model classic OO:
function Class() {
}
Class.prototype.method = ...
Class.staticMethod = ...If you are around tomorrow let's take some time to discuss this in person and see if we can decide which way to go.
Also, property-assignments go back to being property declarations, not function-scoped variable declarations
| @@ -2537,12 +2541,11 @@ namespace ts { | |||
| (namespaceSymbol.members || (namespaceSymbol.members = createSymbolTable())) : | |||
| (namespaceSymbol.exports || (namespaceSymbol.exports = createSymbolTable())); | |||
|
|
|||
| // Declare the method/property | |||
| const jsContainerFlag = isToplevelNamespaceableInitializer ? SymbolFlags.JSContainer : 0; | |||
There was a problem hiding this comment.
this is just a refactoring that moves jsContainerFlag's usage into the call to declareSymbol
There was a problem hiding this comment.
Well, now it just puts JSContainer on everything regardless.
|
This change now has the right semantics, I think. But the implementation allows most merges when the other side of merge has SymbolFlags.Variable set, and this makes me nervous in the checker especially. I'm going to try adding a |
This allows most of the new special-case merge code to go away. It now uses the JSContainer special-case code, which already exists.
|
Well, the mergeFlags solution has to be 0 for special property assignments, and is the same as flags for everything else. It didn't seem like a good solution. Instead I decided to stick JSContainer on all assignment declarations. This allows the existing special-case code to work with namespaces. Well, almost all: I still had to special-case I also plan to standardise on "Assignment declaration" as the name for "JS special assignments". "JS special property expressions", "Property assignment declaration", etc. Look for another PR that just renames a bunch of stuff. @weswigham @RyanCavanaugh can you take another look at this? |
Allow JSContainers to merge with namespaces
Expando functions marked with JSContainer previously failed to merge with namespaces. This change adds JSContainer to ValueModuleExcludes, allowing this kind of merge. It also puts JSContainer on all assignment declarations, which allows the contents of a namespace to merge with the expando properties tacked onto the function.
Fixes #26672