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

Verify if a value is returned for all code paths #3373

Closed
awerlang opened this Issue Aug 12, 2015 · 10 comments

Comments

Projects
None yet
5 participants
@awerlang
Copy link

awerlang commented Aug 12, 2015

I would like to work for a follow-up of #481.

Augment consistent-return rule with an option allCodePaths (off by default).

If it is on, then if a value is returned for a given code path, then a value should be returned for every code path.

These are warning:

if (true) return true;
if (true) return; return true;

These are not:

if (true) return;
if (true) return true; return false;

For this rule, only return <expression>; shall be considered.

Current version: v1.1.0

Pseudo-code:

returnStatus(BlockStatement node):
    status := returnStatus(...node.body);
    case always in status
        Result := always;
    case maybe in status
        Result := maybe;
    case default:
        Result := never;

returnStatus(IfStatement node):
    case returns(node.then) && returns(node.else)
        Result := always;
    case returns(node.then) <> returns(node.else)
        Result := maybe;
    case default:
        Result := never;

returnStatus(ReturnStatement node):
    case node.argument
        Result := always;
    Result := never;

returnStatus(Statement node):
    Result := never;

This listing is not complete for all statements, but will check that.

It is just needed to have returnStatus computed for immediate children, currently I thinking about it. It would be easy to mutate each node in-place, but I haven't see any rule doing that. I plan to maintain it as a simple visitor instead of querying children.

Am I missing something important or there's already a WIP on this?

@eslintbot

This comment has been minimized.

Copy link

eslintbot commented Aug 12, 2015

Thanks for the issue! We get a lot of issues, so this message is automatically posted to each one to help you check that you've included all of the information we need to help you.

Reporting a bug? Please be sure to include:

  1. The version of ESLint you are using (run eslint -v)
  2. The source code that caused the problem
  3. The configuration you're using (for the rule or your entire config file)
  4. The actual ESLint output complete with line numbers

Requesting a new rule? Please be sure to include:

  1. The use case for the rule - what is it trying to prevent or flag?
  2. Whether the rule is trying to prevent an error or is purely stylistic
  3. Why you believe this rule is generic enough to be included

Requesting a feature? Please be sure to include:

  1. The problem you want to solve (don't mention the solution)
  2. Your take on the correct solution to problem

Including this information in your issue helps us to triage it and get you a response as quickly as possible.

Thanks!

@eslintbot eslintbot added the triage label Aug 12, 2015

@nzakas

This comment has been minimized.

Copy link
Member

nzakas commented Aug 12, 2015

We've been unable to find an optimal way to make this work, which is why it hasn't been implemented. The issue is that many rules can benefit from path analysis, so we need to find a way to do it once and allow multiple roles to access the result. Are you interested in investigating that?

@awerlang

This comment has been minimized.

Copy link
Author

awerlang commented Aug 13, 2015

Sure, will investigate.

Doing some basic research, I've found a handful of rules that could benefit from such infrastructure:

  • Non-void functions that don't return a value on all code paths (this proposal);
  • Use of unassigned variables;
  • Unreachable code (no-unreachable, has issues);
  • Variables that are initialized but never used (no-unused-vars).

Some of them are already implemented (in parenthesis above). Some of them need to be aware of returning paths while others are more general (variables stuff).

Another rule already available is no-else-return (doesn't share the issues of no-unreachable).

I'm not sure how specialized or generic this should be. Any ideas?

@nzakas

This comment has been minimized.

Copy link
Member

nzakas commented Aug 13, 2015

Take a look at escope. I'd envision something similar for path analysis.

@mysticatea

This comment has been minimized.

Copy link
Member

mysticatea commented Aug 14, 2015

escope is using esrecurse, this is a library to traverse AST.
eslint is using estraverse.

esrecurse can control timing of traversing child nodes. This is needed for code path analysis.
On the other hand, esrecurse cannot process many eslint rules in traversing once like estraverse. Because esrecurse's logic is recursively.

Now, I'm thinking an utility for path analysis in my eslint plugin.
Maybe it becomes like thing which does map-reduce paths in main traversing.

@awerlang

This comment has been minimized.

Copy link
Author

awerlang commented Aug 17, 2015

esrecurse can control timing of traversing child nodes. This is needed for code path analysis.

Needed...? Can I ask you why?

@nzakas

This comment has been minimized.

Copy link
Member

nzakas commented Aug 17, 2015

We're getting a bit off on a tangent here. It's possible to do path analysis using estraverse (@mysticatea had done it in a rule before), but we need it extract into a component shared by all rules so we can do:

var codePath = espath.getPath(node);
if (codePath.termination.return) {
    // whatever
}
@mysticatea

This comment has been minimized.

Copy link
Member

mysticatea commented Aug 18, 2015

@awerlang I had gone too far in saying. That's an easy way.
For example, IfStatement:

class Analyzer extends esrecurse.Visitor {
    IfStatement(node) {
        this.visit(node.test);

        // enter if block with a path.
        this.enterFork();
        this.visit(node.consequent); 
        this.exitFork();

        // enter else block with a path.
        this.enterFork();
        this.visit(node.alternate);
        this.exitFork();
    }

    enterFork() {}
    exitFork() {}
}

Such above, we can manage paths easily if we can control timing of visiting child nodes.
But this way cannot treat many rules in one traversing. It possibly is a performance issue.

@nzakas When I made constructor-super, I used the same way as esrecurse :)

Now I'm trying to create an utility which treats code paths in eslint's main traversing. I will send a PR after checking whether it's possible.

@nzakas

This comment has been minimized.

Copy link
Member

nzakas commented Aug 18, 2015

Awesome!

@ilyavolodin

This comment has been minimized.

Copy link
Member

ilyavolodin commented Sep 16, 2015

Closing in favor of #3530 (main path analysis issue).

mysticatea added a commit to mysticatea/eslint that referenced this issue Oct 24, 2015

mysticatea added a commit to mysticatea/eslint that referenced this issue Oct 24, 2015

mysticatea added a commit to mysticatea/eslint that referenced this issue Nov 13, 2015

mysticatea added a commit to mysticatea/eslint that referenced this issue Dec 2, 2015

mysticatea added a commit to mysticatea/eslint that referenced this issue Dec 3, 2015

mysticatea added a commit to mysticatea/eslint that referenced this issue Dec 3, 2015

popomore added a commit to eggjs/eslint-config-egg that referenced this issue Mar 4, 2016

@eslint eslint bot locked and limited conversation to collaborators Feb 7, 2018

@eslint eslint bot added the archived due to age label Feb 7, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.