Skip to content

feat(linter): add use_cfg attribute.#3744

Closed
rzvxa wants to merge 4 commits intomainfrom
06-18-feat_linter_add_use_cfg_attribute
Closed

feat(linter): add use_cfg attribute.#3744
rzvxa wants to merge 4 commits intomainfrom
06-18-feat_linter_add_use_cfg_attribute

Conversation

@rzvxa
Copy link
Contributor

@rzvxa rzvxa commented Jun 18, 2024

related to this conversation #3737 (comment)

Adds a use_cfg attribute to use with declare_oxc_lint macro. When this attribute is used it would set the rule's RuleMeta::USE_CFG constant to true.

Since with this information, we can figure out whether a set of rules uses control flow or not we can use this information to provide a LintContext::cfg method that would panic if called and cfg doesn't exist.
Basically in the context of a rule, the linter should make sure the rule which requests control flow generation always gets it.

@github-actions github-actions bot added the A-linter Area - Linter label Jun 18, 2024
Copy link
Contributor Author

rzvxa commented Jun 18, 2024

@graphite-app
Copy link
Contributor

graphite-app bot commented Jun 18, 2024

Your org has enabled the Graphite merge queue for merging into main

Add the label “merge” to the PR and Graphite will automatically add it to the merge queue when it’s ready to merge. Or use the label “hotfix” to add to the merge queue as a hot fix.

You must have a Graphite account and log in to Graphite in order to use the merge queue. Sign up using this link.

@rzvxa rzvxa requested review from Boshen and overlookmotel June 18, 2024 18:38
@rzvxa
Copy link
Contributor Author

rzvxa commented Jun 18, 2024

@Boshen @overlookmotel what do you think?

@rzvxa
Copy link
Contributor Author

rzvxa commented Jun 18, 2024

We can also have 2 types of traits for these to remove the ctx.cfg method altogether for rules that don't have this attribute. For example LintContext and ControlFlowLintContext.

@codspeed-hq
Copy link

codspeed-hq bot commented Jun 18, 2024

CodSpeed Performance Report

Merging #3744 will improve performances by 7.62%

Comparing 06-18-feat_linter_add_use_cfg_attribute (6198181) with main (49d28c0)

Summary

⚡ 1 improvements
✅ 21 untouched benchmarks

Benchmarks breakdown

Benchmark main 06-18-feat_linter_add_use_cfg_attribute Change
sourcemap[cal.com.tsx] 56.6 ms 52.6 ms +7.62%

@rzvxa rzvxa force-pushed the 06-18-feat_linter_add_use_cfg_attribute branch from a343b03 to ebc729b Compare June 18, 2024 19:54
@rzvxa rzvxa changed the base branch from main to 06-18-chore_benchmark_enable_cfg_for_linter_benchmarks June 18, 2024 19:54
@rzvxa rzvxa marked this pull request as ready for review June 18, 2024 20:06
@Boshen Boshen changed the base branch from 06-18-chore_benchmark_enable_cfg_for_linter_benchmarks to graphite-base/3744 June 19, 2024 05:13
@Boshen
Copy link
Member

Boshen commented Jun 19, 2024

I'm not sure about the following 3 PRs, let me hold onto this.

I think it's fine to always run cfg in the linter:

  • it's rare that they are all disabled
  • we want to catch bugs where the cfg panics

I also think we should let it crash if cfg is not set, instead of enforcing it at compile time, which over complicates the code.

@Boshen Boshen force-pushed the 06-18-feat_linter_add_use_cfg_attribute branch from a533fde to 6198181 Compare June 19, 2024 05:17
@Boshen Boshen changed the base branch from graphite-base/3744 to main June 19, 2024 05:17
@overlookmotel
Copy link
Member

My preference tends to be to use the type system to rule out impossible situations at compile time, rather than the risk of runtime panics (and the extra branching and ASM this involves). However, as Boshen says, type system stuff does have the downside of complicating the code.

Probably I shouldn't have waded in on this question as I don't know the linter at all. I don't know how expensive constructing CFG is, or how common it is that all rules that require it would all be disabled. So I can't really assess the cost/benefit balance.

@rzvxa
Copy link
Contributor Author

rzvxa commented Jun 19, 2024

I'm not sure about the following 3 PRs, let me hold onto this.

I think it's fine to always run cfg in the linter:

* it's rare that they are all disabled

* we want to catch bugs where the cfg panics

I also think we should let it crash if cfg is not set, instead of enforcing it at compile time, which over complicates the code.

I guess I should close these then right? Shall I create a PR and salvage some parts like the cfg method which you've liked? Is there anything else that you'd like to keep?

@rzvxa
Copy link
Contributor Author

rzvxa commented Jun 19, 2024

My preference tends to be to use the type system to rule out impossible situations at compile time, rather than the risk of runtime panics (and the extra branching and ASM this involves). However, as Boshen says, type system stuff does have the downside of complicating the code.

I also like the idea of depending on the type system instead of runtime checks especially if it can be a zero-cost abstraction. Makes code more clear in my opinion(for the new contributors/consumers, not the maintainers). But I have to agree with Boshen in regards to it being a bit overkill for our current needs.

I don't know how expensive constructing CFG is, or how common it is that all rules that require it would all be disabled.

It consumes about 2 to 3 percents so it isn't mostly about performance gain, It allows better feature gating though. There are a handful of CFG rules implemented and about a dozen or so in total (some aren't recommended and we might skip).

@overlookmotel
Copy link
Member

Thanks for coming back with the background. I think I don't know enough of the linter at present to contribute more to this particular debate, so best I leave it to you and Boshen to decide what's best. Sorry my comment (which I'd meant as kind of out loud musing) has lead you "up the garden path".

@rzvxa
Copy link
Contributor Author

rzvxa commented Jun 19, 2024

Thanks for coming back with the background. I think I don't know enough of the linter at present to contribute more to this particular debate, so best I leave it to you and Boshen to decide what's best. Sorry my comment (which I'd meant as kind of out loud musing) has lead you "up the garden path".

You don't have to be sorry; Even if you hadn't mentioned it I might've still ended up on this tangent, I love type "ejaculations" so this experiment was a fun ride for me😆

@Boshen Boshen closed this Jun 19, 2024
@Boshen Boshen deleted the 06-18-feat_linter_add_use_cfg_attribute branch June 27, 2024 13:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-linter Area - Linter

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants