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(typescript-estree): don't consider a cached program unless it's specified in the current parserOptions.project config #5999

Merged
merged 3 commits into from Nov 16, 2022

Conversation

bradzacher
Copy link
Member

PR Checklist

Overview

Discovered this while working on #5916

Previously we would consider any cached program as fair game during parsing.
Most of the time this isn't a problem because users always have the same config for an entire lint run.

However if the user has a different value for parserOptions.project around the place, then we would behave incorrectly - we would iterate through our cached programs and return a matching program for a file even if its tsconfig wasn't included in the current parserOptions.project! This is really bad because it means we'll lint a file with probably incorrect types!!!

I discovered this in the above PR because I defined two separate test runs with different tsconfigs in the same file.
Because our rule tester uses the exact same filename, the parser checked the cache, found the old program matched, and returned it - meaning we never used the new tsconfig (causing the test to fail!)

The fix is simply to enforce that we only consider a program from the cache if and only if its tsconfig exists in the current parserOptions.project set.

…pecified in the current `parserOptions.project` config
@bradzacher bradzacher added the bug Something isn't working label Nov 16, 2022
@typescript-eslint
Copy link
Contributor

Thanks for the PR, @bradzacher!

typescript-eslint is a 100% community driven project, and we are incredibly grateful that you are contributing to that community.

The core maintainers work on this in their personal time, so please understand that it may not be possible for them to review your work immediately.

Thanks again!


🙏 Please, if you or your company is finding typescript-eslint valuable, help us sustain the project by sponsoring it transparently on https://opencollective.com/typescript-eslint.

@nx-cloud
Copy link

nx-cloud bot commented Nov 16, 2022

☁️ Nx Cloud Report

CI is running/has finished running commands for commit ba08e6c. As they complete they will appear below. Click to see the status, the terminal output, and the build insights.

📂 See all runs for this branch


Successfully ran 46 targets

Sent with 💌 from NxCloud.

@netlify
Copy link

netlify bot commented Nov 16, 2022

Deploy Preview for typescript-eslint ready!

Name Link
🔨 Latest commit ba08e6c
🔍 Latest deploy log https://app.netlify.com/sites/typescript-eslint/deploys/6374fd1b2c8e4d0008873919
😎 Deploy Preview https://deploy-preview-5999--typescript-eslint.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@@ -1,14 +1,11 @@
import path from 'path';
Copy link
Member Author

Choose a reason for hiding this comment

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

the changes here were extracted from #5916 as an easy test for this behaviour.
could add a regression test against this, but I didn't bother because ultimately it's such a rare case.

@codecov
Copy link

codecov bot commented Nov 16, 2022

Codecov Report

Merging #5999 (ba08e6c) into main (49f623f) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #5999   +/-   ##
=======================================
  Coverage   91.42%   91.42%           
=======================================
  Files         365      365           
  Lines       12339    12342    +3     
  Branches     3609     3610    +1     
=======================================
+ Hits        11281    11284    +3     
  Misses        753      753           
  Partials      305      305           
Flag Coverage Δ
unittest 91.42% <100.00%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...pt-estree/src/create-program/createWatchProgram.ts 78.39% <100.00%> (+0.33%) ⬆️

Copy link
Member

@JoshuaKGoldberg JoshuaKGoldberg left a comment

Choose a reason for hiding this comment

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

Nice find!

// so we don't want to consider it for caching purposes.
//
// if we did consider it we might return a program for a project
// that wasn't specified in the current parser run (which is obv bad!).
Copy link
Member

Choose a reason for hiding this comment

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

To you, maybe 😛

Suggested change
// that wasn't specified in the current parser run (which is obv bad!).
// that wasn't specified in the current parser run (which is bad!).

@bradzacher bradzacher merged commit 530e0e6 into main Nov 16, 2022
40 checks passed
@bradzacher bradzacher deleted the fix-using-cached-program-incorrectly branch November 16, 2022 19:44
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 24, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants