Skip to content
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

fix(cli): fix stack overflow in Deno doc when namespace exports itself #599

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

MujahedSafaa
Copy link
Contributor

@MujahedSafaa MujahedSafaa commented Jun 5, 2024

This PR fixes: denoland/deno#23711

When executing deno doc on a TypeScript file that contains a namespace exporting itself, like in this file example:
https://gist.github.com/crowlKats/1f6ed084bbab0a10267df9ed31564599

export namespace test {
    export { test };
}

The program exits with the following error:
thread 'main' has overflowed its stack
error: process didn't exit successfully: target\debug\examples\ddoc.exe test3.d.ts (exit code: 0xc00000fd, STATUS_STACK_OVERFLOW)

Solution:
I added a check to see if the export is the same as the namespace and, if so, it ignores generating the DocNode for it.

@CLAassistant
Copy link

CLAassistant commented Jun 5, 2024

CLA assistant check
All committers have signed the CLA.

Copy link
Member

@dsherret dsherret left a comment

Choose a reason for hiding this comment

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

Thanks for the PR, but see the comments. Also, would you be able to sign the CLA?

if export_name == &namespace_name
{
continue;
}
Copy link
Member

Choose a reason for hiding this comment

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

What happens for the following code?

export declare namespace test {
    const test: string;
    export { test };
}

Or the following:

export declare namespace test {
    export class test {}
}

I think it can't be based on strings, but probably needs to be based on the symbol id or swc id.

Copy link
Contributor Author

@MujahedSafaa MujahedSafaa Jun 20, 2024

Choose a reason for hiding this comment

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

I previously attempted to use the symbol ID, but the output varied for certain inputs. For instance:

export function describe(name?: string, options?: TestOptions, fn?: SuiteFn): Promise<void>;
export function describe(name?: string, fn?: SuiteFn): Promise<void>;
export function describe(options?: TestOptions, fn?: SuiteFn): Promise<void>;
export function describe(fn?: SuiteFn): Promise<void>;

export namespace describe {
    /**
     * Shorthand for skipping a suite, equivalent to `describe([name], { skip: true }[, fn])`.
     */
    export function skip(name?: string, options?: TestOptions, fn?: SuiteFn): Promise<void>;
    export function skip(name?: string, fn?: SuiteFn): Promise<void>;
    export function skip(options?: TestOptions, fn?: SuiteFn): Promise<void>;
    export function skip(fn?: SuiteFn): Promise<void>;

    /**
     * Shorthand for marking a suite as `TODO`, equivalent to `describe([name], { todo: true }[, fn])`.
     */
    export function todo(name?: string, options?: TestOptions, fn?: SuiteFn): Promise<void>;
    export function todo(name?: string, fn?: SuiteFn): Promise<void>;
    export function todo(options?: TestOptions, fn?: SuiteFn): Promise<void>;
    export function todo(fn?: SuiteFn): Promise<void>;

    /**
     * Shorthand for marking a suite as `only`, equivalent to `describe([name], { only: true }[, fn])`.
     * @since v18.15.0
     */
    export function only(name?: string, options?: TestOptions, fn?: SuiteFn): Promise<void>;
    export function only(name?: string, fn?: SuiteFn): Promise<void>;
    export function only(options?: TestOptions, fn?: SuiteFn): Promise<void>;
    export function only(fn?: SuiteFn): Promise<void>;
}

The documentation is only generated for the first line: export function describe(name?: string, options?: TestOptions, fn?: SuiteFn): Promise<void>; The rest are skipped because they share the same symbol ID. I guess the symbol ID is same since they all have the same symbol and module id

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hello @dsherret
I wanted to follow up on my previous comment. Could you please check my comment above when you get a chance?

Copy link
Member

Choose a reason for hiding this comment

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

We should add a test for this in tests/specs (copy and paste another .txt file, modify it, then run cargo test to see if it works. You can use UPDATE=1 env var to update the expected output with the actual output)

@MujahedSafaa
Copy link
Contributor Author

Thanks for the PR, but see the comments. Also, would you be able to sign the CLA?

I signed the CLA document

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.

deno doc stack overflows on namespace exporting itself
4 participants