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

Implicit function scope #2147

Closed
Wiiseguy opened this issue Sep 22, 2020 · 7 comments
Closed

Implicit function scope #2147

Wiiseguy opened this issue Sep 22, 2020 · 7 comments

Comments

@Wiiseguy
Copy link

Wiiseguy commented Sep 22, 2020

This is slightly related to #953 - the difference is I would like some code to not be visible at all. I am aware of addExtraLib and use that to inject some custom global functions and classes for IntelliSense purposes.

The use case I have is that I want a user to write a function's body only (the function declaration is not part of the model). This works fine until, for instance, the user writes something like:

let varName = 0;

If varName is already declared somewhere with the addExtraLib call, this will give a "Cannot redeclare block-scoped variable 'varName'.(2451)" error. (This won't be the case during the actual execution, but there's no way to tell monaco that!)
It's also impossible to have a return statement, as that will cause an "A 'return' statement can only be used within a function body. (1108)" error.

I would like the code to be validated as though it is in its own function scope, as is the case within a function, so this error is no longer reported.

@alexdima
Copy link
Member

Duplicate of #1661

@alexdima alexdima marked this as a duplicate of #1661 Sep 22, 2020
@spahnke
Copy link
Contributor

spahnke commented Sep 22, 2020

At least for the return statement, you can use the code sample mentioned in microsoft/monaco-typescript#46 to ignore that specific diagnostic.

@Wiiseguy
Copy link
Author

@alexdima I get this issue was closed, but I think #1661 is too broad of a fix and not really a duplicate of this. I was thinking more of something like a DiagnosticsOptions flag like implicitFunctionScope: true. I should have been more clear in my first post.

@spahnke Thanks. The same principle could be used to disable 2451 as well.

@spahnke
Copy link
Contributor

spahnke commented Sep 22, 2020

If varName is already declared somewhere with the addExtraLib call, this will give a "Cannot redeclare block-scoped variable 'varName'.(2451)" error.

You can work around that pretty easily by only including necessary types in your extra lib instead of the actual code you wrap the user's code in. Consider the following function with parameters and a predefined global object foo with one method bar() that returns a number:

function implicitFunction(a: number, b: number) {
    // user code goes here
    return a + b + foo.bar();
}

Then you want your users to just write

return a + b + foo.bar();

into the editor and get completion for everything, as far as I understand. The return statements normally gives you an error which you can suppress by the method mentioned earlier. So you only need to supply a, b and foo in your extra lib and can use a .d.ts file for that:

interface Foo {
    bar(): number;
}

// global object
declare const foo: Foo;

// parameters - declare as var if you want the user to be able to change these, otherwise use const
declare var a: number;
declare var b: number;

This is how we use the editor for this exact use case. Provide the necessary types by a .d.ts file in the extra lib and suppress the error for the return statement. Since you only provide the necessary types there is no chance of name clashes and you probably don't want the user to be able to overwrite the global objects anyway. This is why you declare the global object as const so the user gets an error if they overwrite/redeclare it, and use var for parameters if you want the user to be able to overwrite those.

Does this help you?

Full playground example adopted from https://microsoft.github.io/monaco-editor/playground.html#extending-language-services-configure-javascript-defaults:

// validation settings
monaco.languages.typescript.javascriptDefaults.setDiagnosticsOptions({
    noSemanticValidation: false,
    noSyntaxValidation: false,
    noSuggestionDiagnostics: false,
    diagnosticCodesToIgnore: [1108],
});

// compiler options
monaco.languages.typescript.javascriptDefaults.setCompilerOptions({
    target: monaco.languages.typescript.ScriptTarget.ES6,
    allowNonTsExtensions: true
});

// extra libraries
const libSource = `
interface Foo {
    bar(): number;
}

// global object
declare const foo: Foo;

// parameters
declare var a: number;
declare var b: number;
`
const libUri = 'ts:filename/facts.d.ts';
monaco.languages.typescript.javascriptDefaults.addExtraLib(libSource, libUri);
// When resolving definitions and references, the editor will try to use created models.
// Creating a model for the library allows "peek definition/references" commands to work with the library.
monaco.editor.createModel(libSource, 'typescript', monaco.Uri.parse(libUri));

const jsCode = `return a + b + foo.bar();`

monaco.editor.create(document.getElementById('container'), {
    value: jsCode,
    language: 'javascript'
});

@Wiiseguy
Copy link
Author

Thank you for your reply.

Sadly, it does not address the original problem, though. If you type let foo = 3; in the right-hand panel it will still give the 2451 error on 'foo'.

Surpressing the 2451 error in addition to 1108 seems like the only viable workaround for now:

monaco.languages.typescript.javascriptDefaults.setDiagnosticsOptions({
    noSemanticValidation: false,
    noSyntaxValidation: false,
    noSuggestionDiagnostics: false,
    diagnosticCodesToIgnore: [1108, 2451],
});

@spahnke
Copy link
Contributor

spahnke commented Sep 22, 2020

Of course I can only speak for our application, but if we declare a global variable we would never ever want the user to be able to overwrite those; even in a function scope. Because that only leads to subtle bugs I don't want to hunt down 😄 So the questions that you need to answer for yourself are: If the global variable is essential and should be used inside the function, do you really want the user to overwrite that; even temporarily? And, if it's not used by your users inside your function, do you need to declare it in the first place? If you really want to have both, then you're right TypeScript is a bit too strict for your needs. And even an option like implicitFunctionScope would not solve this problem because foo still would be declared globally. The only thing that would work other than suppressing the error, is declaring foo as var on your end and have the user also have it declare as var.

But also consider that you probably will need to debug the following code for a user at some point in the future if you just suppress the error or use var, because they file the issue that the method bar is not working.

// ... 20 lines of code
var foo = "asdf"; // sneaky redeclaration
// ... another 50 lines of code
return foo.bar(); // this will not work now

Or even worse they write the code

let test = 1;
// ... other code
let test = 2;

and now the editor says everything is fine, but the script crashes at runtime because now they're actually redeclaring block scoped variables which is not allowed.

I myself would always go for the stricter error checking, or just don't declare foo if it's not necessary. But again, this boils down to your needs. Good luck!

@Wiiseguy
Copy link
Author

You're right that it sounds a bit strange.

To explain my use case a bit more, users of our application can create all kinds of identifiers through a web UI that translate in code to classes/constants/functions.

A user had one of those classes named result and would later add a function with a let result = 0; statement in its body (as one does). Of course, this is perfectly valid if the code was in an actual function, you can just redeclare variables if you want. But as the extraLib and the user code resides in the same block-scope, this would hint to the user that they couldn't use result. The code executes fine when tested, so there was a discrepancy.

You could say that they shouldn't use the name 'result' as a class, but it's how they happened to compose their model. 🤷‍♂️

Anyway, thanks again for the tips!

@vscodebot vscodebot bot locked and limited conversation to collaborators Nov 6, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

3 participants