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(linter): Implement jsdoc/check-access #2642

Merged
merged 17 commits into from
Apr 4, 2024
Merged

Conversation

leaysgur
Copy link
Contributor

@leaysgur leaysgur commented Mar 8, 2024

Part of #1170, Finally... 🗻

Some preparation PRs may be needed in advance.

Implement plugin itself.

I'll send a PR to make this plugin public after confirming that a few more rules can be implemented without any problems.

@github-actions github-actions bot added A-linter Area - Linter A-semantic Area - Semantic labels Mar 8, 2024
@leaysgur
Copy link
Contributor Author

leaysgur commented Mar 8, 2024

@Boshen These are some concerns noticed when I tried it. What do you think?

Is it OK to use run_once?

I got a review in a previous PR: If run_once is implemented, but run is still executed.

Is this unavoidable?

Resolved

struct JSDocTag

Currently, there is a single struct JSDocTag and distinguished by its .kind.

Should there be a separate struct for each tag?
Then, for example, something like has_valid_level() could be implemented in JSDocAccessTag.

But current string based implementation also looks fine for me.
(There is no specification of how users actually describe JSDoc. 🙄 )

It depends on how deeply Oxc wants to support JSDoc?

Failed CI

(I'm not sure I can fix this...!)

This is the failed comments from antd.mjs.

/**
 * flat tree data on expanded state
 *
 * @export
 * @template T
 * @param {*} data : table data
 * @param {string} childrenColumnName : 指定树形结构的列名
 * @param {Set<Key>} expandedKeys : 展开的行对应的keys
 * @param {GetRowKey<T>} getRowKey  : 获取当前rowKey的方法
 * @returns flattened data
 */

Something wrong in @param parsing?
(BTW, why this does not happen in main branch...?)

UPDATE: maybe should fix &self.source_text[start..self.current]...? + using chars() to char_indices() everywhere...?

@leaysgur leaysgur mentioned this pull request Mar 14, 2024
3 tasks
Boshen pushed a commit that referenced this pull request Mar 14, 2024
Base work for #2642 🏃🏻 

- [x] struct
- [x] bool flags
- [x] tagNamePreferences
Copy link

codspeed-hq bot commented Mar 15, 2024

CodSpeed Performance Report

Merging #2642 will improve performances by 6.82%

Comparing plugin-jsdoc/check-access (ab77d00) with main (6823482)

Summary

⚡ 1 improvements
✅ 35 untouched benchmarks

Benchmarks breakdown

Benchmark main plugin-jsdoc/check-access Change
sourcemap[react.development.js] 21 ms 19.6 ms +6.82%

@Boshen
Copy link
Member

Boshen commented Mar 15, 2024

Is it OK to use run_once?

This is the work flow:

pub fn run<'a>(&self, ctx: LintContext<'a>) -> Vec<Message<'a>> {
let semantic = Rc::clone(ctx.semantic());
let mut ctx =
ctx.with_fix(self.options.fix).with_settings(&self.settings).with_env(&self.env);
for (rule_name, rule) in &self.rules {
ctx.with_rule_name(rule_name);
rule.run_once(&ctx);
}
for symbol in semantic.symbols().iter() {
for (rule_name, rule) in &self.rules {
ctx.with_rule_name(rule_name);
rule.run_on_symbol(symbol, &ctx);
}
}
for node in semantic.nodes().iter() {
for (rule_name, rule) in &self.rules {
ctx.with_rule_name(rule_name);
rule.run(node, &ctx);
}
}

We try to avoid visiting the AST twice.


Should there be a separate struct for each tag?

Do this instead:

impl<'a> JSDocTag<'a> {
    fn kind(&self) -> JSDocTagKind<'a> {
        self.kind
    }
}

impl<'a> JSDocTagKind<'a> {
    fn is_deprecated(&self) -> bool {
        matches!(self, Self::Deprecated)
    }

    fn parameter(&self) -> Option<&Param<'a>> {
        if let Self::Parameter(param) = self {
            Some(param)
        } else {
            None
        }
    }
}

I think this is easier than many nodes containing comment and then a trait implemented on each node for retrieving the comment ...

@leaysgur
Copy link
Contributor Author

leaysgur commented Mar 15, 2024

Thanks!

This is the work flow:

Many of plugin-jsdoc rules will use run_once() only.

In these cases,

for node in semantic.nodes().iter() { 
   for (rule_name, rule) in &self.rules { 
       ctx.with_rule_name(rule_name); 
       rule.run(node, &ctx); // this run() fn is almost empty 👀 
   } 
} 

Does this mean negligibly cheep?

Or should consider something like:

for node in semantic.nodes().iter() { 
   for (rule_name, rule) in &self.rules { 
       if rule.can_skip_run { continue; } // <-
       ctx.with_rule_name(rule_name); 
       rule.run(node, &ctx);
   } 
} 

?

@leaysgur

This comment was marked as resolved.

@leaysgur leaysgur marked this pull request as ready for review April 1, 2024 05:42
@leaysgur
Copy link
Contributor Author

leaysgur commented Apr 4, 2024

@Boshen Thanks for merging #2893 , how about this PR?

@Boshen Boshen merged commit aa63b64 into main Apr 4, 2024
33 checks passed
@Boshen Boshen deleted the plugin-jsdoc/check-access branch April 4, 2024 14:36
@Boshen
Copy link
Member

Boshen commented Apr 4, 2024

Thank you! Sorry I missed the PR.

Brooooooklyn referenced this pull request in toeverything/AFFiNE Apr 11, 2024
[![Mend Renovate](https://app.renovatebot.com/images/banner.svg)](https://renovatebot.com)

This PR contains the following updates:

| Package | Change | Age | Adoption | Passing | Confidence |
|---|---|---|---|---|---|
| [oxlint](https://oxc-project.github.io) ([source](https://github.com/oxc-project/oxc/tree/HEAD/npm/oxlint)) | [`0.2.14` -> `0.2.17`](https://renovatebot.com/diffs/npm/oxlint/0.2.14/0.2.17) | [![age](https://developer.mend.io/api/mc/badges/age/npm/oxlint/0.2.17?slim=true)](https://docs.renovatebot.com/merge-confidence/) | [![adoption](https://developer.mend.io/api/mc/badges/adoption/npm/oxlint/0.2.17?slim=true)](https://docs.renovatebot.com/merge-confidence/) | [![passing](https://developer.mend.io/api/mc/badges/compatibility/npm/oxlint/0.2.14/0.2.17?slim=true)](https://docs.renovatebot.com/merge-confidence/) | [![confidence](https://developer.mend.io/api/mc/badges/confidence/npm/oxlint/0.2.14/0.2.17?slim=true)](https://docs.renovatebot.com/merge-confidence/) |

---

### Release Notes

<details>
<summary>oxc-project/oxc (oxlint)</summary>

### [`v0.2.17`](https://github.com/oxc-project/oxc/releases/tag/oxlint_v0.2.17): oxlint v0.2.17

[Compare Source](https://github.com/oxc-project/oxc/compare/7066d55153ad70f95ae975adc3958c1010f9c5ff...df11d10a2220e9aa7a33d9ab39ed662c2ba6fdb5)

##### What's Changed

-   feat(linter): eslint-plugin-jest/prefer-lowercase-title by [@&#8203;eryue0220](https://github.com/eryue0220) in [https://github.com/oxc-project/oxc/pull/2911](https://github.com/oxc-project/oxc/pull/2911)
-   feat(linter): typescript-eslint/consistent-type-definitions by [@&#8203;todor-a](https://github.com/todor-a) in [https://github.com/oxc-project/oxc/pull/2885](https://github.com/oxc-project/oxc/pull/2885)
-   fix(cli): fix `oxlint --format json` yields 0 files to lint by [@&#8203;Boshen](https://github.com/Boshen) in [https://github.com/oxc-project/oxc/pull/2940](https://github.com/oxc-project/oxc/pull/2940)
-   fix(cli): if format is json do not print summary information ([#&#8203;2899](https://github.com/oxc-project/oxc/issues/2899)) by [@&#8203;kalvenschraut](https://github.com/kalvenschraut) in [https://github.com/oxc-project/oxc/pull/2925](https://github.com/oxc-project/oxc/pull/2925)
-   fix(linter): import/no-cycle ignore type-only imports by [@&#8203;JohnDaly](https://github.com/JohnDaly) in [https://github.com/oxc-project/oxc/pull/2924](https://github.com/oxc-project/oxc/pull/2924)
-   refactor(semantic/jsdoc): Rework JSDoc struct for better Span handling by [@&#8203;leaysgur](https://github.com/leaysgur) in [https://github.com/oxc-project/oxc/pull/2917](https://github.com/oxc-project/oxc/pull/2917)

##### New Contributors

-   [@&#8203;bradzacher](https://github.com/bradzacher) made their first contribution in [https://github.com/oxc-project/oxc/pull/2938](https://github.com/oxc-project/oxc/pull/2938)

**Full Changelog**: oxc-project/oxc@oxlint_v0.2.16...oxlint_v0.2.17

### [`v0.2.16`](https://github.com/oxc-project/oxc/releases/tag/oxlint_v0.2.16): oxlint v0.2.16

[Compare Source](https://github.com/oxc-project/oxc/compare/e7307ed23ca9b0707586b6bf4220cafb221ae86e...7066d55153ad70f95ae975adc3958c1010f9c5ff)

#### What's Changed

-   feat(linter): [@&#8203;typescript-eslint/prefer-for-of](https://github.com/typescript-eslint/prefer-for-of) by [@&#8203;charnog](https://github.com/charnog) in [https://github.com/oxc-project/oxc/pull/2789](https://github.com/oxc-project/oxc/pull/2789)
-   feat(linter): Implement jsdoc/check-access by [@&#8203;leaysgur](https://github.com/leaysgur) in [https://github.com/oxc-project/oxc/pull/2642](https://github.com/oxc-project/oxc/pull/2642)
-   feat(linter): Implement jsdoc/empty-tags by [@&#8203;leaysgur](https://github.com/leaysgur) in [https://github.com/oxc-project/oxc/pull/2893](https://github.com/oxc-project/oxc/pull/2893)
-   feat(linter): eslint-plugin-jest/prefer-mock-promise-sorthand by [@&#8203;eryue0220](https://github.com/eryue0220) in [https://github.com/oxc-project/oxc/pull/2864](https://github.com/oxc-project/oxc/pull/2864)
-   feat(linter/import): Add `ignoreTypes` option for the `import/no-cycle` rule by [@&#8203;JohnDaly](https://github.com/JohnDaly) in [https://github.com/oxc-project/oxc/pull/2905](https://github.com/oxc-project/oxc/pull/2905)
-   fix(ast): `FinallyClause` won't get visited as `BlockStatement` anymore. by [@&#8203;rzvxa](https://github.com/rzvxa) in [https://github.com/oxc-project/oxc/pull/2881](https://github.com/oxc-project/oxc/pull/2881)
-   fix(linter): handle self closing script tags in astro partial loader ([#&#8203;2017](https://github.com/oxc-project/oxc/issues/2017)) by [@&#8203;kalvenschraut](https://github.com/kalvenschraut) in [https://github.com/oxc-project/oxc/pull/2907](https://github.com/oxc-project/oxc/pull/2907)
-   fix(linter): svelte partial loader handle generics ([#&#8203;2875](https://github.com/oxc-project/oxc/issues/2875)) by [@&#8203;kalvenschraut](https://github.com/kalvenschraut) in [https://github.com/oxc-project/oxc/pull/2906](https://github.com/oxc-project/oxc/pull/2906)

#### New Contributors

-   [@&#8203;charnog](https://github.com/charnog) made their first contribution in [https://github.com/oxc-project/oxc/pull/2789](https://github.com/oxc-project/oxc/pull/2789)
-   [@&#8203;kalvenschraut](https://github.com/kalvenschraut) made their first contribution in [https://github.com/oxc-project/oxc/pull/2906](https://github.com/oxc-project/oxc/pull/2906)
-   [@&#8203;JohnDaly](https://github.com/JohnDaly) made their first contribution in [https://github.com/oxc-project/oxc/pull/2905](https://github.com/oxc-project/oxc/pull/2905)

**Full Changelog**: oxc-project/oxc@oxlint_v0.2.15...oxlint_v0.2.16

### [`v0.2.15`](https://github.com/oxc-project/oxc/releases/tag/oxlint_v0.2.15): oxlint v0.2.15

[Compare Source](https://github.com/oxc-project/oxc/compare/b1343d7bcbd490105583b561946f057ac91e40cf...e7307ed23ca9b0707586b6bf4220cafb221ae86e)

#### What's Changed

-   feat(linter): default_param_last by [@&#8203;JoSeBu1](https://github.com/JoSeBu1) in [https://github.com/oxc-project/oxc/pull/2756](https://github.com/oxc-project/oxc/pull/2756)
-   feat(linter): eslint-plugin-jest/no-untyped-mock-factory by [@&#8203;eryue0220](https://github.com/eryue0220) in [https://github.com/oxc-project/oxc/pull/2807](https://github.com/oxc-project/oxc/pull/2807)
-   feat(linter): eslint-plugin-jest/prefer-comparison-matcher by [@&#8203;eryue0220](https://github.com/eryue0220) in [https://github.com/oxc-project/oxc/pull/2806](https://github.com/oxc-project/oxc/pull/2806)
-   feat(linter): eslint-plugin-react checked-requires-onchange-or-readonly by [@&#8203;keita-hino](https://github.com/keita-hino) in [https://github.com/oxc-project/oxc/pull/2754](https://github.com/oxc-project/oxc/pull/2754)
-   feat(linter): eslint/no-iterator by [@&#8203;JoSeBu1](https://github.com/JoSeBu1) in [https://github.com/oxc-project/oxc/pull/2758](https://github.com/oxc-project/oxc/pull/2758)
-   feat(linter): fallback to the default tsconfig path by [@&#8203;Dunqing](https://github.com/Dunqing) in [https://github.com/oxc-project/oxc/pull/2842](https://github.com/oxc-project/oxc/pull/2842)
-   feat(linter): no_script_url by [@&#8203;JoSeBu1](https://github.com/JoSeBu1) in [https://github.com/oxc-project/oxc/pull/2761](https://github.com/oxc-project/oxc/pull/2761)
-   feat(linter/import) check deep namespace in namespace rule by [@&#8203;Dunqing](https://github.com/Dunqing) in [https://github.com/oxc-project/oxc/pull/2805](https://github.com/oxc-project/oxc/pull/2805)
-   feat(linter/import) check module import in no_duplicates by [@&#8203;Dunqing](https://github.com/Dunqing) in [https://github.com/oxc-project/oxc/pull/2771](https://github.com/oxc-project/oxc/pull/2771)
-   feat(linter/import) check type import in no_duplicates by [@&#8203;Dunqing](https://github.com/Dunqing) in [https://github.com/oxc-project/oxc/pull/2777](https://github.com/oxc-project/oxc/pull/2777)
-   feat(linter/import) support allow_computed option in namespace by [@&#8203;Dunqing](https://github.com/Dunqing) in [https://github.com/oxc-project/oxc/pull/2840](https://github.com/oxc-project/oxc/pull/2840)
-   feat(linter/import) support check re-export in named by [@&#8203;Dunqing](https://github.com/Dunqing) in [https://github.com/oxc-project/oxc/pull/2769](https://github.com/oxc-project/oxc/pull/2769)
-   feat(linter/import): ignore type-only imports and exports in no_unresolved by [@&#8203;Dunqing](https://github.com/Dunqing) in [https://github.com/oxc-project/oxc/pull/2849](https://github.com/oxc-project/oxc/pull/2849)
-   fix(linter/import): false positive for indirect export in namespace by [@&#8203;Dunqing](https://github.com/Dunqing) in [https://github.com/oxc-project/oxc/pull/2862](https://github.com/oxc-project/oxc/pull/2862)
-   fix(linter/import): ignore export declaration in no-duplicates by [@&#8203;Dunqing](https://github.com/Dunqing) in [https://github.com/oxc-project/oxc/pull/2863](https://github.com/oxc-project/oxc/pull/2863)
-   fix(linter/max-lines): only report codes that exceed the line limit by [@&#8203;mysteryven](https://github.com/mysteryven) in [https://github.com/oxc-project/oxc/pull/2778](https://github.com/oxc-project/oxc/pull/2778)
-   fix(parser): add support for empty module declaration by [@&#8203;rzvxa](https://github.com/rzvxa) in [https://github.com/oxc-project/oxc/pull/2834](https://github.com/oxc-project/oxc/pull/2834)

#### New Contributors

-   [@&#8203;rzvxa](https://github.com/rzvxa) made their first contribution in [https://github.com/oxc-project/oxc/pull/2764](https://github.com/oxc-project/oxc/pull/2764)

**Full Changelog**: oxc-project/oxc@oxlint_v0.2.14...oxlint_v0.2.15

</details>

---

### Configuration

📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined).

🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied.

♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox.

🔕 **Ignore**: Close this PR and you won't be reminded about this update again.

---

 - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check this box

---

This PR has been generated by [Mend Renovate](https://www.mend.io/free-developer-tools/renovate/). View repository job log [here](https://developer.mend.io/github/toeverything/AFFiNE).
<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNy4xNTMuMiIsInVwZGF0ZWRJblZlciI6IjM3LjI2OS4yIiwidGFyZ2V0QnJhbmNoIjoiY2FuYXJ5In0=-->
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-linter Area - Linter A-semantic Area - Semantic
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants