eslint / typescript-eslint-parser

An ESLint custom parser which leverages TypeScript ESTree to allow for ESLint to lint TypeScript source code.
Other
916 stars 75 forks source link

Fix: sourceType not correctly set in AST when provided in options. #583

Closed armano2 closed 5 years ago

armano2 commented 5 years ago

Even if sourceType is provided to typescript-estree its not taken into account and its always doing "autodetection".

Typescript is not using this option anyway, there is no concept of script/module there

fixes: https://github.com/bradzacher/eslint-plugin-typescript/issues/255

j-f1 commented 5 years ago

Should this also try to guess a sourceType if one isn’t passed? It should be pretty easy to check if the source contains import or export statements. Also, if they’re present and sourceType is script, should it throw like espree does?

armano2 commented 5 years ago

guessing is build in into typescript-estree

https://github.com/JamesHenry/typescript-estree/blob/master/src/convert.ts#L511

platinumazure commented 5 years ago

Could I get some background on the difference between options.sourceType vs ast.sourceType?

armano2 commented 5 years ago

typescript-estree uses autodetection to determine what is sourceType of parsed file, this data does not come from typescript.

eslint provides sourceType field in configuration, in similar way to jsx

options.sourceType by default in eslint is set to "script" unless overridden to "module".

i intended to make this change non breaking that's why i added check for that

a lot of plugins are some core rules are using this field to determine behavior of rule:


if we want to release this change with next major update we can cut off fixes for autodetection in parse.js

                // Import/Export declarations cannot appear in script.
                // But if those appear only in namespace/module blocks, `ast.sourceType` was `"script"`.
                // This doesn't modify `ast.sourceType` directly for backward compatibility.
                case "ImportDeclaration":
                case "ExportAllDeclaration":
                case "ExportDefaultDeclaration":
                case "ExportNamedDeclaration":
                    extraOptions.sourceType = "module";
                    break;
JamesHenry commented 5 years ago

@armano2 When you say it doesn't come from TypeScript, I'm not sure exactly what you mean. We convert it from a property in the TypeScript AST like we do with many other things:

sourceType: node.externalModuleIndicator ? 'module' : 'script'

armano2 commented 5 years ago

externalModuleIndicator is internal property with array of "importable stuff" and this is not sourceType.

sourceType should be readed from ts config, but we not always have access to it,

in there there is field called module, and there can be alot of values for it:

"module": "commonjs",

that's why i'm saying this is broken for now, let say i'm compiling everything to commonjs, and eslint is complaining that i should use module...

armano2 commented 5 years ago

ok, i updated this PR with changes from typescript-estree

and i added 2nd run on scope analysis

scope analysis behave in different way for them.