Repository: VKCOM/noverify
Pull request #586 (2020-07-15 13:35:08)

linter: fix -allow-disable flag
PR #578 introduced a regression where all files that match
-allow-disable regexp will be ignored. This is not how
allow disable should work, it only allows to disable the
linter in files that match the specified regexp but it doesn't
ignore them if they don't use linter disable annotation.

This PR also removes isDisabled Report field as we can avoid appending
disabled reports instead of trying to filter them out later.

Even when linter is disabled, linterError messages will be reported
as they're our special way to tell that something goes really wrong.

To make allow-disable flag testable, linttest.Suite gets
the AllowDisable regexp field that can be set inside the tests.
It will be forwarded to the parsing functions as one would expect.

Signed-off-by: Iskander Sharipov <quasilyte@gmail.com>


Patch:
diff --git a/src/cmd/main.go b/src/cmd/main.go
index 3466895..1d35ed4 100644
--- a/src/cmd/main.go
+++ b/src/cmd/main.go
@@ -51,15 +51,6 @@ func isEnabled(l *linterRunner, r *linter.Report) bool {
 	return !linter.ExcludeRegex.MatchString(r.GetFilename())
 }
 
-// canBeDisabled returns whether or not '@linter disable' can be used for the specified file
-func canBeDisabled(l *linterRunner, filename string) bool {
-	if l.allowDisableRegex == nil {
-		return false
-	}
-
-	return l.allowDisableRegex.MatchString(filename)
-}
-
 // Run executes linter main function.
 //
 // It is separate from linter so that you can insert your own hooks
@@ -248,13 +239,6 @@ func analyzeReports(l *linterRunner, cfg *MainConfig, diff []*linter.Report) (cr
 			continue
 		}
 
-		if r.IsDisabledByUser() {
-			filename := r.GetFilename()
-			if canBeDisabled(l, filename) {
-				continue
-			}
-		}
-
 		filtered = append(filtered, r)
 
 		if isCritical(l, r) {
diff --git a/src/linter/report.go b/src/linter/report.go
index 3f455b0..53c0a79 100644
--- a/src/linter/report.go
+++ b/src/linter/report.go
@@ -466,16 +466,15 @@ function performance_test() {}`,
 
 // Report is a linter report message.
 type Report struct {
-	checkName  string
-	startLn    string
-	startChar  int
-	startLine  int
-	endChar    int
-	level      int
-	msg        string
-	filename   string
-	hash       uint64
-	isDisabled bool // user-defined flag that file should not be linted
+	checkName string
+	startLn   string
+	startChar int
+	startLine int
+	endChar   int
+	level     int
+	msg       string
+	filename  string
+	hash      uint64
 }
 
 func (r *Report) Hash() uint64 {
@@ -560,11 +559,6 @@ func (r *Report) IsCritical() bool {
 	return r.level != LevelDoNotReject
 }
 
-// IsDisabledByUser returns whether or not user thinks that this file should not be checked
-func (r *Report) IsDisabledByUser() bool {
-	return r.isDisabled
-}
-
 // GetFilename returns report filename
 func (r *Report) GetFilename() string {
 	return r.filename
diff --git a/src/linter/root.go b/src/linter/root.go
index 749f172..6e0638f 100644
--- a/src/linter/root.go
+++ b/src/linter/root.go
@@ -64,9 +64,8 @@ type RootWalker struct {
 
 	currentClassNode node.Node
 
-	allowDisabledRegexp        *regexp.Regexp // user-defined flag that files suitable for this regular expression should not be linted
-	allowDisabledRegexpWasUsed bool           // additional flag indicating that the file name has already been checked by regular expression
-	linterDisabled             bool           // flag indicating whether linter is disabled. Flag is set to true only if the file
+	allowDisabledRegexp *regexp.Regexp // user-defined flag that files suitable for this regular expression should not be linted
+	linterDisabled      bool           // flag indicating whether linter is disabled. Flag is set to true only if the file
 	// name matches the pattern and @linter disable was encountered
 
 	// strictTypes is true if file contains `declare(strict_types=1)`.
@@ -387,6 +386,11 @@ func (d *RootWalker) Report(n node.Node, level int, checkName, msg string, args
 	if d.autoGenerated && !CheckAutoGenerated {
 		return
 	}
+	// We don't report anything if linter was disabled by a
+	// successfull @linter disable, unless it's the linterError.
+	if d.linterDisabled && checkName != "linterError" {
+		return
+	}
 
 	var pos position.Position
 
@@ -466,23 +470,16 @@ func (d *RootWalker) Report(n node.Node, level int, checkName, msg string, args
 			}
 		}
 
-		allowDisabled := false
-
-		if d.allowDisabledRegexp != nil {
-			allowDisabled = d.allowDisabledRegexp.MatchString(d.ctx.st.CurrentFile)
-		}
-
 		d.reports = append(d.reports, &Report{
-			checkName:  checkName,
-			startLn:    string(startLn),
-			startChar:  startChar,
-			startLine:  pos.StartLine,
-			endChar:    endChar,
-			level:      level,
-			filename:   d.ctx.st.CurrentFile,
-			msg:        msg,
-			hash:       hash,
-			isDisabled: allowDisabled,
+			checkName: checkName,
+			startLn:   string(startLn),
+			startChar: startChar,
+			startLine: pos.StartLine,
+			endChar:   endChar,
+			level:     level,
+			filename:  d.ctx.st.CurrentFile,
+			msg:       msg,
+			hash:      hash,
 		})
 	}
 }
@@ -585,13 +582,19 @@ func (d *RootWalker) handleComment(c freefloating.String, n node.Node) {
 		}
 
 		for _, p := range ln.(*phpdoc.RawCommentPart).Params {
-
-			if d.allowDisabledRegexp != nil && !d.allowDisabledRegexpWasUsed {
-				d.linterDisabled = d.allowDisabledRegexp.MatchString(d.ctx.st.CurrentFile)
-				d.allowDisabledRegexpWasUsed = true
+			if p != "disable" {
+				continue
 			}
-
-			if p == "disable" && !d.linterDisabled {
+			if d.linterDisabled {
+				d.Report(nil, LevelInformation, "linterError", "Linter is already disabled for this file")
+				continue
+			}
+			canDisable := false
+			if d.allowDisabledRegexp != nil {
+				canDisable = d.allowDisabledRegexp.MatchString(d.ctx.st.CurrentFile)
+			}
+			d.linterDisabled = canDisable
+			if !canDisable {
 				d.Report(nil, LevelInformation, "linterError", "You are not allowed to disable linter")
 			}
 		}


Test patch:
diff --git a/src/linttest/basic_test.go b/src/linttest/basic_test.go
index b9e0d5a..e43ab21 100644
--- a/src/linttest/basic_test.go
+++ b/src/linttest/basic_test.go
@@ -2,6 +2,7 @@ package linttest_test
 
 import (
 	"log"
+	"regexp"
 	"strings"
 	"testing"
 
@@ -109,16 +110,40 @@ try {
 	test.RunAndMatch()
 }
 
-func TestLinterDisable(t *testing.T) {
+func TestLinterDisableUnmatchedFilename(t *testing.T) {
 	test := linttest.NewSuite(t)
-	// TODO(Petr Makhnev): Here we need the ability to add flags for the file
+	test.AllowDisable = regexp.MustCompile(`will match nothing`)
 	test.AddFile(`<?php
 /** @linter disable */
 $_ = array(1);
 `)
 	test.Expect = []string{
 		`You are not allowed to disable linter`,
-		`Use of old array syntax (use short form instead)`,
+		`Use of old array syntax`,
+	}
+	test.RunAndMatch()
+}
+
+func TestLinterDisable(t *testing.T) {
+	test := linttest.NewSuite(t)
+	test.AllowDisable = regexp.MustCompile(`.*`)
+	test.AddFile(`<?php
+/** @linter disable */
+$_ = array(1);
+`)
+	test.RunAndMatch()
+}
+
+func TestLinterDisableTwice(t *testing.T) {
+	test := linttest.NewSuite(t)
+	test.AllowDisable = regexp.MustCompile(`.*`)
+	test.AddFile(`<?php
+/** @linter disable */
+$_ = array(1);
+/** @linter disable */
+`)
+	test.Expect = []string{
+		`Linter is already disabled for this file`,
 	}
 	test.RunAndMatch()
 }
diff --git a/src/linttest/linttest.go b/src/linttest/linttest.go
index 139561f..a4396f4 100644
--- a/src/linttest/linttest.go
+++ b/src/linttest/linttest.go
@@ -6,6 +6,7 @@ import (
 	"log"
 	"math/rand"
 	"os"
+	"regexp"
 	"strconv"
 	"strings"
 	"sync"
@@ -71,6 +72,8 @@ type Suite struct {
 	Files  []TestFile
 	Expect []string
 
+	AllowDisable *regexp.Regexp
+
 	LoadStubs []string
 }
 
@@ -172,7 +175,7 @@ func (s *Suite) RunLinter() []*linter.Report {
 
 	shuffleFiles(s.Files)
 	for _, f := range s.Files {
-		parseTestFile(s.t, f)
+		parseTestFile(s.t, f, s.AllowDisable)
 	}
 
 	meta.SetIndexingComplete(true)
@@ -187,12 +190,8 @@ func (s *Suite) RunLinter() []*linter.Report {
 			continue
 		}
 
-		_, w := parseTestFile(s.t, f)
-		for _, r := range w.GetReports() {
-			if !r.IsDisabledByUser() {
-				reports = append(reports, r)
-			}
-		}
+		_, w := parseTestFile(s.t, f, s.AllowDisable)
+		reports = append(reports, w.GetReports()...)
 	}
 
 	return reports
@@ -203,7 +202,7 @@ func ParseTestFile(t *testing.T, filename, content string) (rootNode node.Node,
 	return parseTestFile(t, TestFile{
 		Name: filename,
 		Data: []byte(content),
-	})
+	}, nil)
 }
 
 func init() {
@@ -217,9 +216,9 @@ func shuffleFiles(files []TestFile) {
 	})
 }
 
-func parseTestFile(t testing.TB, f TestFile) (rootNode node.Node, w *linter.RootWalker) {
+func parseTestFile(t testing.TB, f TestFile, allowDisable *regexp.Regexp) (rootNode node.Node, w *linter.RootWalker) {
 	var err error
-	rootNode, w, err = linter.ParseContents(f.Name, f.Data, nil, nil)
+	rootNode, w, err = linter.ParseContents(f.Name, f.Data, nil, allowDisable)
 	if err != nil {
 		t.Fatalf("could not parse %s: %v", f.Name, err.Error())
 	}