無駄な代入文を発見する #1

sanposhiho · 2020-09-02T18:56:59Z

以下に対して警告を行う

再代入された後にその値が使われることなく、別の値に書き換わっている
再代入された後にその値が使われず関数が終了する

その他

github actionsでのテストの導入

レビュー頂きたい事

~~自分ではエッジケースが思い付かず、かっっっなりのシンプルな実装になってしまいました~~何かこの実装で引っかからない特殊なパターンなどありましたら教えていただきたいです🙇‍♂️

gasugesu

条件分岐への対応が抜けているようです

tenntenn · 2020-09-03T02:00:25Z

wastedassign.go

+	s := pass.ResultOf[buildssa.Analyzer].(*buildssa.SSA)
+
+	for _, sf := range s.SrcFuncs {
+		for _, local := range sf.Locals {


Naiveモードでビルドしないと厳しくないですか？

buildssaをforkしてforkしたrepositoryでNaiveモードに変更しています🙏

https://github.com/sanposhiho/tools

sanposhiho · 2020-09-03T02:04:14Z

@gasugesu

条件分岐への対応が抜けているようです

は

	useOutOfIf := 0 // want "wasted assignment"
	ifBool := false
	if ifBool {
		useOutOfIf = 10 // want "reassigned, but never used afterwards" が出て欲しいけど、"wasted assignment"が出る
		return
	}
	useOutOfIf = 12
	println(useOutOfIf)
	return

のパターンであっていますか…？（↑直します

gasugesu · 2020-09-03T02:06:01Z

@gasugesu

条件分岐への対応が抜けているようです

は
	useOutOfIf := 0 // want "wasted assignment"
	ifBool := false
	if ifBool {
		useOutOfIf = 10 // want "reassigned, but never used afterwards" が出て欲しいけど、"wasted assignment"が出る
		return
	}
	useOutOfIf = 12
	println(useOutOfIf)
	return
のパターンであっていますか…？（↑直します

そうですね!
僕もまさにこのif elseのパターンで悩んでいるのですが、複数のブロックから構成される場合が多少面倒そうです

knsh14 · 2020-09-03T02:40:49Z

.github/workflows/test_and_lint.yaml

@@ -0,0 +1,45 @@
+name: test_and_lint


GitHub Actionsで自動でチェックするのいいですね！
reviewdog というツールを使って自動で指摘がある箇所にコメントをつけられるツールもあるので導入してみるとより便利になります
http://haya14busa.com/reviewdog/

sanposhiho · 2020-09-03T07:04:17Z

@gasugesu

あっ、と言うか内容被っちゃってたんですね…（今気がついた

tenntenn · 2020-09-03T07:51:17Z

wastedassign.go

+	if noNextSuccs {
+		return noUseUntilReturn
+	} else {
+		return notWasted


いい指摘！

110y · 2020-09-03T08:43:09Z

test caseを少しいじって

package a

func f() {
	useOutOfIf := 0 // want "wasted assignment"
	if false {
		useOutOfIf = 10 // want "reassigned, but never used afterwards"
		useOutOfIf = 10 // want "reassigned, but never used afterwards" // <= added

		return
	}
	useOutOfIf = 12
	println(useOutOfIf)
	return
}

のようにすると

--- FAIL: TestAnalyzer (0.01s)
    analysistest.go:419: a/a.go:6:3: diagnostic "wasted assignment" does not match pattern "reassigned, but never used afterwards"
    analysistest.go:419: a/a.go:7:3: diagnostic "wasted assignment" does not match pattern "reassigned, but never used afterwards"
    analysistest.go:483: a/a.go:6: no diagnostic was reported matching "reassigned, but never used afterwards"
    analysistest.go:483: a/a.go:7: no diagnostic was reported matching "reassigned, but never used afterwards"
FAIL
FAIL	github.com/sanposhiho/wastedassign	0.019s
?   	github.com/sanposhiho/wastedassign/cmd/wastedassign	[no test files]
?   	github.com/sanposhiho/wastedassign/plugin	[no test files]
FAIL

のような形でテストが落ちるのですが、これは想定通りですか？
追加した useOutOfIf = 10 も reassigned, but never used afterwards で警告されるべきなのかなと思いました。

sanposhiho · 2020-09-03T09:04:26Z

@110y san
レビューありがとうございます！

package a

func f() {
	useOutOfIf := 0 // want "wasted assignment"
	if false {
		useOutOfIf = 10 // want "wasted assignment"  // <= fix
		useOutOfIf = 10 // want "reassigned, but never used afterwards" // <= added

		return
	}
	useOutOfIf = 12
	println(useOutOfIf)
	return
}

テストケースは正しくはこうなります!

wasted assignment: 代入後にその値を使うことなく別の値に書き換えられている
reassigned, but never used afterwards: 代入後にその値を使うことなく関数がreturn

と言った使い分けになっています

が、どちらにしろ追加していただいた部分でエラーが出るのは間違っていたので修正しました🙇‍♂️

gasugesu · 2020-09-04T02:30:25Z

close後に後出しですみません
このテストケースだと

elseの中のretが間違ってret declared but not usedと報告される
analyzerが途中で落ちる
ようです.
ちなみにreturn の直前のuseOutOfIf += ...に報告がなされないのは想定通りですか？

(報告用のwantを正しく修正できてないですがよろしくお願いします)

func p(x int) int {
	return x + 1
}

func f(param int) int {
	useOutOfIf := 0
	ret := 0
	if false {
		useOutOfIf = 10 // want "wasted assignment"
		useOutOfIf = 10 // want "reassigned, but never used afterwards"

		return 0
	} else if param == 100 {
		ret = useOutOfIf
	} else if param == 200 {
		useOutOfIf = 100
		useOutOfIf = 100
		useOutOfIf = p(useOutOfIf)
		useOutOfIf += 200
	} else {
		ret := 100
	}
	useOutOfIf = 12
	println(useOutOfIf)
	useOutOfIf = 192 // want "reassigned, but never used afterwards"
	useOutOfIf += 100
	useOutOfIf += 200
	return ret
}

sanposhiho · 2020-09-04T02:46:46Z

@gasugesu

コメントありがとうございます！

elseの中のretが間違ってret declared but not usedと報告される

はwastedassignが出力しているエラーではないです（goが出している物）（else句のscopeにretを:=で新たに定義したけど使われてないやんけの意味

なのでそもそもtestcaseがgoのコード的によくないです

なので正しいtestcaseは（ついでにwantも付与して）こうなると思います

package a

func p(x int) int {
	return x + 1
}

func f(param int) int {
	useOutOfIf := 0 // want "wasted assignment"
	ret := 0        // want "wasted assignment"
	if false {
		useOutOfIf = 10 // want "wasted assignment"
		useOutOfIf = 10 // want "reassigned, but never used afterwards"

		return 0
	} else if param == 100 {
		ret = useOutOfIf
	} else if param == 200 {
		useOutOfIf = 100 // want "wasted assignment"
		useOutOfIf = 100
		useOutOfIf = p(useOutOfIf)
		useOutOfIf += 200 // want "reassigned, but never used afterwards"
	}
	useOutOfIf = 12
	println(useOutOfIf)
	useOutOfIf = 192
	useOutOfIf += 100
	useOutOfIf += 200 // want "reassigned, but never used afterwards"
	return ret
}

これなら通ります

ちなみにreturn の直前のuseOutOfIf += ...に報告がなされないのは想定通りですか？

これに関してですが上のテストケースで"reassigned, but never used afterwards"がちゃんと出力されると思います

長くなりましたが、以上から総じてこのtestcaseは正しく動作している気がします🙏

sanposhiho · 2020-09-04T02:54:30Z

@gasugesu
詳しいテストケースとてもありがたいです
他に気になる点があれば是非お願いします！🙏

僕としては上のテストケースだと

ret := 0        // want "wasted assignment"

が怪しい感じを感じ取っています（returnでしか使ってないしwastedではあるが…と言った感じ

gasugesu · 2020-09-04T02:54:48Z

Oh... ret := ..は僕のミスです
正しいテストケースはこちらになります。。。
コードの中にコメント追加しましたが、func f() の一番最初に定義されている変数2個についてどちらも正しく報告されていないような気がしています

package a

func p(x int) int {
	return x + 1
}

func f(param int) int {
	useOutOfIf := 0
	// このretは条件分岐によっては最後のretで使用されるが wasted assignmentと表示される
	ret := 0
	if false {
		useOutOfIf = 10 // want "wasted assignment"
		useOutOfIf = 10 // want "reassigned, but never used afterwards"

		return 0
	} else if param == 100 {
		// 8行目のuseOutOfIfはここで使われるが8行目でwasted assignmentと出ている
		ret = useOutOfIf
	} else if param == 200 {
		useOutOfIf = 100
		useOutOfIf = 100
		useOutOfIf = p(useOutOfIf)
		useOutOfIf += 200
	} else {
                  // ここも使用されていない系のエラー出る
		ret = 100
	}
	useOutOfIf = 12
	println(useOutOfIf)
	useOutOfIf = 192 // want "reassigned, but never used afterwards"
	useOutOfIf += 100
	useOutOfIf += 200
	return ret
}

sanposhiho · 2020-09-04T03:03:18Z

@gasugesu

大きく分けて

returnに使用されているのを使用されているとみなすか
別の値へのStoreに使用されているのを使用されているとみなすか

という問題だと思っていて

両方使用しているとみなすべきな気がしてきましたね…

前者は関数からの結果をreturnするというのは明らかに存在するケースですし、後者も色んなスコープで色んな変数に同じ値を定義する際など使用することがありそうです

issueに立てておきます！ありがとうございます🙏

sanposhiho added 8 commits September 3, 2020 04:46

feat: find wasted assignment statements

a2e9cb0

add: add testdata

d071e75

add: test and lint on github actions

58ef393

fix: analizer name and doc

1284adc

fix: choose lint tools

19fe8a1

remove: wasted white space

019af93

remove: wasted white space

02eefa2

fix: rename error message

2feea5b

sanposhiho force-pushed the find-wasted-assignment branch from 26e7157 to 2feea5b Compare September 2, 2020 19:50

gasugesu reviewed Sep 3, 2020

View reviewed changes

tenntenn reviewed Sep 3, 2020

View reviewed changes

knsh14 reviewed Sep 3, 2020

View reviewed changes

add: 質問のために一旦commit

d96a2a8

sanposhiho added 2 commits September 3, 2020 16:16

fix: ポインタとポインタを比較していたのでエンジニア向いていない

49dc0a1

fix: エラーメッセージを良さげに整えた

0f6047d

tenntenn reviewed Sep 3, 2020

View reviewed changes

wastedassign.go Outdated

if noNextSuccs {

return noUseUntilReturn

} else {

return notWasted

Copy link

tenntenn Sep 3, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

いい指摘！

fix: lintで落ちたので

311f6d2

sanposhiho added 2 commits September 3, 2020 18:00

fix: 重複して捜査していたのを修正

af48dac

fix: 関数の並び順を変更

e0734a2

sanposhiho merged commit 0cf5b0d into master Sep 4, 2020

This was referenced Sep 4, 2020

returnに該当の変数が用いられている時は使用しているとみなす #2

Closed

他の変数のStoreに用いられているときは使用されているとみなす #3

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

無駄な代入文を発見する #1

無駄な代入文を発見する #1

sanposhiho commented Sep 2, 2020 •

edited

gasugesu left a comment

tenntenn Sep 3, 2020

sanposhiho Sep 3, 2020 •

edited

sanposhiho commented Sep 3, 2020

gasugesu commented Sep 3, 2020

knsh14 Sep 3, 2020

sanposhiho commented Sep 3, 2020

tenntenn Sep 3, 2020

110y commented Sep 3, 2020

sanposhiho commented Sep 3, 2020 •

edited

gasugesu commented Sep 4, 2020 •

edited

sanposhiho commented Sep 4, 2020 •

edited

sanposhiho commented Sep 4, 2020

gasugesu commented Sep 4, 2020 •

edited

sanposhiho commented Sep 4, 2020 •

edited

無駄な代入文を発見する #1

無駄な代入文を発見する #1

Conversation

sanposhiho commented Sep 2, 2020 • edited

その他

レビュー頂きたい事

gasugesu left a comment

Choose a reason for hiding this comment

tenntenn Sep 3, 2020

Choose a reason for hiding this comment

sanposhiho Sep 3, 2020 • edited

Choose a reason for hiding this comment

sanposhiho commented Sep 3, 2020

gasugesu commented Sep 3, 2020

knsh14 Sep 3, 2020

Choose a reason for hiding this comment

sanposhiho commented Sep 3, 2020

tenntenn Sep 3, 2020

Choose a reason for hiding this comment

110y commented Sep 3, 2020

sanposhiho commented Sep 3, 2020 • edited

gasugesu commented Sep 4, 2020 • edited

sanposhiho commented Sep 4, 2020 • edited

sanposhiho commented Sep 4, 2020

gasugesu commented Sep 4, 2020 • edited

sanposhiho commented Sep 4, 2020 • edited

sanposhiho commented Sep 2, 2020 •

edited

sanposhiho Sep 3, 2020 •

edited

sanposhiho commented Sep 3, 2020 •

edited

gasugesu commented Sep 4, 2020 •

edited

sanposhiho commented Sep 4, 2020 •

edited

gasugesu commented Sep 4, 2020 •

edited

sanposhiho commented Sep 4, 2020 •

edited