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: new rule for declare(strict_types) #1213

Closed
wants to merge 8 commits into from
Closed
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
2 changes: 2 additions & 0 deletions .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -6,3 +6,5 @@ src/cmd/stubs/phpstorm-stubs/
y.output
.idea
vendor
dev
src/tests/golden/testdata/quickfix/*.fix
1 change: 1 addition & 0 deletions example/custom/custom_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ func TestAssignmentAsExpression(t *testing.T) {
addCheckers(test.Config())

test.AddFile(`<?php
declare(strict_types = 1);
// PHPDoc annotations are not required for NoVerify in simple cases.
function something() {
$a = "test";
Expand Down
2 changes: 2 additions & 0 deletions src/ir/irutil/keyword.go
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,8 @@ func Keywords(n ir.Node) []*token.Token {
switch n := n.(type) {
case *ir.FunctionStmt:
return []*token.Token{n.FunctionTkn}
case *ir.DeclareStmt:
return []*token.Token{n.DeclareTkn}
case *ir.DefaultStmt:
return []*token.Token{n.DefaultTkn}
case *ir.CaseStmt:
Expand Down
33 changes: 32 additions & 1 deletion src/linter/block_linter.go
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,9 @@ func (b *blockLinter) enterNode(n ir.Node) {
case *ir.ExpressionStmt:
b.checkStmtExpression(n)

case *ir.DeclareStmt:
b.checkDeclareStrictTypes(n)

case *ir.ConstFetchExpr:
b.checkConstFetch(n)

Expand Down Expand Up @@ -547,7 +550,6 @@ func (b *blockLinter) checkNew(e *ir.NewExpr) {

func (b *blockLinter) checkStmtExpression(s *ir.ExpressionStmt) {
report := false

// All branches except default try to filter-out common
// cases to reduce the number of type solving performed.
if irutil.IsAssign(s.Expr) {
Expand Down Expand Up @@ -1533,3 +1535,32 @@ func (b *blockLinter) classParseState() *meta.ClassParseState {
func (b *blockLinter) metaInfo() *meta.Info {
return b.walker.r.ctx.st.Info
}

func (b *blockLinter) checkDeclareStrictTypes(statement *ir.DeclareStmt) {
if b.walker.r.strictTypes {
return
}

for _, i := range statement.Consts {
node, ok := i.(*ir.ConstantStmt)
if !ok {
return
}

if node.ConstantName.Value != "strict_types" {
return
}

value, isNumber := node.Expr.(*ir.Lnumber)
if !isNumber {
return
}

if value.Value == "1" {
return
}

b.report(statement, LevelWarning, "notStrictTypes", "strict_types value is not 1")
b.walker.r.addQuickFix("notStrictTypes", b.quickfix.StrictTypes(value))
}
}
16 changes: 16 additions & 0 deletions src/linter/quickfix.go
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,22 @@ func (g *QuickFixGenerator) Array(arr *ir.ArrayExpr) quickfix.TextEdit {
}
}

func (g *QuickFixGenerator) StrictTypes(lnumber *ir.Lnumber) quickfix.TextEdit {
return quickfix.TextEdit{
StartPos: lnumber.Position.StartPos,
EndPos: lnumber.Position.EndPos,
Replacement: "1",
}
}

func (g *QuickFixGenerator) CreateDeclareStrictTypes(root *ir.Root) quickfix.TextEdit {
return quickfix.TextEdit{
StartPos: root.Position.StartPos,
EndPos: root.Position.StartPos,
Replacement: "declare(strict_types = 1);\n",
}
}

func (g *QuickFixGenerator) NullForNotNullableProperty(prop *ir.PropertyStmt) quickfix.TextEdit {
from := prop.Position.StartPos
to := prop.Variable.Position.EndPos
Expand Down
36 changes: 27 additions & 9 deletions src/linter/report.go
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,24 @@ func addBuiltinCheckers(reg *CheckersRegistry) {
After: `$s = strip_tags($s, '<br>')`,
},

{
Name: "notStrictTypes",
Default: true,
Quickfix: true,
Comment: "Report strict_types value is not 1 in declare section.",
Before: `declare(strict_types = 0);`,
After: `declare(strict_types = 1);`,
},

{
Name: "noDeclareSection",
Default: true,
Quickfix: true,
Comment: "Report declare(strict_types=1) has not been set.",
Before: ` `,
After: `declare(strict_types = 1);`,
},

{
Name: "emptyStmt",
Default: true,
Expand Down Expand Up @@ -1185,8 +1203,8 @@ func DiffReports(gitRepo string, diffArgs []string, changesList []git.Change, ch
}
}

old := reportListToMap(oldList)
new := reportListToMap(newList)
oldReportMap := reportListToMap(oldList)
newReportMap := reportListToMap(newList)
changes := gitChangesToMap(changesList)

var mu sync.Mutex
Expand All @@ -1196,7 +1214,7 @@ func DiffReports(gitRepo string, diffArgs []string, changesList []git.Change, ch

limitCh := make(chan struct{}, maxConcurrency)

for filename, list := range new {
for filename, list := range newReportMap {
wg.Add(1)
go func(filename string, list []*Report) {
limitCh <- struct{}{}
Expand All @@ -1212,7 +1230,7 @@ func DiffReports(gitRepo string, diffArgs []string, changesList []git.Change, ch
oldName = filename // full diff mode
}

reports, err := diffReportsList(gitRepo, ignoreCommits, diffArgs, filename, c, old[oldName], list)
reports, err := diffReportsList(gitRepo, ignoreCommits, diffArgs, filename, c, oldReportMap[oldName], list)
if err != nil {
mu.Lock()
resErr = err
Expand Down Expand Up @@ -1266,8 +1284,8 @@ func diffReportsList(gitRepo string, ignoreCommits map[string]struct{}, diffArgs
}
}

old, oldMaxLine := reportListToPerLineMap(oldList)
new, newMaxLine := reportListToPerLineMap(newList)
oldPerLineMap, oldMaxLine := reportListToPerLineMap(oldList)
newPerLineMap, newMaxLine := reportListToPerLineMap(newList)

var maxLine = oldMaxLine
if newMaxLine > maxLine {
Expand All @@ -1284,17 +1302,17 @@ func diffReportsList(gitRepo string, ignoreCommits map[string]struct{}, diffArgs
// just deletion
if ok && ch.new.HaveRange && ch.new.Range == 0 {
oldLine = ch.old.To
newLine-- // cancel the increment of newLine, because code was deleted, no new lines added
newLine-- // cancel the increment of newLine, because code was deleted, no newPerLineMap lines added
continue
}

res = maybeAppendReports(res, new, old, newLine, oldLine, blame, ignoreCommits)
res = maybeAppendReports(res, newPerLineMap, oldPerLineMap, newLine, oldLine, blame, ignoreCommits)

if ok {
oldLine = 0 // all changes and additions must be checked
for j := newLine + 1; j <= ch.new.To; j++ {
newLine = j
res = maybeAppendReports(res, new, old, newLine, oldLine, blame, ignoreCommits)
res = maybeAppendReports(res, newPerLineMap, oldPerLineMap, newLine, oldLine, blame, ignoreCommits)
}
oldLine = ch.old.To
}
Expand Down
15 changes: 11 additions & 4 deletions src/linter/root.go
Original file line number Diff line number Diff line change
Expand Up @@ -69,10 +69,12 @@ type rootWalker struct {
// name matches the pattern and @linter disable was encountered

// strictTypes is true if file contains `declare(strict_types=1)`.
strictTypes bool
strictMixed bool
strictTypes bool
strictMixed bool
declareSection bool

reports []*Report
reports []*Report
quickfix *QuickFixGenerator

config *Config

Expand Down Expand Up @@ -141,7 +143,12 @@ func (d *rootWalker) EnterNode(n ir.Node) (res bool) {
}
if c.ConstantName.Value == "strict_types" {
v, ok := c.Expr.(*ir.Lnumber)
if ok && v.Value == "1" {
if !ok {
continue
}

d.declareSection = true
if v.Value == "1" {
d.strictTypes = true
}
}
Expand Down
5 changes: 5 additions & 0 deletions src/linter/worker.go
Original file line number Diff line number Diff line change
Expand Up @@ -315,6 +315,11 @@ func (w *Worker) analyzeFile(file *workspace.File, rootNode *ir.Root) (*rootWalk
}
walker.afterLeaveFile()

if !walker.declareSection {
walker.Report(rootNode, LevelWarning, "noDeclareSection", "Missed declare(strict_types=1) directive")
walker.addQuickFix("notStrictTypes", walker.quickfix.CreateDeclareStrictTypes(rootNode))
}

if len(walker.ctx.fixes) != 0 {
needApplyFixes := !file.AutoGenerated() || w.config.CheckAutoGenerated

Expand Down
8 changes: 8 additions & 0 deletions src/tests/checkers/anon_class_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import (

func TestSimpleAnonClass(t *testing.T) {
linttest.SimpleNegativeTest(t, `<?php
declare(strict_types = 1);
function f() {
$a = new class {
/** */
Expand All @@ -21,6 +22,7 @@ function f() {

func TestAnonClassAsInterface(t *testing.T) {
linttest.SimpleNegativeTest(t, `<?php
declare(strict_types = 1);
interface IFace {}

function f(IFace $if) {}
Expand All @@ -32,6 +34,7 @@ f(new class implements IFace {});
func TestAnonClassFromDocumentation(t *testing.T) {
test := linttest.NewSuite(t)
test.AddFile(`<?php
declare(strict_types = 1);
class Outer {
private $prop = 1;
protected $prop2 = 2;
Expand Down Expand Up @@ -66,6 +69,7 @@ echo (new Outer)->func2()->func3();

func TestAnonClassWithConstructor(t *testing.T) {
linttest.SimpleNegativeTest(t, `<?php
declare(strict_types = 1);
function f() {
$a = new class(100, "s") {
/** */
Expand All @@ -84,6 +88,7 @@ function f() {

func TestAnonClassWithExtends(t *testing.T) {
linttest.SimpleNegativeTest(t, `<?php
declare(strict_types = 1);
class Boo {
/** */
public function b() {}
Expand All @@ -103,6 +108,7 @@ function f() {

func TestAnonClassWithImplements(t *testing.T) {
linttest.SimpleNegativeTest(t, `<?php
declare(strict_types = 1);
interface IBoo {
/** */
public function b() {}
Expand All @@ -121,6 +127,8 @@ function f() {

func TestAnonClassWithSeveralImplements(t *testing.T) {
linttest.SimpleNegativeTest(t, `<?php
declare(strict_types = 1);

interface IBoo {
/** */
public function b() {}
Expand Down
3 changes: 3 additions & 0 deletions src/tests/checkers/arrow_functions_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import (
func TestArrowFunction(t *testing.T) {
test := linttest.NewSuite(t)
test.AddFile(`<?php
declare(strict_types = 1);
class Boo {
/** @return int */
public function b() { }
Expand Down Expand Up @@ -94,6 +95,8 @@ func TestUnusedInArrowFunction(t *testing.T) {
`stubs/phpstorm-stubs/standard/standard_9.php`,
}
test.AddFile(`<?php
declare(strict_types = 1);

function f() {
$a1 = [];
$a2 = [];
Expand Down
Loading
Loading