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

feat(biome_js_analyze): noTsIgnoreComment add initial rule #2576

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

emab
Copy link
Contributor

@emab emab commented Apr 23, 2024

Summary

Implements noTsIgnoreComment rule, which prevents the use of @ts-ignore.

Test Plan

TBC

@github-actions github-actions bot added A-Project Area: project A-Linter Area: linter L-JavaScript Language: JavaScript and super languages A-Diagnostic Area: diagnostocis labels Apr 23, 2024
@emab
Copy link
Contributor Author

emab commented Apr 23, 2024

Bit of a work in progress.

Not sure if I should include an action to remove the comment, or replace with ts-expect-error.

Copy link

codspeed-hq bot commented Apr 23, 2024

CodSpeed Performance Report

Merging #2576 will not alter performance

Comparing emab:feat/noTsIgnoreComment (c884063) with main (7ea5dff)

Summary

✅ 84 untouched benchmarks

@emab
Copy link
Contributor Author

emab commented Apr 24, 2024

I'm guessing the performance hit is due to using AnyJsModuleItem?

I wasn't sure how to directly find only comment stings. Should I be using the semantic model instead?

@ematipico
Copy link
Member

ematipico commented Apr 24, 2024

I'm guessing the performance hit is due to using AnyJsModuleItem?

It's a false positive, don't worry

@ematipico
Copy link
Member

I wasn't sure how to directly find only comment stings. Should I be using the semantic model instead?

No. You only need to query the CST, comments are trivia and they have no part in the semantic model.

However, you have to widen the query. Comments can be anywhere! Technically, you'd need to query any node

@emab
Copy link
Contributor Author

emab commented Apr 24, 2024

No. You only need to query the CST, comments are trivia and they have no part in the semantic model.

Okay cool, I thought that was the case!

However, you have to widen the query. Comments can be anywhere! Technically, you'd need to query any node

Ah so right now I'll be missing comments within module items I suppose?

I was hoping I'd be able to match on a comment line, but since it's trivia I assume I'll need to search for any node and collect matching ts ignore trivia's?

@ematipico
Copy link
Member

I was hoping I'd be able to match on a comment line, but since it's trivia I assume I'll need to search for any node and collect matching ts ignore trivia's?

Yeah, that's right. There was this old initial implementation that you could use as a source of inspiration: https://github.com/rome/tools/pull/2811/files
Be mindful that the PR was closed, which means that some cases weren't caught. Also, as a piece of personal advice, I would avoid a code action for now. Working with trivia and mutations can be tricky.

@emab
Copy link
Contributor Author

emab commented Apr 24, 2024

Yeah, that's right. There was this old initial implementation that you could use as a source of inspiration: https://github.com/rome/tools/pull/2811/files Be mindful that the PR was closed, which means that some cases weren't caught. Also, as a piece of personal advice, I would avoid a code action for now. Working with trivia and mutations can be tricky.

Thanks for sharing that. I guess my question now is - are the performance issues mentioned now no longer a problem?

Also:

ability for lint rules (and visitors) to inspect syntax tokens (currently lint rules can only query AST nodes)

Am I correct in thinking we can now query syntax tokens? Should I be querying these in order to catch the edge cases? Quite new to this world so apologies if these are simple questions!

@adriencaccia
Copy link

adriencaccia commented Apr 25, 2024

I'm guessing the performance hit is due to using AnyJsModuleItem?

It's a false positive, don't worry

Hey @ematipico, we released a feature to ignore benchmarks that have a tendency to be flaky.
You can ignore the benchmark big5-added.json[uncached] on its dashboard page.

This is not ideal, but sometimes the code used in some benchmarks is by definition not deterministic, so removing them from the report is preferable to avoid noise. Ignored benchmarks are still measured at every run though, and their history can still be accessed on CodSpeed 😉
The more ignored benchmarks we have, the more data we will have to work on automatically identifying flaky code, so that in the future we will remove this kind of noise.

Let me know if that works for you!

@ematipico
Copy link
Member

Hey @adriencaccia! That's an amazing feature, thank you for implementing that, I will start ignoring flaky benchmarks now :)

@ematipico
Copy link
Member

Am I correct in thinking we can now query syntax tokens? Should I be querying these in order to catch the edge cases? Quite new to this world so apologies if these are simple questions!

You can give it a try, let's see how it goes!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Diagnostic Area: diagnostocis A-Linter Area: linter A-Project Area: project L-JavaScript Language: JavaScript and super languages
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants