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

fix: literal calls case #909

Merged

Conversation

mfederowicz
Copy link
Contributor

related with issue #902

fix is simple, we should add case *ast.FuncLit in rule: rule/unconditional-recursion.go , and return nil, then test pass.

but at the end I dont know if I should add some additional code as in previous cases for ex: w.updateFuncStatus(n.Body) but if test pass it should be ok :P

@chavacava
Copy link
Collaborator

chavacava commented Sep 26, 2023

Hi @mfederowicz, thanks for the PR.
I added a new test case:

func nr902() {
	go func() {
		nr902() // MATCH /unconditional recursive call/
	}()
}

In this case, the function literal contains an actual unconditional recursive call and the rule should spot it.
To cope with this case, and still avoid false positives as the initial case of this issue, the rule should skip analyzing func literals used as function call arguments.

@mfederowicz mfederowicz force-pushed the fix-unconditional-recursion-test branch from 9c47c81 to 6140e99 Compare September 26, 2023 23:59
@mfederowicz
Copy link
Contributor Author

ok @chavacava with that last one test was a problem with checking line number in: if p.Position.Start.Line != in.Line { statement and signature used to identifate current function:

w.currentFunc = &funcStatus{&funcDesc{rec, n.Name}, false}

after some tweaks, all tests passes

of course you can propose other solution, maye my changes were not optimal :P

FYI: I made cleanup in commits (rebase on branch), so please git pull on your copy

@chavacava chavacava merged commit fb5bbe7 into mgechev:master Sep 30, 2023
@mfederowicz mfederowicz deleted the fix-unconditional-recursion-test branch October 2, 2024 09:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants