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

Extract-method with namespace declaration results in invalid code #18546

Closed
ghost opened this issue Sep 18, 2017 · 6 comments
Closed

Extract-method with namespace declaration results in invalid code #18546

ghost opened this issue Sep 18, 2017 · 6 comments
Assignees
Labels
Bug A bug in TypeScript Domain: Refactorings e.g. extract to constant or function, rename symbol

Comments

@ghost
Copy link

ghost commented Sep 18, 2017

TypeScript Version: nightly (2.6.0-dev.20170916)

See test extract-method-empty-namespace.ts.

Code

function f() {
    namespace N {}
}

Extract "namespace N" out to a global function.

Expected behavior:

function f() {
    newFunction();
}
function newFunction() {
    namespace N { }
}

Actual behavior:

function f() {
    newFunction(N);
}
function newFunction(N: any) {
    namespace N { }
}
@ghost ghost added Bug A bug in TypeScript Domain: Refactorings e.g. extract to constant or function, rename symbol labels Sep 18, 2017
@mhegazy mhegazy added this to the TypeScript 2.6 milestone Sep 18, 2017
@amcasey
Copy link
Member

amcasey commented Sep 19, 2017

I'm curious about why it's doing that but, unless I'm mistaken, that's not even legal code. Generally speaking, I don't think we make many guarantees about the behavior of refactorings and code fixes on non-compiling code.

@amcasey
Copy link
Member

amcasey commented Sep 19, 2017

If you do it with a class instead (also illegal), you get newFunction(C: typeof C). 😄

@amcasey
Copy link
Member

amcasey commented Sep 19, 2017

The same thing happens for a nested function, which is legal. I believe the issue is that rangeContainsRange is comparing the selection range to the full range of the declaration. Since the selection does not contain the full range, we assume that it was declared elsewhere and try to pass it along.

@ghost
Copy link
Author

ghost commented Sep 19, 2017

Actually, namespaces are legal to appear anywhere a declaration would be. In that case they create a local variable, not a global variable.

@amcasey
Copy link
Member

amcasey commented Sep 19, 2017

@andy-ms, that seems reasonable. Is there a switch or something I have to set to suppress/disable TS1235 (a namespace declaration is only allowed in a namespace or module)?

@ghost
Copy link
Author

ghost commented Sep 19, 2017

That is strange, I was seeing no error yesterday. Probably my services were failing but vscode doesn't tell me. I wonder why we have this restriction, since namespaces are legal inside of a module (not global) and the emit for a namespace inside a function looks fine.
Classes are legal though.

amcasey added a commit to amcasey/TypeScript that referenced this issue Sep 19, 2017
amcasey added a commit to amcasey/TypeScript that referenced this issue Sep 20, 2017
@microsoft microsoft locked and limited conversation to collaborators Jun 14, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Bug A bug in TypeScript Domain: Refactorings e.g. extract to constant or function, rename symbol
Projects
None yet
Development

No branches or pull requests

2 participants