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

Using references makes eslint lint compiled files instead of original typescript files #1573

Closed
sassanh opened this issue Feb 5, 2020 · 21 comments · Fixed by #1575
Closed
Labels
bug Something isn't working has pr there is a PR raised to close this package: typescript-estree Issues related to @typescript-eslint/typescript-estree

Comments

@sassanh
Copy link

sassanh commented Feb 5, 2020

What did you expect to happen?

Eslint to lint my code.

What actually happened?

Eslint lints compiled js files and reports the errors to the original typescript file. I noticed it when I saw eslint is giving errors at location that doesn't exist in the typescript file and I checked the compiled js files and noticed these errors are for the js file.

Versions

package version
@typescript-eslint/typescript-estree 2.19.0
TypeScript 3.7.4
node 12.11.1
npm 6.11.3

I traced the issue to this line https://github.com/typescript-eslint/typescript-eslint/blob/master/packages/typescript-estree/src/create-program/createIsolatedProgram.ts#L45 here the source code returned by typescript is the compiled js file, it happens when I'm using references and when the files are compiled, if I remove dist directories, eslint will start working as expected but as soon as I compile again, eslint starts giving errors of compiled js files on ts files.

@sassanh sassanh added package: typescript-estree Issues related to @typescript-eslint/typescript-estree triage Waiting for maintainers to take a look labels Feb 5, 2020
@bradzacher
Copy link
Member

Could you please provide some more information.

Could you please provide a repro?
What's your codebase look like?
What's your config look like?
What code is failing specifically?
What error messages are you seeing?

@bradzacher bradzacher added awaiting response Issues waiting for a reply from the OP or another party and removed triage Waiting for maintainers to take a look labels Feb 5, 2020
@sassanh
Copy link
Author

sassanh commented Feb 5, 2020

What error messages are you seeing?

It's not a specific error message, it has errors from all kind of different rules, so I guess the exact error message I'm getting shouldn't matter. It just lints the js file instead of the ts file and all the errors return match exactly the js file. I logged the output of the function I mentioned above (https://github.com/typescript-eslint/typescript-eslint/blob/master/packages/typescript-estree/src/create-program/createIsolatedProgram.ts#L45) and it shows me the source code of the compiled js file with the file path of the ts file in the ast returned by typescript.

What code is failing specifically?

Any code, it's a big repository and any typescript file failed because the equivalent js file is linted.

What's your config look like?

{
  "compilerOptions": {
    "allowJs": true,
    "allowSyntheticDefaultImports": true,
    "baseUrl": ".",
    "esModuleInterop": true,
    "experimentalDecorators": true,
    "forceConsistentCasingInFileNames": true,
    "incremental": true,
    "isolatedModules": true,
    "jsx": "preserve",
    "lib": [
      "dom",
      "dom.iterable",
      "esnext"
    ],
    "module": "commonjs",
    "moduleResolution": "node",
    "outDir": "./dist",
    "resolveJsonModule": true,
    "rootDir": "./src",
    "skipLibCheck": true,
    "sourceMap": true,
    "strict": true,
    "target": "ESNext"
  },
  "exclude": ["src/modules/*/dist"],
  "include": [
    "src/**/*"
  ],
  "references": [
    {
      "path": "<REFERENCE_1>"
    },
    {
      "path": "<REFERENCE_2>"
    }
  ]
}

and the referenced tsconfig.json files look like this:

{
  "extends": "../../../tsconfig.json",
  "compilerOptions": {
    "noEmit": false,
    "rootDir": "../..",
    "composite": true,
    "incremental": true,
    "outDir": "./dist"
  },
  "include": [
    "**/*"
  ]
}

I can provide a sample repository but it'll take time, please let me know if above information aren't enough and the repository is needed.

@bradzacher
Copy link
Member

What about your eslint config?
Also how are you running eslint?

@sassanh
Copy link
Author

sassanh commented Feb 5, 2020

This is my eslint config:

const process = require('process');

const typescriptOverride = {
  extends: [
    'plugin:@typescript-eslint/recommended',
    'plugin:import/errors',
    'plugin:import/warnings',
    'plugin:import/typescript',
  ],
  files: ['*.ts', '*.tsx'],
  rules: {
    '@typescript-eslint/explicit-function-return-type': 'error',
    '@typescript-eslint/explicit-member-accessibility': 'error',
    '@typescript-eslint/indent': ['error', 2],
    '@typescript-eslint/no-empty-function': 'off',
    '@typescript-eslint/no-explicit-any': 'off',
    '@typescript-eslint/no-non-null-assertion': 'off',
    '@typescript-eslint/no-unused-vars': [
      'warn',
      {ignoreRestSiblings: true, varsIgnorePattern: '^_'},
    ],
    '@typescript-eslint/no-use-before-define': 'off',
    'import/no-unresolved': 'off',
  },
};

module.exports = {
  env: {
    browser: true,
    es6: true,
  },
  extends: [
    'plugin:react/recommended',
    'plugin:jsx-a11y/strict',
  ],
  globals: {
    Atomics: 'readonly',
    SharedArrayBuffer: 'readonly',
  },
  overrides: [
    typescriptOverride,
    {
      files: ['*.js', '*.jsx'],
      parserOptions: {
        ecmaVersion: 2018,
        project: [],
        sourceType: 'commonjs',
      },
      rules: {
        'global-require': 'off',
        'no-process-exit': 'off',
        strict: 'off',
      },
    },
  ],
  parser: '@typescript-eslint/parser',
  parserOptions: {
    ecmaFeatures: {
      jsx: true,
    },
    ecmaVersion: 2018,
    project: [
        './tsconfig.json',
        './<REFERENCE_1>/tsconfig.json',
        './<REFERENCE_2>/tsconfig.json',
    ],
    sourceType: 'module',
  },
  plugins: [
    'react',
    '@typescript-eslint',
    'import',
    'sort-keys-fix',
    'jsx-a11y',
  ],
  rules: {
    'accessor-pairs': 'error',
    'array-bracket-newline': 'error',
    'array-bracket-spacing': 'error',
    'array-callback-return': 'error',
    'arrow-body-style': 'error',
    'arrow-parens': 'error',
    'arrow-spacing': 'error',
    'block-scoped-var': 'error',
    'block-spacing': 'error',
    'brace-style': ['error', 'stroustrup'],
    'callback-return': 'error',
    camelcase: 'error',
    'capitalized-comments': 'off',
    'class-methods-use-this': 'error',
    'comma-dangle': [
      'error',
      'always-multiline',
    ],
    'comma-spacing': 'error',
    'comma-style': 'error',
    complexity: ['error', 25],
    'computed-property-spacing': 'error',
    'consistent-return': 'error',
    'consistent-this': 'error',
    curly: 'error',
    'default-case': 'error',
    'dot-location': ['error', 'property'],
    'dot-notation': 'error',
    'eol-last': 'error',
    eqeqeq: 'error',
    'func-name-matching': 'error',
    'func-names': 'error',
    'generator-star-spacing': 'error',
    'global-require': 'error',
    'guard-for-in': 'error',
    'handle-callback-err': 'error',
    'id-blacklist': 'error',
    'id-length': 'error',
    'id-match': 'error',
    'implicit-arrow-linebreak': 'error',
    'import/order': [
      'error',
      {
        alphabetize: {order: 'asc'},
        groups: ['builtin', 'external', 'internal', 'parent', 'sibling', 'index'],
        'newlines-between': 'always',
      },
    ],
    'jsx-a11y/label-has-associated-control': [
      'error',
      {
        assert: 'either',
        controlComponents: ['Input'],
      },
    ],
    'jsx-a11y/label-has-for': 'off',
    'jsx-quotes': [
      'error',
      'prefer-single',
    ],
    'key-spacing': 'error',
    'keyword-spacing': 'error',
    'line-comment-position': 'error',
    'linebreak-style': [
      'error',
      'unix',
    ],
    'lines-around-directive': 'error',
    'lines-between-class-members': 'error',
    'max-classes-per-file': 'error',
    'max-depth': 'error',
    'max-len': ['error', 120],
    'max-nested-callbacks': 'error',
    'max-params': 'error',
    'max-statements': ['error', 25],
    'max-statements-per-line': 'error',
    'new-cap': 'error',
    'new-parens': 'error',
    'newline-after-var': 'error',
    'newline-before-return': 'error',
    'newline-per-chained-call': ['error', {ignoreChainWithDepth: 3}],
    'no-alert': 'error',
    'no-array-constructor': 'error',
    'no-async-promise-executor': 'error',
    'no-await-in-loop': 'error',
    'no-bitwise': 'error',
    'no-buffer-constructor': 'error',
    'no-caller': 'error',
    'no-catch-shadow': 'error',
    'no-continue': 'error',
    'no-div-regex': 'error',
    'no-duplicate-imports': 'error',
    'no-empty-function': 'off',
    'no-eq-null': 'error',
    'no-eval': 'error',
    'no-extend-native': 'error',
    'no-extra-bind': 'error',
    'no-extra-label': 'error',
    'no-floating-decimal': 'error',
    'no-implicit-coercion': 'error',
    'no-implicit-globals': 'error',
    'no-implied-eval': 'error',
    'no-inline-comments': 'off',
    'no-invalid-this': 'error',
    'no-iterator': 'error',
    'no-label-var': 'error',
    'no-labels': 'error',
    'no-lone-blocks': 'error',
    'no-lonely-if': 'error',
    'no-loop-func': 'error',
    'no-misleading-character-class': 'error',
    'no-mixed-operators': 'error',
    'no-mixed-requires': 'error',
    'no-multi-assign': 'error',
    'no-multi-spaces': 'error',
    'no-multiple-empty-lines': 'error',
    'no-native-reassign': 'error',
    'no-negated-condition': 'error',
    'no-negated-in-lhs': 'error',
    'no-new': 'error',
    'no-new-func': 'error',
    'no-new-object': 'error',
    'no-new-require': 'error',
    'no-new-wrappers': 'error',
    'no-octal-escape': 'error',
    'no-param-reassign': 'error',
    'no-path-concat': 'error',
    'no-plusplus': 'error',
    'no-process-exit': 'error',
    'no-proto': 'error',
    'no-prototype-builtins': 'error',
    'no-restricted-globals': 'error',
    'no-restricted-imports': 'error',
    'no-restricted-modules': 'error',
    'no-restricted-properties': 'error',
    'no-restricted-syntax': 'error',
    'no-return-assign': 'error',
    'no-return-await': 'error',
    'no-script-url': 'error',
    'no-self-compare': 'error',
    'no-sequences': 'error',
    'no-shadow': 'error',
    'no-shadow-restricted-names': 'error',
    'no-tabs': 'error',
    'no-template-curly-in-string': 'error',
    'no-throw-literal': 'error',
    'no-trailing-spaces': 'error',
    'no-undef-init': 'error',
    'no-undefined': 'off',
    'no-unmodified-loop-condition': 'error',
    'no-unneeded-ternary': 'error',
    'no-unused-expressions': 'error',
    'no-use-before-define': 'off',
    'no-useless-call': 'error',
    'no-useless-catch': 'error',
    'no-useless-computed-key': 'error',
    'no-useless-concat': 'error',
    'no-useless-constructor': 'error',
    'no-useless-rename': 'error',
    'no-useless-return': 'error',
    'no-var': 'error',
    'no-void': 'error',
    'no-warning-comments': process.env.CI ? 'off' : 'warn',
    'no-whitespace-before-property': 'error',
    'no-with': 'error',
    'nonblock-statement-body-position': 'error',
    'object-curly-newline': 'error',
    'object-curly-spacing': 'error',
    'object-shorthand': 'error',
    'one-var-declaration-per-line': 'error',
    'operator-assignment': 'error',
    'operator-linebreak': 'error',
    'padded-blocks': 'off',
    'padding-line-between-statements': 'error',
    'prefer-arrow-callback': 'error',
    'prefer-const': 'error',
    'prefer-destructuring': 'error',
    'prefer-numeric-literals': 'error',
    'prefer-object-spread': 'error',
    'prefer-promise-reject-errors': 'error',
    'prefer-reflect': 'error',
    'prefer-rest-params': 'error',
    'prefer-spread': 'error',
    'prefer-template': 'error',
    'quote-props': [
      'error',
      'as-needed',
    ],
    quotes: [
      'error',
      'single',
      {allowTemplateLiterals: true},
    ],
    radix: 'error',
    'react/display-name': 'off',
    'react/jsx-curly-newline': ['error', {multiline: 'forbid'}],
    'react/jsx-sort-props': [
      'error',
      {
        callbacksLast: true,
        ignoreCase: true,
        reservedFirst: true,
        shorthandLast: true,
      },
    ],
    'react/jsx-tag-spacing': [
      'error',
      {
        afterOpening: 'never',
        beforeClosing: 'never',
        beforeSelfClosing: 'always',
        closingSlash: 'never',
      },
    ],
    'react/jsx-wrap-multilines': [
      'error',
      {
        arrow: 'parens-new-line',
        assignment: 'parens-new-line',
        condition: 'ignore',
        declaration: 'parens-new-line',
        logical: 'ignore',
        prop: 'ignore',
        return: 'parens-new-line',
      },
    ],
    'react/no-array-index-key': 'error',
    'require-atomic-updates': 'error',
    'require-await': 'error',
    'require-unicode-regexp': 'error',
    'rest-spread-spacing': 'error',
    semi: 'error',
    'semi-spacing': 'error',
    'semi-style': [
      'error',
      'last',
    ],
    'sort-keys-fix/sort-keys-fix': 'error',
    'space-before-blocks': 'error',
    'space-before-function-paren': 'off',
    'space-in-parens': [
      'error',
      'never',
    ],
    'space-infix-ops': 'error',
    'space-unary-ops': 'error',
    'spaced-comment': ['error', 'always', {markers: ['/']}],
    strict: 'error',
    'switch-colon-spacing': 'error',
    'symbol-description': 'error',
    'template-curly-spacing': 'error',
    'template-tag-spacing': 'error',
    'unicode-bom': [
      'error',
      'never',
    ],
    'valid-jsdoc': 'error',
    'vars-on-top': 'error',
    'wrap-iife': 'error',
    'wrap-regex': 'error',
    'yield-star-spacing': 'error',
    yoda: 'error',
  },
  settings: {
    'import/resolver': 'babel-module',
    react: {
      version: 'detect',
    },
  },
};

@sassanh
Copy link
Author

sassanh commented Feb 5, 2020

I run eslint with node_modules/.bin/eslint and node inspect node_modules/.bin/eslint while I was debugging the issue.
Note that I tested with 2.19.0 and 2.14.0
Also note that as soon as I remove dist directories eslint starts working

@bradzacher
Copy link
Member

Looking at your eslint config, you've got parserOptions.project, which means we shouldn't ever actually hit the function you linked, as it should use the createProjectProgram path instead.


Note that we don't use project references at all. We do not resolve them ourselves (we don't even look at your tsconfig contents). The typescript APIs that we use should not resolve or use them them either (unless they've changed). The only code that should be parsed is what is matched by your include globs. Which leads me to...

You have "include": ["**/*"].
This will cause typescript to attempt to compile all files in the directory, including files in your dist output folder.
Typescript, unfortunately, provides no mechanism to automatically exclude your outDir from the include glob.
https://github.com/typescript-eslint/typescript-eslint/blob/master/docs/getting-started/linting/FAQ.md#wide-includes-in-your-tsconfig

@sassanh
Copy link
Author

sassanh commented Feb 5, 2020

I understand that it's happening in typescript and is not related to this project, so probably I should create an issue in typescript repository.
Unfortunately excluding dist directories in tsconfig.json didn't help.

@sassanh
Copy link
Author

sassanh commented Feb 5, 2020

I was able to solve the issue by narrowing include, thanks for quick responses!

@sassanh sassanh closed this as completed Feb 5, 2020
@sassanh
Copy link
Author

sassanh commented Feb 6, 2020

Actually it's not resolved, the problem is that when a file is in another typescript project referenced in tsconfig.json of the current project, typescript returns the js file if it's present in dist when calling createSourceFile in its API, I'll check if there's a way to get the typescript source file and if there isn't any I'll create an issue in typescript repository.

Looking at your eslint config, you've got parserOptions.project, which means we shouldn't ever actually hit the function you linked, as it should use the createProjectProgram path instead.

You're right, at the time I was debugging it the settings were different, now that project is set in parserOptions, it's this line that's responsible for this issue: https://github.com/typescript-eslint/typescript-eslint/blob/master/packages/typescript-estree/src/create-program/createProjectProgram.ts#L28

@sassanh
Copy link
Author

sassanh commented Feb 6, 2020

And I noticed it's not the js files, but it's .d.ts file, and it's expected considering that typescript returns .d.ts files for referenced projects.

@bradzacher
Copy link
Member

Another note is that you're using the recommended configs for eslint-plugin-import, which has some rules we do not recommend. See the above linked FAQ for more info.


Also note that our parser will parse all files covered by your tsconfigs (if you provide parserOptions.project), but it will only return the files that eslint asks for (which is the code path you highlighted).

Eslint asks for whatever files are matched by the globs you pass to the CLI that aren't matched by your eslintignore file. The only smarts eslint does is to auto ignore node_modules.

If you do something like yarn eslint ., then eslint will attempt to lint files in your build folders, unless you have your build folders listed in an ignore file.

@sassanh
Copy link
Author

sassanh commented Feb 6, 2020

Thanks for quick responses Brad. I'm running eslint on a single file currently and I do have build directories in .gitignore.

I think I found the issue, we need to pass useSourceOfProjectReferenceRedirect: true as the options of the host somehow, I'm still inspecting code in typescript, take a look at this meanwhile:
https://github.com/microsoft/TypeScript/blob/master/src/compiler/program.ts#L818
If this option is set to true, then typescript will return the ast of the original typescript file instead of the .d.ts file in referenced projects.

@sassanh
Copy link
Author

sassanh commented Feb 6, 2020

@bradzacher
Copy link
Member

I suspect that there's something else at work here. If you could provide a repro repo, I could help look into this.

We do not want to resolve project references in our parser. We maintain one ts program for each tsconfig passed in parserOptions.project.
If we ask typescript to resolve project references and use the source files for project, then we will end up using much more memory, and it would take longer to parse. A .d.ts file is much lighter weight than a source file, as it only contains type information about the exported members.

@sassanh
Copy link
Author

sassanh commented Feb 6, 2020

A .d.ts file is much lighter weight than a source file, as it only contains type information about the exported members.

Yes it's indeed lighter but it doesn't have the same ast (and even if it had the same ast it doesn't have the same location for each node in the ast) typescript-eslint needs to lint the real source file not the .d.ts file. I'll create a reproduction repository soon.

@bradzacher
Copy link
Member

bradzacher commented Feb 6, 2020

Sorry, this is what I'm trying to say though - if you are doing yarn eslint packages/foo/src/bar.ts, the following process occurs (slightly simplified, but the important steps are there):

  1. eslint resolves the input path to an absolute path: const absolutePath = /some/path/to/packages/foo/src/bar.ts.
  2. eslint reads the file contents const fileContents = fs.readFileSync(absolutePath)
  3. eslint calls our parser with absolutePath, fileContents, and parserOptions.
  4. we inspect the parser options, and if you have parserOptions.project, we use the createProjectProgram path.
    • for each tsconfig in parserOptions.project:
      a) call the typescript API ts.createWatchProgram(parserOptions.project[i]).
      - typescript parses every file referenced by the include config.
      b) if absolutePath is NOT part of the program, goto (a).
      c) return the source file for absolutePath.
  5. if no source file was returned, throw an error.
  6. convert the source file to an ESTree AST.
  7. return the AST and parser services.

This is why I'm so confused:

  • the only files that should hit the line you linked are those that ESLint has specifically asked for.
    • Unless you're asking it to parse foo.d.ts, its source file should never hit that line.
    • At no point in this process should the line you linked ever get hit for a file that is not matched by a tsconfig's include - i.e. files in dist should never get hit.
  • if we ask the typescript program for /some/path/to/packages/foo/src/bar.ts, it will never ever return the source file for /some/path/to/packages/foo/src/bar.d.ts.
  • at no point do we resolve project references. at no point does typescript ever resolve project.
    • typescript isn't resolving a project reference here, it's resolving an import, which resolves to a .d.ts file

@sassanh
Copy link
Author

sassanh commented Feb 6, 2020

  • if we ask the typescript program for /some/path/to/packages/foo/src/bar.ts, it will never ever return the source file for /some/path/to/packages/foo/src/bar.d.ts.

Based on the inspections I've done (with help of node inspect and chrome dev-tools and putting debugger on the line I mentioned) it's clear for me that when typescript-estree asks the file I gave as the input of eslint to typescript (for example I called eslint test.ts and typescript-estree asks typescript to generate the source for the absolute path of test.ts) and when dist is present (typescript files are compiled) and test.ts is inside a referenced project, typescript returns dist/test.d.ts instead of test.ts actually what typescript returns is a mix, I guess it's doing it because it doesn't want to waste time to compile the file again when it's already compiled and present in dist directory. As soon as I remove dist directory of that referenced project, typescript in the same line I linked returns the original typescript test.ts file instead of test.d.ts.

I guess I'm bothering you with this discussion without enough evidence, so please wait I find some time to provide a reproduction repository. It may be that I have an obvious mistake in my setup.

@sassanh
Copy link
Author

sassanh commented Feb 6, 2020

@bradzacher Reproduction:

git clone git@github.com:sassanh/typescript-eslint-1573-reproduction.git
cd typescript-eslint-1573-reproduction/
yarn install
node_modules/.bin/eslint reference/b.ts # It works as expected
node_modules/.bin/tsc --build
node_modules/.bin/eslint reference/b.ts # Now it doesn't work, it's linting `reference/dist/b.d.ts`
rm -rf reference/dist
node_modules/.bin/eslint reference/b.ts # It works again

@sassanh sassanh reopened this Feb 6, 2020
@sassanh
Copy link
Author

sassanh commented Feb 6, 2020

Until someone notices microsoft/TypeScript#36655 among 4K issues in that repository or we find time to provide a PR, if you can think of a workaround I'd appreciate if you share it here.

@bradzacher bradzacher added bug Something isn't working and removed awaiting response Issues waiting for a reply from the OP or another party labels Feb 6, 2020
@bradzacher
Copy link
Member

Hmm. I've spent a bit of time looking into it and this is indeed a bug

The problem is because of this block of code.
https://github.com/microsoft/TypeScript/blob/b8b59489e1040c288d6cf4593bd561d14770bbe7/src/compiler/program.ts#L2379-L2430

When the project references are nested, then refFile exists, and thus addFileToFilesByName(file, path, redirectedPath) gets called with

path = '......./b.ts'
redirectedPath = '......../b.d.ts'

Which then causes TS to set this mapping up in the internal source file cache. Then when we ask for b.ts, it gives us b.d.ts.

We check the projects in the order that you provide them in your eslint config.
If you reorder them such that the referenced configs are first, then we will use them first - so we will get the correct file from TS, and thus you won't hit this issue.


We haven't run into it because when people use project references, they have a different folder structure.

Your folder structure is this:

+ /
    + a.ts
    + tsconfig.json
    + reference/
        + b.ts
        + tsconfig.json

With /tsconfig.json referencing reference/tsconfig.json.
I've never seen this reference structure before, what we see instead is the following:

+ /packages/
    + foo/
        + a.ts
        + tsconfig.json
    + reference/
        + b.ts
        + tsconfig.json

In my testing, I made this adjustment to your repro, and it worked fine when I did this.
This seems like it's a bug in typescript itself? Why does your folder structure cause this new behaviour, but the "standard" folder structure does not?


I can submit a "fix" for this locally.
Interim workaround is to reorder your projects in your eslint config, or restructure your project.

@bradzacher bradzacher added the has pr there is a PR raised to close this label Feb 6, 2020
@sassanh
Copy link
Author

sassanh commented Feb 6, 2020

Thanks! I'm glad I can eventually lint my files, I really appreciate your quick responses and your help.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 20, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Something isn't working has pr there is a PR raised to close this package: typescript-estree Issues related to @typescript-eslint/typescript-estree
Projects
None yet
2 participants