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): eslint-plugin-next/no-duplicate-head #2438

Conversation

IWANABETHATGUY
Copy link
Contributor

@IWANABETHATGUY IWANABETHATGUY commented Feb 19, 2024

@github-actions github-actions bot added A-linter Area - Linter A-ast Area - AST labels Feb 19, 2024
Copy link

codspeed-hq bot commented Feb 19, 2024

CodSpeed Performance Report

Merging #2438 will not alter performance

Comparing IWANABETHATGUY:feat/eslint-plugin-next/no-duplicate-head (02e2f13) with main (a78303d)

Summary

✅ 27 untouched benchmarks

@IWANABETHATGUY IWANABETHATGUY force-pushed the feat/eslint-plugin-next/no-duplicate-head branch from 1cc84d6 to 9523a61 Compare February 20, 2024 01:44
@IWANABETHATGUY IWANABETHATGUY marked this pull request as ready for review February 20, 2024 01:44
@IWANABETHATGUY IWANABETHATGUY changed the title feat: eslint-plugin-next/no-duplicate-head feat(linter): eslint-plugin-next/no-duplicate-head Feb 20, 2024
use crate::{context::LintContext, rule::Rule};

#[derive(Debug, Error, Diagnostic)]
#[error("eslint-plugin-next(no-duplicate-head):")]
Copy link
Member

Choose a reason for hiding this comment

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

Need a error message here.

let nodes = ctx.semantic().nodes();
for node in nodes.iter() {
match node.kind() {
oxc_ast::AstKind::ImportDeclaration(decl) => {
Copy link
Member

@Dunqing Dunqing Feb 20, 2024

Choose a reason for hiding this comment

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

Here we can use AstKind:ImportDefaultSpecifier instead, the parent kind of the AstKind:ImportDefaultSpecifier is AstKind::ImportDeclaration

Comment on lines +72 to +82
oxc_ast::AstKind::ReturnStatement(stmt) => {
let document_class_id = nodes.ancestors(node.id()).find(|node_id| {
matches!(nodes.get_node(*node_id).kind(),
AstKind::Class(class)
if class
.super_class
.as_ref()
.and_then(|sc| sc.as_identifier())
.map(|id| &id.name)
== document_import_name.as_ref())
});
Copy link
Member

@Dunqing Dunqing Feb 20, 2024

Choose a reason for hiding this comment

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

I think we can have a simpler way to find the class, for example via ctx.semantic.classes

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure if this would be faster, you still need to figure out if the class is the ancestor of current ReturnStatement

@Boshen
Copy link
Member

Boshen commented Feb 20, 2024

Wait ... can't we just find all the read references of Head? We don't need to go over the AST because we have semantic data.

@Dunqing
Copy link
Member

Dunqing commented Feb 20, 2024

Wait ... can't we just find all the read references of Head? We don't need to go over the AST because we have semantic data.

Yes! Sounds reasonable.

@IWANABETHATGUY
Copy link
Contributor Author

IWANABETHATGUY commented Feb 20, 2024

Wait ... can't we just find all the read references of Head? We don't need to go over the AST because we have semantic data.

Good idea, however, you still need to extract the documentImport from ImportDefaultSpecifier, and the ImportDeclaration could be everywhere in a js file. e.g. :

console.log("what ever")
test
Document
import Document, {test} from './test'

So iterating the nodes could not be skipped

@Boshen
Copy link
Member

Boshen commented Feb 20, 2024

Wait ... can't we just find all the read references of Head? We don't need to go over the AST because we have semantic data.

Good idea, however, you still need to extract the documentImport from ImportDefaultSpecifier, and the ImportDeclaration could be everywhere in a js file. e.g. :

console.log("what ever")
test
Document
import Document, {test} from './test'

So iterating the nodes could not be skipped

Yeah, the import part is still needed.

@Dunqing
Copy link
Member

Dunqing commented Feb 20, 2024

Wait ... can't we just find all the read references of Head? We don't need to go over the AST because we have semantic data.

Good idea, however, you still need to extract the documentImport from ImportDefaultSpecifier, and the ImportDeclaration could be everywhere in a js file. e.g. :

console.log("what ever")
test
Document
import Document, {test} from './test'

So iterating the nodes could not be skipped

You can find the documentImport by symbols. The Document symbol flag will contain SymbolFlags:ImportBinding

https://oxc-project.github.io/oxc/playground/?code=3YCAAIDMgICAgICAgICxG0qZRWtpV52IeeJbT1tdEcK4UeIwEHHwiWefH4zZxP7cwptFUwhrdc4TI795I028%2FXPY7FZcAXXzk10cqHp4LqCsHLj%2FfQlBwA%3D%3D

@Boshen
Copy link
Member

Boshen commented Feb 20, 2024

Wait ... can't we just find all the read references of Head? We don't need to go over the AST because we have semantic data.

Good idea, however, you still need to extract the documentImport from ImportDefaultSpecifier, and the ImportDeclaration could be everywhere in a js file. e.g. :

console.log("what ever")
test
Document
import Document, {test} from './test'

So iterating the nodes could not be skipped

You can find the documentImport by symbols. The Document symbol flag will contain SymbolFlags:ImportBinding

https://oxc-project.github.io/oxc/playground/?code=3YCAAIDMgICAgICAgICxG0qZRWtpV52IeeJbT1tdEcK4UeIwEHHwiWefH4zZxP7cwptFUwhrdc4TI795I028%2FXPY7FZcAXXzk10cqHp4LqCsHLj%2FfQlBwA%3D%3D

But you don't know if it's imported from next or renamed :-(

@Boshen Boshen marked this pull request as draft February 24, 2024 09:26
Boshen added a commit that referenced this pull request May 6, 2024
Boshen added a commit that referenced this pull request May 6, 2024
@Dunqing Dunqing closed this in #3174 May 6, 2024
Dunqing pushed a commit that referenced this pull request May 6, 2024
@IWANABETHATGUY IWANABETHATGUY deleted the feat/eslint-plugin-next/no-duplicate-head branch May 6, 2024 16:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-ast Area - AST A-linter Area - Linter
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants