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(eslint-plugin): [no-extra-semi] false negatives when used with eslint 8.3.0 #4458

Merged
merged 3 commits into from Jan 17, 2022

Conversation

ota-meshi
Copy link
Contributor

PR Checklist

Overview

This PR modifies to call the changed method name of the no-extra-semi core rule.

This issue was found while working on #4448.
#4448 (comment)

@typescript-eslint
Copy link
Contributor

Thanks for the PR, @ota-meshi!

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. As a thank you, your profile/company logo will be added to our main README which receives thousands of unique visitors per day.

@nx-cloud
Copy link

nx-cloud bot commented Jan 17, 2022

☁️ Nx Cloud Report

CI ran the following commands for commit 69311fd. Click to see the status, the terminal output, and the build insights.

📂 See all runs for this branch


✅ Successfully ran 48 targets

Sent with 💌 from NxCloud.

@netlify
Copy link

netlify bot commented Jan 17, 2022

✔️ Deploy Preview for typescript-eslint ready!

🔨 Explore the source changes: 69311fd

🔍 Inspect the deploy log: https://app.netlify.com/sites/typescript-eslint/deploys/61e52fb5f3988300081094d3

😎 Browse the preview: https://deploy-preview-4458--typescript-eslint.netlify.app

@ota-meshi ota-meshi marked this pull request as draft January 17, 2022 08:56
@bradzacher bradzacher added the bug Something isn't working label Jan 17, 2022
@ota-meshi
Copy link
Contributor Author

I need to include the #4448 change to fix the CI errors.

@ota-meshi ota-meshi marked this pull request as ready for review January 17, 2022 09:01
@ota-meshi
Copy link
Contributor Author

I merged the main branch into this PR.

@WikiRik
Copy link

WikiRik commented Jan 17, 2022

Will you enable the codepath additions from #4448 in here? Or will that happen in a third PR?

@codecov
Copy link

codecov bot commented Jan 17, 2022

Codecov Report

Merging #4458 (69311fd) into main (e56f1e5) will decrease coverage by 2.00%.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##             main    #4458      +/-   ##
==========================================
- Coverage   94.38%   92.38%   -2.01%     
==========================================
  Files         154      346     +192     
  Lines        8107    11633    +3526     
  Branches     2583     3308     +725     
==========================================
+ Hits         7652    10747    +3095     
- Misses        255      619     +364     
- Partials      200      267      +67     
Flag Coverage Δ
unittest 92.38% <0.00%> (-2.01%) ⬇️

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

Impacted Files Coverage Δ
packages/eslint-plugin/src/rules/no-extra-semi.ts 54.54% <0.00%> (-12.13%) ⬇️
...src/definition/ImplicitGlobalVariableDefinition.ts 100.00% <0.00%> (ø)
packages/scope-manager/src/lib/es2019.object.ts 100.00% <0.00%> (ø)
packages/typescript-estree/src/version-check.ts 100.00% <0.00%> (ø)
...kages/scope-manager/src/referencer/ClassVisitor.ts 93.96% <0.00%> (ø)
packages/scope-manager/src/lib/webworker.ts 100.00% <0.00%> (ø)
packages/scope-manager/src/lib/es2021.weakref.ts 100.00% <0.00%> (ø)
...ges/scope-manager/src/definition/DefinitionType.ts 100.00% <0.00%> (ø)
packages/scope-manager/src/variable/index.ts 100.00% <0.00%> (ø)
packages/type-utils/src/getSourceFileOfNode.ts 0.00% <0.00%> (ø)
... and 183 more

@ota-meshi
Copy link
Contributor Author

Will you enable the codepath additions from #4448 in here? Or will that happen in a third PR?

I will not add it in this PR. I think it needs a feature request.

@WikiRik
Copy link

WikiRik commented Jan 17, 2022

So we need a feature request for the code you have added in the previous PR but commented out because you wanted to update the ESLint version here?

I think I might not have been clear enough in my previous message. I am just talking about un-commenting lines 714 and 715 of the eslint-rules typings file. Or is that not a working solution for #4444?

@ota-meshi
Copy link
Contributor Author

For the same reason as #4448, I find it difficult to prevent loss of coverage for this PR.

@ota-meshi
Copy link
Contributor Author

@WikiRik
Code Path Analysis has long existed as an API for ESLint.
https://eslint.org/docs/developer-guide/code-path-analysis

However, the rules provided by typescript-eslint do not currently have rules that use the API (directly), so no type definition is needed either.
For now the @typescript-eslint/no-invalid-this rule doesn't even have to use it directly.

If you want to create a rule that uses Code Path Analysis and want to use the type definition provided by typescript-eslint, you'll need to request a feature.

@WikiRik
Copy link

WikiRik commented Jan 17, 2022

Ah, so if I understand correctly it's not part of the solution for #4444. Then it's all good to me. Thanks for your work on these issues!

@bradzacher
Copy link
Member

@WikiRik we don't need to worry about doing anything to the extension rule. The new code-path analysis selectors do not change the overrides we need to apply.

If it did - then the tests would be failing, which they are not.

In a nuttshell - previously the rule used explicit node selectors on things like functions to trigger the analysis. Our extension rule didn't touch that logic - it just augmented it so it could track this parameters and prevent the rule's ThisExpression selector appropriately.

Now the base rule uses the codepath selectors instead, but the overall logic doesn't change, thus nothing to change on our end aside from making sure we don't call undefined functions.

@bradzacher bradzacher merged commit f4016c2 into typescript-eslint:main Jan 17, 2022
@ota-meshi ota-meshi deleted the no-extra-semi branch January 17, 2022 10:56
lonyele pushed a commit to lonyele/typescript-eslint that referenced this pull request Feb 12, 2022
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 17, 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

3 participants