Skip to content

Remove Arguments from jsdoc#24583

Merged
sbc100 merged 1 commit intoemscripten-core:mainfrom
sbc100:fix_jsdocs
Jun 17, 2025
Merged

Remove Arguments from jsdoc#24583
sbc100 merged 1 commit intoemscripten-core:mainfrom
sbc100:fix_jsdocs

Conversation

@sbc100
Copy link
Collaborator

@sbc100 sbc100 commented Jun 16, 2025

Looks like this was originally added in #10525.

Perhaps closure compiler used to have a distinct type for arguments builtin in JS? I can't see any mention of it in closure compiler documentation today though.

Fixes: #24579

Looks like this was originally added in emscripten-core#10525.

Perhaps closure compiler used to have a distinct type for `arguments`
builtin in JS?  I can't see any mention of it in closure compiler
documentation today though.

Fixes: emscripten-core#24579
@sbc100 sbc100 requested review from dschuff and juj June 16, 2025 21:19
@RReverser
Copy link
Collaborator

Perhaps closure compiler used to have a distinct type for arguments builtin in JS?

It would make sense, since arguments object is historically special and isn't actually an array (eg it lacks all the array methods, and you would want typechecker like Closure to catch you using them by accident).

@RReverser
Copy link
Collaborator

I can't see any mention of it in closure compiler documentation today though.

Not documentation, but here's the declaration of extern: https://github.com/google/closure-compiler/blob/71975c675e458b9bfed8fe0e737d388a82b71918/externs/es3.js#L230

@sbc100 sbc100 requested a review from RReverser June 16, 2025 21:33
@sbc100
Copy link
Collaborator Author

sbc100 commented Jun 16, 2025

Perhaps closure compiler used to have a distinct type for arguments builtin in JS?

It would make sense, since arguments object is historically special and isn't actually an array (eg it lacks all the array methods, and you would want typechecker like Closure to catch you using them by accident).

But we don't use arguments any more in the code base right? So this should no longer be needed. Especially since it seems to confuse TSC.

@RReverser
Copy link
Collaborator

But we don't use arguments any more in the code base right?

I don't know, haven't checked, just saw the PR description and wanted to clarify they're not the same. If it's not used, go for it of course.

@sbc100
Copy link
Collaborator Author

sbc100 commented Jun 16, 2025

But we don't use arguments any more in the code base right?

I don't know, haven't checked, just saw the PR description and wanted to clarify they're not the same. If it's not used, go for it of course.

I'm pretty sure it was needed for the cwrap implemention which was converted to the rest operator in #21331

@sbc100 sbc100 enabled auto-merge (squash) June 16, 2025 21:47
Copy link
Collaborator

@juj juj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@sbc100 sbc100 merged commit 39d93be into emscripten-core:main Jun 17, 2025
30 checks passed
@sbc100 sbc100 deleted the fix_jsdocs branch June 17, 2025 10:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

🐛 The --emit-tsd option generated TypeScript type for the args parameter of the ccall function should use IArguments instead of Arguments

3 participants