Repository: VKCOM/noverify
Pull request #772 (2020-09-08 19:42:10)

ir: eagerly parse phpdoc in irconv
The irconv package will now attempt to parse phpdoc for common
nodes like function/method/class statements if
Converter was created with phpdoc types parser.
(This API may change in the future!)

We need to do eager parsing in 99% of cases.
The only case where we don't want to do it
are phpgrep-like utils as well as some php-guru
parts that are not interested in phpdoc comments at all.

All packages that were parsing raw strings are changed to
use []phpdoc.CommentPart directly.

I returned irconv.ConvertNode() helper function to
make it easier to make AST->IR conversion in places
where we are too lazy to reuse the converter or
we don't care about their performance (some of these
places can be fixed later if we ever want to).

The main benefit of the eager phpdoc parsing is that
custom (external) checkers don't have to repeat all
phpdoc parsing efforts. Right now these checkers need
to re-parse class/method/function phpdoc strings if
they want to do something fancy with them. With this
change, we can access the parsed structures directly
from both internal and external checkers.

Signed-off-by: Iskander Sharipov <quasilyte@gmail.com>


Patch:
diff --git a/src/cmd/php-guru/dupcode/main.go b/src/cmd/php-guru/dupcode/main.go
index d4a0971..ba23a5d 100644
--- a/src/cmd/php-guru/dupcode/main.go
+++ b/src/cmd/php-guru/dupcode/main.go
@@ -95,7 +95,7 @@ func Main(ctx *guru.Context) (int, error) {
 	wg.Add(nworkers)
 	for i := 0; i < nworkers; i++ {
 		go func(workerID int) {
-			irConverter := irconv.NewConverter()
+			irConverter := irconv.NewConverter(nil)
 			workerResult := make(funcSet)
 			for f := range filenamesCh {
 				data, err := ioutil.ReadFile(f.Filename)
diff --git a/src/ir/helper_types.go b/src/ir/helper_types.go
index 073f32c..1fe4288 100644
--- a/src/ir/helper_types.go
+++ b/src/ir/helper_types.go
@@ -1,5 +1,9 @@
 package ir
 
+import (
+	"github.com/VKCOM/noverify/src/phpdoc"
+)
+
 // Helper types are not real nodes, they're usually used
 // to express some structure that is common between several nodes.
 //
@@ -9,6 +13,7 @@ package ir
 // It doesn't include positions/freefloating info.
 type Class struct {
 	PhpDocComment string
+	PhpDoc        []phpdoc.CommentPart
 	Extends       *ClassExtendsStmt
 	Implements    *ClassImplementsStmt
 	Stmts         []Node
diff --git a/src/ir/irconv/irconv.go b/src/ir/irconv/irconv.go
index b98b6f4..5864649 100644
--- a/src/ir/irconv/irconv.go
+++ b/src/ir/irconv/irconv.go
@@ -13,14 +13,30 @@ import (
 	"github.com/VKCOM/noverify/src/php/parser/node/name"
 	"github.com/VKCOM/noverify/src/php/parser/node/scalar"
 	"github.com/VKCOM/noverify/src/php/parser/node/stmt"
+	"github.com/VKCOM/noverify/src/phpdoc"
 )
 
+func ConvertNode(n node.Node) ir.Node {
+	c := NewConverter(phpdoc.NewTypeParser())
+	return c.ConvertNode(n)
+}
+
 type Converter struct {
 	namespace string
+
+	phpdocTypeParser *phpdoc.TypeParser
 }
 
-func NewConverter() *Converter {
-	return &Converter{}
+// NewConverter returns a new AST->IR converter.
+//
+// If typeParser is nil, it will not eagerly try to parse phpdoc
+// strings into phpdoc.CommentPart.
+//
+// It's intended to be re-used inside a signle thread context.
+func NewConverter(typeParser *phpdoc.TypeParser) *Converter {
+	return &Converter{
+		phpdocTypeParser: typeParser,
+	}
 }
 
 func (c *Converter) ConvertRoot(n *node.Root) *ir.Root {
@@ -641,6 +657,7 @@ func (c *Converter) convNode(n node.Node) ir.Node {
 		out.ReturnsRef = n.ReturnsRef
 		out.Static = n.Static
 		out.PhpDocComment = n.PhpDocComment
+		out.PhpDoc = c.parsePHPDoc(n.PhpDocComment)
 		out.Params = c.convNodeSlice(n.Params)
 		out.ClosureUse = c.convNode(n.ClosureUse).(*ir.ClosureUseExpr)
 		out.ReturnType = c.convNode(n.ReturnType)
@@ -1218,6 +1235,7 @@ func (c *Converter) convNode(n node.Node) ir.Node {
 		out.Position = n.Position
 		out.ReturnsRef = n.ReturnsRef
 		out.PhpDocComment = n.PhpDocComment
+		out.PhpDoc = c.parsePHPDoc(n.PhpDocComment)
 		out.MethodName = c.convNode(n.MethodName).(*ir.Identifier)
 		{
 			slice := make([]*ir.Identifier, len(n.Modifiers))
@@ -1387,6 +1405,7 @@ func (c *Converter) convNode(n node.Node) ir.Node {
 		out.Position = n.Position
 		out.ReturnsRef = n.ReturnsRef
 		out.PhpDocComment = n.PhpDocComment
+		out.PhpDoc = c.parsePHPDoc(n.PhpDocComment)
 		out.FunctionName = c.convNode(n.FunctionName).(*ir.Identifier)
 		out.Params = c.convNodeSlice(n.Params)
 		out.ReturnType = c.convNode(n.ReturnType)
@@ -1522,6 +1541,7 @@ func (c *Converter) convNode(n node.Node) ir.Node {
 		out.FreeFloating = n.FreeFloating
 		out.Position = n.Position
 		out.PhpDocComment = n.PhpDocComment
+		out.PhpDoc = c.parsePHPDoc(n.PhpDocComment)
 		out.Variable = c.convNode(n.Variable).(*ir.SimpleVar)
 		out.Expr = c.convNode(n.Expr)
 		return out
@@ -1768,6 +1788,7 @@ func (c *Converter) convCastExpr(n, e node.Node, typ string) *ir.TypeCastExpr {
 func (c *Converter) convClass(n *stmt.Class) ir.Node {
 	class := ir.Class{
 		PhpDocComment: n.PhpDocComment,
+		PhpDoc:        c.parsePHPDoc(n.PhpDocComment),
 		Extends:       c.convNode(n.Extends).(*ir.ClassExtendsStmt),
 		Implements:    c.convNode(n.Implements).(*ir.ClassImplementsStmt),
 		Stmts:         c.convNodeSlice(n.Stmts),
@@ -1803,6 +1824,13 @@ func (c *Converter) convClass(n *stmt.Class) ir.Node {
 	return out
 }
 
+func (c *Converter) parsePHPDoc(doc string) []phpdoc.CommentPart {
+	if c.phpdocTypeParser != nil {
+		return phpdoc.Parse(c.phpdocTypeParser, doc)
+	}
+	return nil
+}
+
 func convString(n *scalar.String) ir.Node {
 	out := &ir.String{
 		FreeFloating: n.FreeFloating,
diff --git a/src/ir/node_types.go b/src/ir/node_types.go
index 84826b3..0118c0c 100644
--- a/src/ir/node_types.go
+++ b/src/ir/node_types.go
@@ -3,6 +3,7 @@ package ir
 import (
 	"github.com/VKCOM/noverify/src/php/parser/freefloating"
 	"github.com/VKCOM/noverify/src/php/parser/position"
+	"github.com/VKCOM/noverify/src/phpdoc"
 )
 
 // TODO: make Alt and AltSyntax field names consistent.
@@ -453,6 +454,7 @@ type ClosureExpr struct {
 	ReturnsRef    bool
 	Static        bool
 	PhpDocComment string
+	PhpDoc        []phpdoc.CommentPart
 	Params        []Node
 	ClosureUse    *ClosureUseExpr
 	ReturnType    Node
@@ -908,6 +910,7 @@ type ClassMethodStmt struct {
 	Position      *position.Position
 	ReturnsRef    bool
 	PhpDocComment string
+	PhpDoc        []phpdoc.CommentPart
 	MethodName    *Identifier
 	Modifiers     []*Identifier
 	Params        []Node
@@ -1042,6 +1045,7 @@ type FunctionStmt struct {
 	Position      *position.Position
 	ReturnsRef    bool
 	PhpDocComment string
+	PhpDoc        []phpdoc.CommentPart
 	FunctionName  *Identifier
 	Params        []Node
 	ReturnType    Node
@@ -1150,6 +1154,7 @@ type PropertyStmt struct {
 	FreeFloating  freefloating.Collection
 	Position      *position.Position
 	PhpDocComment string
+	PhpDoc        []phpdoc.CommentPart
 	Variable      *SimpleVar
 	Expr          Node
 }
diff --git a/src/langsrv/langsrv.go b/src/langsrv/langsrv.go
index ebefc96..0d67642 100644
--- a/src/langsrv/langsrv.go
+++ b/src/langsrv/langsrv.go
@@ -832,7 +832,7 @@ func getMethodCompletionItems(st *meta.ClassParseState, str string, sc *meta.Sco
 		lintdebug.Send("Could not parse %s", strTemp)
 		return nil
 	}
-	rootIR := irconv.NewConverter().ConvertRoot(tempNode)
+	rootIR := irconv.ConvertNode(tempNode).(*ir.Root)
 
 	stmtLst := rootIR.Stmts
 	if len(stmtLst) == 0 {
diff --git a/src/langsrv/refs.go b/src/langsrv/refs.go
index 40034e6..071f802 100644
--- a/src/langsrv/refs.go
+++ b/src/langsrv/refs.go
@@ -224,7 +224,7 @@ func findReferences(substr string, parse parseFn) []vscode.Location {
 						parser.Parse()
 
 						rootNode := parser.GetRootNode()
-						rootIR := irconv.NewConverter().ConvertRoot(rootNode)
+						rootIR := irconv.ConvertNode(rootNode).(*ir.Root)
 						if rootNode != nil {
 							var found []vscode.Location
 							func() {
diff --git a/src/linter/block.go b/src/linter/block.go
index f9fa1b5..93bc79d 100644
--- a/src/linter/block.go
+++ b/src/linter/block.go
@@ -1203,7 +1203,7 @@ func (b *BlockWalker) enterClosure(fun *ir.ClosureExpr, haveThis bool, thisType
 		sc.AddVarName("this", meta.NewTypesMap("possibly_late_bound"), "possibly late bound $this", meta.VarAlwaysDefined)
 	}
 
-	doc := b.r.parsePHPDoc(fun, fun.PhpDocComment, fun.Params)
+	doc := b.r.parsePHPDoc(fun, fun.PhpDoc, fun.Params)
 	b.r.reportPhpdocErrors(fun, doc.errs)
 	phpDocParamTypes := doc.types
 
diff --git a/src/linter/root.go b/src/linter/root.go
index aec1815..9829621 100644
--- a/src/linter/root.go
+++ b/src/linter/root.go
@@ -241,7 +241,7 @@ func (d *RootWalker) EnterNode(n ir.Node) (res bool) {
 		}
 		d.checkCommentMisspellings(n.ClassName, n.PhpDocComment)
 		d.checkIdentMisspellings(n.ClassName)
-		doc := d.parseClassPHPDoc(n.ClassName, n.PhpDocComment)
+		doc := d.parseClassPHPDoc(n.ClassName, n.PhpDoc)
 		d.reportPhpdocErrors(n.ClassName, doc.errs)
 		// If we ever need to distinguish @property-annotated and real properties,
 		// more work will be required here.
@@ -901,7 +901,7 @@ func (d *RootWalker) enterPropertyList(pl *ir.PropertyListStmt) bool {
 		nm := p.Variable.Name
 
 		d.checkCommentMisspellings(p, p.PhpDocComment)
-		typ := d.parsePHPDocVar(p, p.PhpDocComment)
+		typ := d.parsePHPDocVar(p, p.PhpDoc)
 		if p.Expr != nil {
 			typ = typ.Append(solver.ExprTypeLocal(d.scope(), d.ctx.st, p.Expr))
 		}
@@ -1006,7 +1006,7 @@ func (d *RootWalker) enterClassMethod(meth *ir.ClassMethodStmt) bool {
 	for _, p := range meth.Params {
 		d.checkVarnameMisspellings(p, p.(*ir.Parameter).Variable.Name)
 	}
-	doc := d.parsePHPDoc(meth.MethodName, meth.PhpDocComment, meth.Params)
+	doc := d.parsePHPDoc(meth.MethodName, meth.PhpDoc, meth.Params)
 	d.reportPhpdocErrors(meth.MethodName, doc.errs)
 	phpdocReturnType := doc.returnType
 	phpDocParamTypes := doc.types
@@ -1113,8 +1113,8 @@ func (d *RootWalker) reportPhpdocErrors(n ir.Node, errs phpdocErrors) {
 	}
 }
 
-func (d *RootWalker) parsePHPDocVar(n ir.Node, doc string) (m meta.TypesMap) {
-	for _, part := range phpdoc.Parse(d.ctx.phpdocTypeParser, doc) {
+func (d *RootWalker) parsePHPDocVar(n ir.Node, doc []phpdoc.CommentPart) (m meta.TypesMap) {
+	for _, part := range doc {
 		d.checkPHPDocRef(n, part)
 		part, ok := part.(*phpdoc.TypeVarCommentPart)
 		if ok && part.Name() == "var" {
@@ -1263,10 +1263,10 @@ func (d *RootWalker) checkPHPDocRef(n ir.Node, part phpdoc.CommentPart) {
 	}
 }
 
-func (d *RootWalker) parsePHPDoc(n ir.Node, doc string, actualParams []ir.Node) phpDocParseResult {
+func (d *RootWalker) parsePHPDoc(n ir.Node, doc []phpdoc.CommentPart, actualParams []ir.Node) phpDocParseResult {
 	var result phpDocParseResult
 
-	if doc == "" {
+	if len(doc) == 0 {
 		return result
 	}
 
@@ -1280,7 +1280,7 @@ func (d *RootWalker) parsePHPDoc(n ir.Node, doc string, actualParams []ir.Node)
 
 	var curParam int
 
-	for _, part := range phpdoc.Parse(d.ctx.phpdocTypeParser, doc) {
+	for _, part := range doc {
 		d.checkPHPDocRef(n, part)
 
 		if part.Name() == "deprecated" {
@@ -1532,7 +1532,7 @@ func (d *RootWalker) enterFunction(fun *ir.FunctionStmt) bool {
 	for _, p := range fun.Params {
 		d.checkVarnameMisspellings(p, p.(*ir.Parameter).Variable.Name)
 	}
-	doc := d.parsePHPDoc(fun.FunctionName, fun.PhpDocComment, fun.Params)
+	doc := d.parsePHPDoc(fun.FunctionName, fun.PhpDoc, fun.Params)
 	d.reportPhpdocErrors(fun.FunctionName, doc.errs)
 	phpdocReturnType := doc.returnType
 	phpDocParamTypes := doc.types
@@ -1985,10 +1985,10 @@ func (d *RootWalker) nodeText(n ir.Node) []byte {
 	return d.fileContents[pos.StartPos:pos.EndPos]
 }
 
-func (d *RootWalker) parseClassPHPDoc(n ir.Node, doc string) classPhpDocParseResult {
+func (d *RootWalker) parseClassPHPDoc(n ir.Node, doc []phpdoc.CommentPart) classPhpDocParseResult {
 	var result classPhpDocParseResult
 
-	if doc == "" {
+	if len(doc) == 0 {
 		return result
 	}
 
@@ -1998,7 +1998,7 @@ func (d *RootWalker) parseClassPHPDoc(n ir.Node, doc string) classPhpDocParseRes
 	result.properties = make(meta.PropertiesMap)
 	result.methods = meta.NewFunctionsMap()
 
-	for _, part := range phpdoc.Parse(d.ctx.phpdocTypeParser, doc) {
+	for _, part := range doc {
 		d.checkPHPDocRef(n, part)
 		switch part.Name() {
 		case "property", "property-read", "property-write":
diff --git a/src/linter/worker.go b/src/linter/worker.go
index 26494fb..7495469 100644
--- a/src/linter/worker.go
+++ b/src/linter/worker.go
@@ -58,10 +58,12 @@ func NewIndexingWorker(id int) *Worker {
 }
 
 func newWorker(id int) *Worker {
+	ctx := NewWorkerContext()
+	irConverter := irconv.NewConverter(ctx.phpdocTypeParser)
 	return &Worker{
 		id:     id,
-		ctx:    NewWorkerContext(),
-		irconv: irconv.NewConverter(),
+		ctx:    ctx,
+		irconv: irConverter,
 		reParserNoLiterals: syntax.NewParser(&syntax.ParserOptions{
 			NoLiterals: true,
 		}),
diff --git a/src/phpgrep/compile.go b/src/phpgrep/compile.go
index 2e83497..a965a6f 100644
--- a/src/phpgrep/compile.go
+++ b/src/phpgrep/compile.go
@@ -21,7 +21,7 @@ func compile(opts *Compiler, pattern []byte) (*Matcher, error) {
 	if err != nil {
 		return nil, err
 	}
-	rootIR := irconv.NewConverter().ConvertNode(root)
+	rootIR := irconv.ConvertNode(root)
 
 	if st, ok := rootIR.(*ir.ExpressionStmt); ok {
 		rootIR = st.Expr
diff --git a/src/rules/parser.go b/src/rules/parser.go
index 28fd5a6..97c47c5 100644
--- a/src/rules/parser.go
+++ b/src/rules/parser.go
@@ -69,7 +69,7 @@ func (p *parser) parse(filename string, r io.Reader) (*Set, error) {
 	if err != nil {
 		return res, err
 	}
-	rootIR := irconv.NewConverter().ConvertRoot(root)
+	rootIR := irconv.ConvertNode(root).(*ir.Root)
 
 	// Convert PHP file into the rule set.
 	p.filename = filename
@@ -371,11 +371,11 @@ func (p *parser) parseRule(st ir.Node, proto *Rule) error {
 }
 
 func (p *parser) parseFuncComment(fn *ir.FunctionStmt) error {
-	if fn.PhpDocComment == "" {
+	if len(fn.PhpDoc) == 0 {
 		return nil
 	}
 	var doc RuleDoc
-	for _, part := range phpdoc.Parse(p.typeParser, fn.PhpDocComment) {
+	for _, part := range fn.PhpDoc {
 		part := part.(*phpdoc.RawCommentPart)
 		switch part.Name() {
 		case "comment":


Test patch:
diff --git a/src/ir/irfmt/printer_test.go b/src/ir/irfmt/printer_test.go
index 08506da..c1dc58e 100644
--- a/src/ir/irfmt/printer_test.go
+++ b/src/ir/irfmt/printer_test.go
@@ -124,7 +124,7 @@ func TestPrinterSingleLine(t *testing.T) {
 		if err != nil {
 			t.Fatalf("parse %s: %v", code, err)
 		}
-		rootIR := irconv.NewConverter().ConvertNode(root)
+		rootIR := irconv.ConvertNode(root)
 
 		var buf strings.Builder
 		irfmt.NewPrettyPrinter(&buf, "    ").Print(rootIR)
@@ -587,7 +587,7 @@ endswitch;
 			t.Fatalf("parse %s: %v", code, err)
 		}
 
-		rootIR := irconv.NewConverter().ConvertNode(root)
+		rootIR := irconv.ConvertNode(root)
 
 		var buf strings.Builder
 		irfmt.NewPrettyPrinter(&buf, "    ").Print(rootIR)
diff --git a/src/ir/normalize/normalize_test.go b/src/ir/normalize/normalize_test.go
index 08eb2fc..e788f17 100644
--- a/src/ir/normalize/normalize_test.go
+++ b/src/ir/normalize/normalize_test.go
@@ -104,7 +104,7 @@ func runNormalizeTests(t *testing.T, tests []normalizationTest) {
 
 	conf := Config{}
 	st := &meta.ClassParseState{CurrentClass: `\Foo`}
-	irConverter := irconv.NewConverter()
+	irConverter := irconv.NewConverter(nil)
 	for _, test := range tests {
 		n, _, err := parseutil.ParseStmt([]byte(test.orig))
 		if err != nil {
@@ -228,6 +228,6 @@ func parseStmtList(s string) ([]ir.Node, error) {
 	if len(p.GetErrors()) != 0 {
 		return nil, errors.New(p.GetErrors()[0].String())
 	}
-	rootIR := irconv.NewConverter().ConvertRoot(p.GetRootNode())
+	rootIR := irconv.ConvertNode(p.GetRootNode()).(*ir.Root)
 	return rootIR.Stmts, nil
 }
diff --git a/src/linter/phpdoc_test.go b/src/linter/phpdoc_test.go
index 864a250..4075c58 100644
--- a/src/linter/phpdoc_test.go
+++ b/src/linter/phpdoc_test.go
@@ -5,6 +5,7 @@ import (
 	"testing"
 
 	"github.com/VKCOM/noverify/src/meta"
+	"github.com/VKCOM/noverify/src/phpdoc"
 )
 
 func TestParseClassPHPDoc(t *testing.T) {
@@ -60,7 +61,8 @@ func TestParseClassPHPDoc(t *testing.T) {
 	st := &meta.ClassParseState{}
 	walker := RootWalker{ctx: newRootContext(NewWorkerContext(), st)}
 	for _, test := range tests {
-		doc := fmt.Sprintf(`/** %s */`, test.line)
+		docString := fmt.Sprintf(`/** %s */`, test.line)
+		doc := phpdoc.Parse(walker.ctx.phpdocTypeParser, docString)
 		result := walker.parseClassPHPDoc(nil, doc)
 
 		switch {
diff --git a/src/phpgrep/matcher_test.go b/src/phpgrep/matcher_test.go
index ea16da2..7f21fab 100644
--- a/src/phpgrep/matcher_test.go
+++ b/src/phpgrep/matcher_test.go
@@ -16,7 +16,7 @@ func mustParse(t testing.TB, code string) ir.Node {
 	if err != nil {
 		t.Fatalf("parse `%s`: %v", code, err)
 	}
-	irnode := irconv.NewConverter().ConvertNode(n)
+	irnode := irconv.ConvertNode(n)
 	if n, ok := irnode.(*ir.ExpressionStmt); ok {
 		return n.Expr
 	}