-
Notifications
You must be signed in to change notification settings - Fork 674
fix(1374): support declaration emit for expando functions #1399
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
base: main
Are you sure you want to change the base?
Conversation
698d40d
to
c063ac4
Compare
...elines/reference/submodule/compiler/declarationEmitDefaultExportWithStaticAssignment.js.diff
Outdated
Show resolved
Hide resolved
testdata/baselines/reference/submodule/compiler/declarationEmitLateBoundAssignments.js.diff
Outdated
Show resolved
Hide resolved
- var _a: boolean; | ||
- export var normal: boolean; | ||
+ var _b: boolean; |
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.
It's interesting that this temp variable turns into _b
and not _a
; it's not incorrect but I wonder if something isn't being cleared between expandos or something.
(I am reading this on my phone so have not actually read the code yet)
+export declare function foo(): void; |
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.
Maybe not for this PR but I wonder what the leftover code difference is that's adding "declare" to these.
...ata/baselines/reference/submodule/compiler/jsxDeclarationsWithEsModuleInteropNoCrash.js.diff
Outdated
Show resolved
Hide resolved
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.
This looks like a pretty straight port of the logic from strada - @sandersn should weigh in on weather he thinks that's right. I do not recall if these is one of the constructs that changed meaningfully in the js reparser rewrite and, thus, should also have it's declaration emit logic adjusted, too.
Expandos didn't fundamentally change, but they are losing a lot of their complicated features. So the core changes to emit might be right-ish, but you won't need to reintroduce any code in the parser or binder, I think. |
@sandersn Thanks for the clarification. |
internal/ast/utilities.go
Outdated
@@ -3633,3 +3637,79 @@ func GetSemanticJsxChildren(children []*JsxChild) []*JsxChild { | |||
} | |||
}) | |||
} | |||
|
|||
func IsExpandoPropertyDeclaration(node *Node) bool { |
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.
the binder already has to detect these patterns, so you shouldn't need to port anything new.
edit: your explanation of why these are needed makes sense, but much of it can be simplified given how much less Corsa supports.
internal/ast/utilities.go
Outdated
if node == nil { | ||
return false | ||
} | ||
return IsPropertyAccessExpression(node) || IsElementAccessExpression(node) || IsBinaryExpression(node) |
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.
expando declarations should only be binary expressions now.
/** @type {string} */
f.p
is no longer supported by Corsa, and access expressions don't have a Symbol field.
internal/ast/utilities.go
Outdated
return IsPropertyAccessExpression(node) || IsElementAccessExpression(node) || IsBinaryExpression(node) | ||
} | ||
|
||
func GetExpandoInitializer(initializer *Node, isPrototypeAssignment bool) *Node { |
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.
prototype assignments aren't supported in Corsa
(C.prototype = { ... }
)
internal/ast/utilities.go
Outdated
func GetEffectiveInitializer(node *Node) *Expression { | ||
if IsInJSFile(node) && node.Initializer() != nil && IsBinaryExpression(node.Initializer()) { | ||
initializer := node.Initializer().AsBinaryExpression() | ||
if initializer.OperatorToken.Kind == KindBarBarToken || initializer.OperatorToken.Kind == KindQuestionQuestionToken { |
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.
defaulting assignments with x.p = x.p || function() {}
are not supported in Corsa.
This needs a merge from main again since one of the baselines changed via another PR. |
@jakebailey Thanks for pointing it out. I’ve merged main |
@@ -14,4 +14,7 @@ | |||
export declare function f(): I; | |||
export {}; | |||
+//// [b.d.ts] |
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.
It's super odd that the original code did not emit a declaration file here, not sure why (the new code looks correct).
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.
This is still pretty semantic in how it operates - the binder is much more syntactic now, and I think we can be more syntactic in the declaration emitter now, too, because of it. Specifically, I'd like to avoid needing a GetPropertiesOfContainerFunction
emit resolver function. I'd rather we visited expression statements, looking for assignment expressions that are expando assignments (ast.GetAssignmentDeclarationKind
result of Property
- I think the js reparser even sets a .Type
on these now!), and translate them to the equivalent expansion, instead of seeing the function declaration and semantically checking for extra properties to patch on. So, for example, we see the AST for
export function foo() { return 1 }
/** @type {"ok"} */
foo.bar = "ok"
we visit export function foo() { return 1 }
and emit export function foo(): number
and then we visit
/** @type {"ok"} */
foo.bar = "ok"
and emit
export namespace foo { export const bar: "ok" }
Notably, multiple expandos would emit as multiple namespaces:
export namespace foo { export const bar: "ok" }
export namespace foo { export const baz: "yes" }
by default, and then you can have a "collection" pass at the end of a scope that merges all the newly added namespaces together (or into an explicit one, if it's there and the names all work out). The only awkward thing I can see here is that the export
modifier-ness of the namespace declaration relies on nonlocal information (if the host function is exported). This could be alleviated by never exporting a function with expando properties (which is still a nonlocal semantic check, unfortunately...) via an export
modifier, and instead adding a export { foo }
declaration - then all the declarations can always be non-exported (which is roughly how the old js declaration emitter in the node builder would have done it, and then had an extra pass to "inline" the export declarations as modifiers where possible).
@weswigham, thanks for the feedback. I've started implementing this approach and ran into a few edge cases that are worth considering:
export default function foo() {}
foo.meta = 123; We can't emit: export default function foo(): void;
export namespace foo { export const meta: 123; } As part of this, I’ve added a temporary solution using
For: function foo() {}
foo.extra = true; We cannot emit: export namespace foo { const extra: true; } At this stage, it's necessary to determine whether the host is exported, to correctly apply or omit the export modifier from the merged namespace declaration
To correctly serialize the type of an expando property and avoid self-references, declare namespace Example {
const Bar: typeof Bar;
} The type resolver typically relies on the @weswigham WDYT? |
Fixes #1374