feat(linter): eslint/no-constructor-return #3321

woai3c · 2024-05-17T05:39:55Z

No description provided.

graphite-app · 2024-05-17T05:40:00Z

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.

codspeed-hq · 2024-05-17T05:44:53Z

CodSpeed Performance Report

Merging #3321 will not alter performance

_{Comparing woai3c:feat/no-constructor-return (12bdddc) with woai3c:feat/no-constructor-return (dbb0686)}

Summary

✅ 22 untouched benchmarks

Boshen · 2024-05-17T06:01:01Z

This approach is slow, it is preferred to use the control flow graph

https://github.com/eslint/eslint/blob/5c28d9a367e1608e097c491f40b8afd0730a8b9e/lib/rules/no-constructor-return.js#L36-L41

woai3c · 2024-05-17T09:52:33Z

This approach is slow, it is preferred to use the control flow graph

https://github.com/eslint/eslint/blob/5c28d9a367e1608e097c491f40b8afd0730a8b9e/lib/rules/no-constructor-return.js#L36-L41

Ok, Is there a demo related to the control flow graph in oxc? I don't understand how to implement it in oxc.

Boshen · 2024-05-17T10:11:34Z

This approach is slow, it is preferred to use the control flow graph
https://github.com/eslint/eslint/blob/5c28d9a367e1608e097c491f40b8afd0730a8b9e/lib/rules/no-constructor-return.js#L36-L41

Ok, Is there a demo related to the control flow graph in oxc? I don't understand how to implement it in oxc.

CFG is a bit complicated, you may start from https://github.com/oxc-project/oxc/blob/main/crates/oxc_semantic/examples/cfg.rs

and also the linter rules that use it, e.g. eslint/getter_return.rs

woai3c · 2024-05-21T15:39:49Z

This approach is slow, it is preferred to use the control flow graph
https://github.com/eslint/eslint/blob/5c28d9a367e1608e097c491f40b8afd0730a8b9e/lib/rules/no-constructor-return.js#L36-L41

Ok, Is there a demo related to the control flow graph in oxc? I don't understand how to implement it in oxc.

CFG is a bit complicated, you may start from https://github.com/oxc-project/oxc/blob/main/crates/oxc_semantic/examples/cfg.rs

and also the linter rules that use it, e.g. eslint/getter_return.rs

@Boshen @rzvxa Hi, I have been reading the code for require_render_return. While implementing no-constructor-return, I encountered a problem. I need to access the return statement node for two reasons: first, to display the position of the return statement span, and second, to check if the return statement node is in the block statement.

Consider the following example:

class C { constructor(a) { if (!a) { return } else { a() } } }

The code above is correct. The return is used for control flow. Therefore, I would like to know how to access the return statement node using neighbors_filtered_by_edge_weight.

rzvxa · 2024-05-21T18:51:03Z

As far as I know, this information is lost in our current control flow implementation. For some other reason - which isn't that far off from what you need here #3238 - I'm working on adding the list of instructions to the basic blocks and reworking some parts of cfg to make it more versatile. So if things go according to the plan soon it should be possible to somehow access our nodes in basic blocks.

If you only need to know whether a return has an expression or not you can use the AssignmentValue - which is what you are already doing in this screenshot - is that not sufficient for this problem? What do you need from the return statement? maybe we can work around it.

woai3c · 2024-05-22T03:21:15Z

As far as I know, this information is lost in our current control flow implementation. For some other reason - which isn't that far off from what you need here #3238 - I'm working on adding the list of instructions to the basic blocks and reworking some parts of cfg to make it more versatile. So if things go according to the plan soon it should be possible to somehow access our nodes in basic blocks.

If you only need to know whether a return has an expression or not you can use the AssignmentValue - which is what you are already doing in this screenshot - is that not sufficient for this problem? What do you need from the return statement? maybe we can work around it.

I need to access the return statement node because the error information related to the span is incorrect.

The second season is that I need to know if the return statement node in a block statement. consider the following example:

class C { constructor(a) { if (!a) { return } else { a() } } }

The above code should be correct. Because the return is used for flow control.

I'm not sure if there's a way to solve these two problems. Looking forward to your suggestions. Here is my code:

impl Rule for NoConstructorReturn {
    fn run<'a>(&self, node: &AstNode<'a>, ctx: &LintContext<'a>) {
        if !matches!(node.kind(), AstKind::ArrowFunctionExpression(_) | AstKind::Function(_)) {
            return;
        }

        let Some(parent) = ctx.nodes().parent_node(node.id()) else {
            return;
        };

        if contains_return_statement(node, ctx) {
            match parent.kind() {
                AstKind::MethodDefinition(method)
                    if method.kind == MethodDefinitionKind::Constructor =>
                {
                    ctx.diagnostic(no_constructor_return_diagnostic(method.span));
                }
                _ => {}
            };
        }
    }
}

const KEEP_WALKING_ON_THIS_PATH: bool = true;
const STOP_WALKING_ON_THIS_PATH: bool = false;

fn contains_return_statement<'a>(node: &AstNode<'a>, ctx: &LintContext<'a>) -> bool {
    let cfg = ctx.semantic().cfg();
    let state = neighbors_filtered_by_edge_weight(
        &cfg.graph,
        node.cfg_ix(),
        &|edge| match edge {
            // We only care about normal edges having a return statement.
            EdgeType::Normal => None,
            // For these two type, we flag it as not found.
            EdgeType::NewFunction | EdgeType::Backedge => Some(FoundReturn::No),
        },
        &mut |basic_block_id, _state_going_into_this_rule| {
            // If its an arrow function with an expression, marked as founded and stop walking.
            if let AstKind::ArrowFunctionExpression(arrow_expr) = node.kind() {
                if arrow_expr.expression {
                    return (FoundReturn::Yes, STOP_WALKING_ON_THIS_PATH);
                }
            }

            for entry in cfg.basic_block_by_index(*basic_block_id) {
                if let BasicBlockElement::Assignment(to_reg, val) = entry {
                    if matches!(to_reg, Register::Return)
                        && matches!(val, AssignmentValue::NotImplicitUndefined)
                    {
                        return (FoundReturn::Yes, STOP_WALKING_ON_THIS_PATH);
                    }
                }
            }

            (FoundReturn::No, KEEP_WALKING_ON_THIS_PATH)
        },
    );

    state.iter().any(|&state| state == FoundReturn::Yes)
}

#[derive(Clone, Copy, Debug, Default, PartialEq)]
enum FoundReturn {
    #[default]
    No,
    Yes,
}

#[test]
fn test() {
    use crate::tester::Tester;

    let pass = vec![
        "function fn() { return }",
        "function fn(kumiko) { if (kumiko) { return kumiko } }",
        "const fn = function () { return }",
        "const fn = function () { if (kumiko) { return kumiko } }",
        "const fn = () => { return }",
        "const fn = () => { if (kumiko) { return kumiko } }",
        "return 'Kumiko Oumae'",
        "class C {  }",
        "class C { constructor() {} }",
        "class C { constructor() { let v } }",
        "class C { method() { return '' } }",
        "class C { get value() { return '' } }",
        "class C { constructor(a) { if (!a) { return } else { a() } } }",
        "class C { constructor() { function fn() { return true } } }",
        "class C { constructor() { this.fn = function () { return true } } }",
        "class C { constructor() { this.fn = () => { return true } } }",
    ];

    let fail = vec![
        // "class C { constructor() { return } }",
        "class C { constructor() { return '' } }",
        "class C { constructor(a) { if (!a) { return '' } else { a() } } }",
    ];

    Tester::new(NoConstructorReturn::NAME, pass, fail).test_and_snapshot();
}

rzvxa · 2024-05-22T11:07:44Z

Can you test this approach and see how it performs?

impl Rule for NoConstructorReturn {
    fn run<'a>(&self, node: &AstNode<'a>, ctx: &LintContext<'a>) {
        let AstKind::ReturnStatement(ret) = node.kind() else { return };
        let Some(is_in_constructor_root) = is_in_constructor_root(ctx, node.id()) else { return };

        if is_in_constructor_root {
            ctx.diagnostic(no_constructor_return_diagnostic(ret.span));
        } else if ret.argument.is_some() && is_definitely_in_constructor(ctx, node.id()) {
            ctx.diagnostic(no_constructor_return_diagnostic(ret.span));
        }
    }
}

fn is_constructor(node: &AstNode<'_>) -> bool {
    matches!(
        node.kind(),
        AstKind::MethodDefinition(MethodDefinition { kind: MethodDefinitionKind::Constructor, .. })
    )
}

/// Checks to see if the given node is in the root of a constructor.
/// Returns `None` if it isn't possible for this node to be in a constructor.
fn is_in_constructor_root(ctx: &LintContext, node_id: AstNodeId) -> Option<bool> {
    ctx.nodes().ancestors(node_id).nth(3).map(|id| ctx.nodes().get_node(id)).map(is_constructor)
}

fn is_definitely_in_constructor(ctx: &LintContext, node_id: AstNodeId) -> bool {
    ctx.nodes()
        .ancestors(node_id)
        .map(|id| ctx.nodes().get_node(id))
        .skip_while(|node| !node.kind().is_function_like())
        .nth(1)
        .is_some_and(is_constructor)
}

Obviously, it isn't as good as what you've been doing here, And we might want to switch back to that when we have the improved control flow, But I suspect that the snippet above should perform OKish at least for now.

Edit:

Bug fix; Please use this function instead:

fn is_in_constructor_root(ctx: &LintContext, node_id: AstNodeId) -> Option<bool> {
    ctx.nodes()
        .ancestors(node_id)
        .map(|id| ctx.nodes().get_node(id))
        .filter(|it| !matches!(it.kind(), AstKind::BlockStatement(_)))
        .nth(3)
        .map(is_constructor)
}

And also add something like this to test cases:

class C { constructor() { { { return } } } }

rzvxa · 2024-05-22T11:25:09Z

This approach is slow, it is preferred to use the control flow graph

Is what I've suggested here your initial approach?

woai3c · 2024-05-22T14:11:03Z

Can you test this approach and see how it performs?

impl Rule for NoConstructorReturn {
    fn run<'a>(&self, node: &AstNode<'a>, ctx: &LintContext<'a>) {
        let AstKind::ReturnStatement(ret) = node.kind() else { return };
        let Some(is_in_constructor_root) = is_in_constructor_root(ctx, node.id()) else { return };

        if is_in_constructor_root {
            ctx.diagnostic(no_constructor_return_diagnostic(ret.span));
        } else if ret.argument.is_some() && is_definitely_in_constructor(ctx, node.id()) {
            ctx.diagnostic(no_constructor_return_diagnostic(ret.span));
        }
    }
}

fn is_constructor(node: &AstNode<'_>) -> bool {
    matches!(
        node.kind(),
        AstKind::MethodDefinition(MethodDefinition { kind: MethodDefinitionKind::Constructor, .. })
    )
}

/// Checks to see if the given node is in the root of a constructor.
/// Returns `None` if it isn't possible for this node to be in a constructor.
fn is_in_constructor_root(ctx: &LintContext, node_id: AstNodeId) -> Option<bool> {
    ctx.nodes().ancestors(node_id).nth(3).map(|id| ctx.nodes().get_node(id)).map(is_constructor)
}

fn is_definitely_in_constructor(ctx: &LintContext, node_id: AstNodeId) -> bool {
    ctx.nodes()
        .ancestors(node_id)
        .map(|id| ctx.nodes().get_node(id))
        .skip_while(|node| !node.kind().is_function_like())
        .nth(1)
        .is_some_and(is_constructor)
}

Obviously, it isn't as good as what you've been doing here, And we might want to switch back to that when we have the improved control flow, But I suspect that the snippet above should perform OKish at least for now.

Edit:

Bug fix; Please use this function instead:

fn is_in_constructor_root(ctx: &LintContext, node_id: AstNodeId) -> Option<bool> {
    ctx.nodes()
        .ancestors(node_id)
        .map(|id| ctx.nodes().get_node(id))
        .filter(|it| !matches!(it.kind(), AstKind::BlockStatement(_)))
        .nth(3)
        .map(is_constructor)
}

And also add something like this to test cases:

class C { constructor() { { { return } } } }

The above code works well, but the test case class C { constructor() { { { return } } } } is fails.

The test case should be correct, see the following eslint example:

…to feat/no-constructor-return

rzvxa · 2024-05-22T14:29:35Z

The simpler form of this was already present in your cases - although it was commented out - and in the original eslint plugin. However, You are right that the case passes in eslint.

But do these 2 have a real difference? Why should one pass and the other one fail? There are other rules that would point out the redundant block and return statement, Is it possible that it is just a dormant bug happening in a stupid edge case that nobody noticed?

class C { constructor() { return } }
class C { constructor() { { { return } } } }

Well in either case you can just ignore the edit and it would pass the test.

woai3c · 2024-05-22T15:26:12Z

The simpler form of this was already present in your cases - although it was commented out - and in the original eslint plugin. However, You are right that the case passes in eslint.

But do these 2 have a real difference? Why should one pass and the other one fail? There are other rules that would point out the redundant block and return statement, Is it possible that it is just a dormant bug happening in a stupid edge case that nobody noticed?
class C { constructor() { return } }
class C { constructor() { { { return } } } }
Well in either case you can just ignore the edit and it would pass the test.

Yes, I agree with your opinion. I believe the behavior of the two test cases is the same. Therefore, we can remove the use case class C { constructor() { { { return } } } }.

woai3c · 2024-05-22T15:37:34Z

@rzvxa I have replaced the code with your suggestion. And the performance is ok. Could you please check if there are other any other issues?

rzvxa · 2024-05-22T16:00:30Z

It might actually be a bug, although a different one.
So if you don't mind let's wait to see if this behavior is going to change or not. On the bright side, we might get to ship this with the new cfg, It may or may not perform better; Worth a benchmark in my opinion.

woai3c · 2024-05-22T16:26:52Z

It might actually be a bug, although a different one. So if you don't mind let's wait to see if this behavior is going to change or not. On the bright side, we might get to ship this with the new cfg, It may or may not perform better; Worth a benchmark in my opinion.

ok, I have seen the discussion about the rule. It seems that there are different opinions within the ESLint team.

rzvxa · 2024-05-22T16:37:46Z

Therefore, we can remove the use case class C { constructor() { { { return } } } }.

We can't for the failing case, They result in different AST/CFG and we have to consider them, Preferably we would want more than one layer of nested blocks to ensure that the implementation is handling nested blocks correctly and doesn't just pass with shallow blocks.
If it ends up as a passing test, Then yes it isn't that necessary.

rzvxa · 2024-05-28T11:56:46Z

@woai3c Hello, How are you doing my friend? I hope you've been having an amazing day!
I just wanted to inform you about the recent progress regarding this rule. The ESLint team decided to go with allowing all implicit undefined returns from constructors, There are some changes in passing and failing cases for this rule which you can check out here.

So if you implement the required changes I can approve this PR for merge.

There is also #3381 which is almost ready; It provides AstNodeId appropriate for each CFG instruction so with the help of this you can do what you wanted originally and implement it using CFG alone.

The decision to wait for the new CFG to merge or move on with this rule as is and later on expect it to be refactored to use CFG(either by you or somebody else) is yours.

woai3c · 2024-05-29T08:03:02Z

@woai3c Hello, How are you doing my friend? I hope you've been having an amazing day! I just wanted to inform you about the recent progress regarding this rule. The ESLint team decided to go with allowing all implicit undefined returns from constructors, There are some changes in passing and failing cases for this rule which you can check out here.

So if you implement the required changes I can approve this PR for merge.

There is also #3381 which is almost ready; It provides AstNodeId appropriate for each CFG instruction so with the help of this you can do what you wanted originally and implement it using CFG alone.

The decision to wait for the new CFG to merge or move on with this rule as is and later on expect it to be refactored to use CFG(either by you or somebody else) is yours.

Thank you for the update! That's great news. I have already implemented the required changes. If you have some time, could you please check the code for any other issues?

crates/oxc_linter/src/rules/eslint/no_constructor_return.rs

woai3c · 2024-05-30T05:27:59Z

@rzvxa Hi, rzvxa. I have solved the issue you reviewed. Thank you for your feedback.

graphite-app · 2024-05-30T09:01:41Z

Merge activity

May 30, 5:01 AM EDT: The merge label 'merge' was detected. This PR will be added to the Graphite merge queue once it meets the requirements.
May 30, 5:01 AM EDT: rzvxa added this pull request to the Graphite merge queue.
May 30, 5:02 AM EDT: The Graphite merge queue wasn't able to merge this pull request due to Stack merges are not currently supported for forked repositories. Please create a branch in the target repository in order to merge.

rzvxa · 2024-05-30T09:02:06Z

@rzvxa Hi, rzvxa. I have solved the issue you reviewed. Thank you for your feedback.

Thank you❤️

## [0.4.3] - 2024-06-07 ### Features - 1fb9d23 linter: Add fixer for no-useless-fallback-in-spread rule (#3544) (Don Isaac) - 6506d08 linter: Add fixer for no-single-promise-in-promise-methods (#3531) (Don Isaac) - daf559f linter: Eslint-plugin-jest/no-large-snapshot (#3436) (cinchen) - 4c17bc6 linter: Eslint/no-constructor-return (#3321) (谭光志) - 4a075cc linter/jsdoc: Implement require-param rule (#3554) (Yuji Sugiura) - 747500a linter/jsdoc: Implement require-returns-type rule (#3458) (Yuji Sugiura) - 6b39654 linter/tree-shaking: Support options (#3504) (Wang Wenzhe) - 0cdb45a oxc_codegen: Preserve annotate comment (#3465) (IWANABETHATGUY) ### Bug Fixes - b188778 linter/eslint: Fix `require-await` false positives in `ForOfStatement`. (#3457) (rzvxa) - 350cd91 parser: Should parser error when function declaration has no name (#3461) (Dunqing) Co-authored-by: Boshen <Boshen@users.noreply.github.com>

github-actions bot added the A-linter Area - Linter label May 17, 2024

feat(linter): eslint/no-constructor-return

db22370

woai3c force-pushed the feat/no-constructor-return branch from 542f58d to db22370 Compare May 21, 2024 04:59

woai3c added 3 commits May 22, 2024 22:12

Merge branch 'main' into feat/no-constructor-return

1abd9ea

chore: warning -> warn

b1bf2b7

Merge branch 'feat/no-constructor-return' of github.com:woai3c/oxc in…

d5dcd04

…to feat/no-constructor-return

chore: remove one test case.

3ebcb51

chore: remove clippy warning.

54e284f

chore: allow implicit undefined return statement.

97dd462

rzvxa reviewed May 29, 2024

View reviewed changes

crates/oxc_linter/src/rules/eslint/no_constructor_return.rs Outdated Show resolved Hide resolved

chore: remove unnecessary "is_in_constructor_root" fn.

dbb0686

rzvxa added the 0-merge Merge with Graphite Merge Queue label May 30, 2024

graphite-app bot removed the 0-merge Merge with Graphite Merge Queue label May 30, 2024

Merge branch 'main' into feat/no-constructor-return

12bdddc

rzvxa enabled auto-merge (squash) May 30, 2024 09:04

rzvxa merged commit 4c17bc6 into oxc-project:main May 30, 2024
21 checks passed

This was referenced Jun 6, 2024

Release oxlint v0.4.3 #3558

Closed

Release oxlint v0.4.3 #3573

Closed

Release oxlint v0.4.3 #3574

Closed

Release oxlint v0.4.3 #3575

Closed

Release oxlint v0.4.3 #3581

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(linter): eslint/no-constructor-return #3321

feat(linter): eslint/no-constructor-return #3321

woai3c commented May 17, 2024

graphite-app bot commented May 17, 2024

codspeed-hq bot commented May 17, 2024 •

edited

Loading

Boshen commented May 17, 2024

woai3c commented May 17, 2024

Boshen commented May 17, 2024

woai3c commented May 21, 2024

rzvxa commented May 21, 2024

woai3c commented May 22, 2024

rzvxa commented May 22, 2024 •

edited

Loading

rzvxa commented May 22, 2024

woai3c commented May 22, 2024

Edit:

rzvxa commented May 22, 2024 •

edited

Loading

woai3c commented May 22, 2024

woai3c commented May 22, 2024

rzvxa commented May 22, 2024 •

edited

Loading

woai3c commented May 22, 2024

rzvxa commented May 22, 2024 •

edited

Loading

rzvxa commented May 28, 2024

woai3c commented May 29, 2024 •

edited

Loading

woai3c commented May 30, 2024 •

edited

Loading

graphite-app bot commented May 30, 2024 •

edited

Loading

rzvxa commented May 30, 2024

feat(linter): eslint/no-constructor-return #3321

feat(linter): eslint/no-constructor-return #3321

Conversation

woai3c commented May 17, 2024

graphite-app bot commented May 17, 2024

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

codspeed-hq bot commented May 17, 2024 • edited Loading

CodSpeed Performance Report

Merging #3321 will not alter performance

Summary

Boshen commented May 17, 2024

woai3c commented May 17, 2024

Boshen commented May 17, 2024

woai3c commented May 21, 2024

rzvxa commented May 21, 2024

woai3c commented May 22, 2024

rzvxa commented May 22, 2024 • edited Loading

Edit:

rzvxa commented May 22, 2024

woai3c commented May 22, 2024

Edit:

rzvxa commented May 22, 2024 • edited Loading

woai3c commented May 22, 2024

woai3c commented May 22, 2024

rzvxa commented May 22, 2024 • edited Loading

woai3c commented May 22, 2024

rzvxa commented May 22, 2024 • edited Loading

rzvxa commented May 28, 2024

woai3c commented May 29, 2024 • edited Loading

woai3c commented May 30, 2024 • edited Loading

graphite-app bot commented May 30, 2024 • edited Loading

Merge activity

rzvxa commented May 30, 2024

codspeed-hq bot commented May 17, 2024 •

edited

Loading

rzvxa commented May 22, 2024 •

edited

Loading

rzvxa commented May 22, 2024 •

edited

Loading

rzvxa commented May 22, 2024 •

edited

Loading

rzvxa commented May 22, 2024 •

edited

Loading

woai3c commented May 29, 2024 •

edited

Loading

woai3c commented May 30, 2024 •

edited

Loading

graphite-app bot commented May 30, 2024 •

edited

Loading