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

InlineControlStructure CBF issue while adding braces to an if thats returning a nested function #1590

Closed
hchow77 opened this issue Aug 4, 2017 · 3 comments
Milestone

Comments

@hchow77
Copy link

hchow77 commented Aug 4, 2017

Not very happy with how this code came into being, but does seem to have resulted in a pretty bad CBF fix while using Generic.ControlStructures.InlineControlStructure.NotAllowed. Can't really put it into words because it's so hairy, but here's what it starts out as:

class Clazz {
    public function foo() {
        $num = 0;
        switch ($num) {
            case 0:
                if (1 > $num)
                    return bar(
                        baz(
                            "foobarbaz"
                        )
                    );
                break;

            default:
                return bar("foobarbaz");
        }
    }
}

After running I get:

class Clazz {
    public function foo() {
        $num = 0;
        switch ($num) {
            case 0:
                if (1 > $num) {
                    return;
                } bar(
                        baz(
                            "foobarbaz"
                        )
                    );
                break;

            default:
                return bar("foobarbaz");
        }
    }
}

Note, it's no longer returning the result of bar, which seems pretty bad. Given the code was awful to begin with, and noticed that making fine adjustments that would have made this code better makes it work right, but I'm surprised that CBF is sneaking in this semicolon in this case in particular.

Appreciate the consideration!

@gsherwood
Copy link
Member

It looks like the tokenizer thinks the return keyword is the closing statement of the case. So the inline control structure sniff thinks the return keyword is where the if statement should end as well.

So I think the problem is inside the tokenizer itself, although I'm not really sure how to fix it. Inline control structures seem to break everything, all the time :)

@gsherwood gsherwood added this to the 3.2.0 milestone Aug 7, 2017
gsherwood added a commit that referenced this issue Nov 9, 2017
…s to an if thats returning a nested function
@gsherwood
Copy link
Member

The reason this was failing is because the tokenizer was stopping its search for the IF opener after 3 lines. I've got other code blocks that should stop that search early when they find a semicolon or another scope opener, but neither was found in this specific case.

I've increased the search to 30 lines instead of making it unlimited just because it was originally introduced for extreme edge cases that caused bad performance. I may consider removing it in the future all together.

Thanks for the report.

@hchow77
Copy link
Author

hchow77 commented Nov 22, 2017

Pulled the updated phpcs code and ran it on the original code that caused my issue and it worked great. Thanks for getting this fix in!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants