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

linter: improve method_exists handling #364

Merged
merged 1 commit into from
Mar 20, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 19 additions & 1 deletion src/linter/block.go
Original file line number Diff line number Diff line change
Expand Up @@ -950,7 +950,11 @@ func (b *BlockWalker) handleMethodCall(e *expr.MethodCall) bool {
e.Method.Walk(b)

if !foundMethod && !magic && !b.r.st.IsTrait && !b.isThisInsideClosure(e.Variable) {
b.r.Report(e.Method, LevelError, "undefined", "Call to undefined method {%s}->%s()", exprType, methodName)
// The method is undefined but we permit calling it if `method_exists`
// was called prior to that call.
if !b.ctx.customMethodExists(e.Variable, methodName) {
b.r.Report(e.Method, LevelError, "undefined", "Call to undefined method {%s}->%s()", exprType, methodName)
}
} else {
// Method is defined.

Expand Down Expand Up @@ -1471,6 +1475,18 @@ type andWalker struct {

func (a *andWalker) EnterNode(w walker.Walkable) (res bool) {
switch n := w.(type) {
case *expr.FunctionCall:
args := n.ArgumentList.Arguments
nm, ok := n.Function.(*name.Name)
if ok && meta.NameEquals(nm, `method_exists`) && len(args) == 2 {
obj := args[0].(*node.Argument).Expr
methodName := args[1].(*node.Argument).Expr
lit, ok := methodName.(*scalar.String)
if ok {
a.b.ctx.addCustomMethod(obj, unquote(lit.Value))
}
}

case *binary.BooleanAnd:
return true

Expand Down Expand Up @@ -1556,11 +1572,13 @@ func (b *BlockWalker) handleVariable(v node.Node) bool {

func (b *BlockWalker) handleIf(s *stmt.If) bool {
var varsToDelete []node.Node
customMethods := len(b.ctx.customMethods)
// Remove all isset'ed variables after we're finished with this if statement.
defer func() {
for _, v := range varsToDelete {
b.ctx.sc.DelVar(v, "isset/!empty")
}
b.ctx.customMethods = b.ctx.customMethods[:customMethods]
}()
walkCond := func(cond node.Node) {
a := &andWalker{b: b}
Expand Down
25 changes: 25 additions & 0 deletions src/linter/block_context.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,9 +2,15 @@ package linter

import (
"github.com/VKCOM/noverify/src/meta"
"github.com/VKCOM/noverify/src/php/parser/node"
"github.com/VKCOM/noverify/src/solver"
)

type customMethod struct {
obj node.Node
name string
}

// blockContext is a state that is used to hold inner blocks info.
//
// When BlockWalker enters another block, new context is created.
Expand All @@ -25,6 +31,24 @@ type blockContext struct {
// having for loop outside of that switch.
insideLoop bool
customTypes []solver.CustomType

customMethods []customMethod
}

func (ctx *blockContext) addCustomMethod(obj node.Node, methodName string) {
ctx.customMethods = append(ctx.customMethods, customMethod{
obj: obj,
name: methodName,
})
}

func (ctx *blockContext) customMethodExists(obj node.Node, methodName string) bool {
for _, m := range ctx.customMethods {
if m.name == methodName && solver.NodeAwareDeepEqual(m.obj, obj) {
return true
}
}
return false
}

// copyBlockContext returns a copy of the context.
Expand All @@ -34,6 +58,7 @@ func copyBlockContext(ctx *blockContext) *blockContext {
return &blockContext{
sc: ctx.sc.Clone(),
customTypes: append([]solver.CustomType{}, ctx.customTypes...),
customMethods: append([]customMethod{}, ctx.customMethods...),
innermostLoop: ctx.innermostLoop,
insideLoop: ctx.insideLoop,
}
Expand Down
71 changes: 71 additions & 0 deletions src/linttest/regression_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -880,6 +880,77 @@ func TestIssue183(t *testing.T) {
test.RunAndMatch()
}

func TestIssue362_1(t *testing.T) {
linttest.SimpleNegativeTest(t, `<?php
function method_exists($object, $method_name) { return 1 == 1; }

class Foo {
public $value;
}

$x = new Foo();
if (method_exists($x, 'm1')) {
$x->m1();
$x->m1(1, 2);
}

if (method_exists(new Foo(), 'm2')) {
if (method_exists(new Foo(), 'm3')) {
(new Foo())->m2((new Foo())->m3());
}
(new Foo())->m2();
}

$y = new Foo();
if (method_exists($x, 'x1')) {
$x->x1();
} elseif (method_exists($y, 'y1')) {
$foo = $y->y1();
if ($foo instanceof Foo) {
echo $foo->value;
}
}
`)
}

func TestIssue362_2(t *testing.T) {
test := linttest.NewSuite(t)
test.AddFile(`<?php
function method_exists($object, $method_name) { return 1 == 1; }

class Foo {}

$x = new Foo();
if (method_exists($x, 'm1')) {
}
$x->m1(); // Bad: called outside of if

if (method_exists(new Foo(), 'm2')) {
if (method_exists(new Foo(), 'm3')) {
$x->m2(); // Bad: called a method on a different object expression
}
}
(new Foo())->m3(); // Bad: called outside of if

$y = new Foo();
if (method_exists($x, 'x1')) {
$x->y1();
} elseif (method_exists($y, 'y1')) {
$v = $y->x1();
$v->foo();
}
`)
test.Expect = []string{
`Call to undefined method {\Foo}->m1()`,
`Call to undefined method {\Foo}->m2()`,
`Call to undefined method {\Foo}->m3()`,
`Call to undefined method {\Foo}->y1()`,
`Call to undefined method {\Foo}->x1()`,
`Call to undefined method {mixed}->foo()`,
}
test.RunAndMatch()
}

func TestIssue252(t *testing.T) {
linttest.SimpleNegativeTest(t, `<?php
class Foo {
Expand Down
2 changes: 1 addition & 1 deletion src/solver/exprtype.go
Original file line number Diff line number Diff line change
Expand Up @@ -179,7 +179,7 @@ func exprTypeLocalCustom(sc *meta.Scope, cs *meta.ClassParseState, n node.Node,
}

for _, c := range custom {
if nodeAwareDeepEqual(c.Node, n) {
if NodeAwareDeepEqual(c.Node, n) {
return c.Typ
}
}
Expand Down
8 changes: 4 additions & 4 deletions src/solver/node_equal.go
Original file line number Diff line number Diff line change
Expand Up @@ -8,9 +8,9 @@ import (
"github.com/VKCOM/noverify/src/php/parser/position"
)

// Like reflect.DeepEqual but knows how to compare node.Node by ignoring
// NodeAwareDeepEqual is a reflect.DeepEqual but knows how to compare node.Node by ignoring
// freefloating text and positions
func nodeAwareDeepEqual(a, b interface{}) bool {
func NodeAwareDeepEqual(a, b interface{}) bool {
if a == nil || b == nil {
return a == b
}
Expand Down Expand Up @@ -57,7 +57,7 @@ func nodeDeepEqual(a, b node.Node) bool {
continue
}

if !nodeAwareDeepEqual(f1.Interface(), f2.Interface()) {
if !NodeAwareDeepEqual(f1.Interface(), f2.Interface()) {
return false
}
}
Expand All @@ -72,7 +72,7 @@ func nodeSliceDeepEqual(a, b []node.Node) bool {
}

for i, n := range a {
if !nodeAwareDeepEqual(n, b[i]) {
if !NodeAwareDeepEqual(n, b[i]) {
return false
}
}
Expand Down