[tools] go/tools: add vet check for time formats with 2006-02-01
Gerrit Bot (Gerrit)
Gerrit Bot has uploaded this change for review.
go/tools: add vet check for time formats with 2006-02-01
yyyy-dd-mm is a time format that isn't really used anywhere [1]. It is
much more likely that the user intended to use yyyy-mm-dd instead and
made a mistake. This happens quite often [2] because of the unusual way
to handle time formatting and parsing in Go. Since the mistake is Go
specific and happens so often a vet check will be useful.
1. https://stackoverflow.com/questions/2254014/are-there-locales-or-common-programs-that-use-yyyy-dd-mm-as-the-date-format
2. https://github.com/search?l=&p=1&q=%222006-02-01%22+language%3AGo&type=Code
Change-Id: I20960c93710766f20a7df90873bff960dea41b28
GitHub-Last-Rev: 378e8e674c1955a6a8c810d59b8c6b215ac42c4b
GitHub-Pull-Request: golang/tools#342
---
M internal/lsp/source/api_json.go
A go/analysis/passes/timeformat/timeformat_test.go
A go/analysis/passes/timeformat/timeformat.go
M internal/lsp/source/options.go
M gopls/doc/analyzers.md
A go/analysis/passes/timeformat/testdata/src/a/a.go
6 files changed, 146 insertions(+), 0 deletions(-)
diff --git a/go/analysis/passes/timeformat/testdata/src/a/a.go b/go/analysis/passes/timeformat/testdata/src/a/a.go
new file mode 100644
index 0000000..ad5e34e
--- /dev/null
+++ b/go/analysis/passes/timeformat/testdata/src/a/a.go
@@ -0,0 +1,22 @@
+// Copyright 2019 The Go Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style
+// license that can be found in the LICENSE file.
+
+// This file contains tests for the timeformat checker.
+
+package a
+
+import (
+ "time"
+)
+
+func hasError() {
+ a, _ := time.Parse("2006-02-01 15:04:05", "2021-01-01 00:00:00") // want `2006-02-01 should be 2006-01-02`
+ a.Format("2006-02-01") // want `2006-02-01 should be 2006-01-02`
+ a.Format("2006-02-01 15:04:05") // want `2006-02-01 should be 2006-01-02`
+}
+
+func notHasError() {
+ a, _ := time.Parse("2006-01-02 15:04:05", "2021-01-01 00:00:00")
+ a.Format("2006-01-02")
+}
diff --git a/go/analysis/passes/timeformat/timeformat.go b/go/analysis/passes/timeformat/timeformat.go
new file mode 100644
index 0000000..628b558
--- /dev/null
+++ b/go/analysis/passes/timeformat/timeformat.go
@@ -0,0 +1,66 @@
+// Copyright 2019 The Go Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style
+// license that can be found in the LICENSE file.
+
+// Package timeformat defines an Analyzer that checks for the use
+// of time.Format or time.Parse calls with a bad format.
+package timeformat
+
+import (
+ "go/ast"
+ "go/constant"
+ "go/types"
+ "strings"
+
+ "golang.org/x/tools/go/analysis"
+ "golang.org/x/tools/go/analysis/passes/inspect"
+ "golang.org/x/tools/go/ast/inspector"
+ "golang.org/x/tools/go/types/typeutil"
+)
+
+const Doc = `check for calls of (time.Time).Format or time.Parse with 2006-02-01
+
+The timeformat checker looks time formats with the bad 2006-02-01 format
+which should be replaced by the intended 2006-01-02 format.
+`
+
+var Analyzer = &analysis.Analyzer{
+ Name: "timeformat",
+ Doc: Doc,
+ Requires: []*analysis.Analyzer{inspect.Analyzer},
+ Run: run,
+}
+
+func run(pass *analysis.Pass) (interface{}, error) {
+ inspect := pass.ResultOf[inspect.Analyzer].(*inspector.Inspector)
+
+ nodeFilter := []ast.Node{
+ (*ast.CallExpr)(nil),
+ }
+ inspect.Preorder(nodeFilter, func(n ast.Node) {
+ call := n.(*ast.CallExpr)
+ fn, ok := typeutil.Callee(pass.TypesInfo, call).(*types.Func)
+ if !ok {
+ return
+ }
+ if (fn.FullName() == "(time.Time).Format" || fn.FullName() == "time.Parse") && isBadFormat(pass, call.Args[0]) {
+ pass.ReportRangef(call, "2006-02-01 should be 2006-01-02")
+ }
+ })
+ return nil, nil
+}
+
+// isBadFormat return true when e is a string containing 2006-02-01.
+func isBadFormat(pass *analysis.Pass, e ast.Expr) bool {
+ tv, ok := pass.TypesInfo.Types[e]
+ if !ok { // no type info, assume good
+ return false
+ }
+
+ t, ok := tv.Type.(*types.Basic)
+ if !ok || t.Info()&types.IsString == 0 {
+ return false
+ }
+
+ return strings.Contains(constant.StringVal(tv.Value), "2006-02-01")
+}
diff --git a/go/analysis/passes/timeformat/timeformat_test.go b/go/analysis/passes/timeformat/timeformat_test.go
new file mode 100644
index 0000000..fba937b
--- /dev/null
+++ b/go/analysis/passes/timeformat/timeformat_test.go
@@ -0,0 +1,17 @@
+// Copyright 2019 The Go Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style
+// license that can be found in the LICENSE file.
+
+package timeformat_test
+
+import (
+ "testing"
+
+ "golang.org/x/tools/go/analysis/analysistest"
+ "golang.org/x/tools/go/analysis/passes/timeformat"
+)
+
+func Test(t *testing.T) {
+ testdata := analysistest.TestData()
+ analysistest.Run(t, testdata, timeformat.Analyzer, "a")
+}
diff --git a/gopls/doc/analyzers.md b/gopls/doc/analyzers.md
index 3a5ae5f..fc914c4 100644
--- a/gopls/doc/analyzers.md
+++ b/gopls/doc/analyzers.md
@@ -483,6 +483,15 @@
**Enabled by default.**
+## **timeformat**
+
+check for calls of (time.Time).Format or time.Parse with 2006-02-01
+
+The timeformat checker looks time formats with the bad 2006-02-01 format
+which should be replaced by the intended 2006-01-02 format.
+
+**Enabled by default.**
+
## **unmarshal**
report passing non-pointer or non-interface values to unmarshal
diff --git a/internal/lsp/source/api_json.go b/internal/lsp/source/api_json.go
index 0c32807..2b0b34e 100755
--- a/internal/lsp/source/api_json.go
+++ b/internal/lsp/source/api_json.go
@@ -527,6 +527,11 @@
Default: "true",
},
{
+ Name: "\"timeformat\"",
+ Doc: "check for calls of (time.Time).Format or time.Parse with 2006-02-01\n\nThe timeformat checker looks time formats with the bad 2006-02-01 format\nwhich should be replaced by the intended 2006-01-02 format.",
+ Default: "true",
+ },
+ {
Name: "\"unmarshal\"",
Doc: "report passing non-pointer or non-interface values to unmarshal\n\nThe unmarshal analysis reports calls to functions such as json.Unmarshal\nin which the argument type is not a pointer or an interface.",
Default: "true",
@@ -1105,6 +1110,11 @@
Default: true,
},
{
+ Name: "timeformat",
+ Doc: "check for calls of (time.Time).Format or time.Parse with 2006-02-01\n\nThe timeformat checker looks time formats with the bad 2006-02-01 format\nwhich should be replaced by the intended 2006-01-02 format.",
+ Default: true,
+ },
+ {
Name: "unmarshal",
Doc: "report passing non-pointer or non-interface values to unmarshal\n\nThe unmarshal analysis reports calls to functions such as json.Unmarshal\nin which the argument type is not a pointer or an interface.",
Default: true,
diff --git a/internal/lsp/source/options.go b/internal/lsp/source/options.go
index cb4b11d..ec39778 100644
--- a/internal/lsp/source/options.go
+++ b/internal/lsp/source/options.go
@@ -42,6 +42,7 @@
"golang.org/x/tools/go/analysis/passes/structtag"
"golang.org/x/tools/go/analysis/passes/testinggoroutine"
"golang.org/x/tools/go/analysis/passes/tests"
+ "golang.org/x/tools/go/analysis/passes/timeformat"
"golang.org/x/tools/go/analysis/passes/unmarshal"
"golang.org/x/tools/go/analysis/passes/unreachable"
"golang.org/x/tools/go/analysis/passes/unsafeptr"
@@ -1240,6 +1241,7 @@
unusedwrite.Analyzer.Name: {Analyzer: unusedwrite.Analyzer, Enabled: false},
useany.Analyzer.Name: {Analyzer: useany.Analyzer, Enabled: true},
infertypeargs.Analyzer.Name: {Analyzer: infertypeargs.Analyzer, Enabled: true},
+ timeformat.Analyzer.Name: {Analyzer: timeformat.Analyzer, Enabled: true},
// gofmt -s suite:
simplifycompositelit.Analyzer.Name: {
To view, visit change 354010. To unsubscribe, or for help writing mail filters, visit settings.
Rebecca Stambler (Gerrit)
Attention is currently required from: Michael Matloob.
Rebecca Stambler removed Ian Cottrell from this change.
To view, visit change 354010. To unsubscribe, or for help writing mail filters, visit settings.
Tim King (Gerrit)
Attention is currently required from: Robert Findley, Michael Matloob.
1 comment:
Patchset:
Mostly a question for reviewers for release timing.
Should this happen now in the Go 1.18 "freeze grace period" or wait for 1.19? This CL is not turning this on in cmd/vet yet. Also the code reviewing has not really started yet. My suggestion would be this CL goes forward at a normal pace now (so gopls gets it), and [assuming all goes well] cmd/vet updates in 1.19. Thoughts?
To view, visit change 354010. To unsubscribe, or for help writing mail filters, visit settings.
Ian Lance Taylor (Gerrit)
Attention is currently required from: Robert Findley, Tim King, Michael Matloob.
1 comment:
Patchset:
Mostly a question for reviewers for release timing. […]
SGTM.
To view, visit change 354010. To unsubscribe, or for help writing mail filters, visit settings.
Tim King (Gerrit)
Attention is currently required from: Robert Findley, Ian Lance Taylor, Michael Matloob.
1 comment:
Patchset:
SGTM.
Erik, we are no longer waiting for the release. Can you rebase this CL so we can test it at head?
To view, visit change 354010. To unsubscribe, or for help writing mail filters, visit settings.
Gerrit Bot (Gerrit)
Attention is currently required from: Robert Findley, Ian Lance Taylor, Michael Matloob.
Gerrit Bot uploaded patch set #2 to this change.
go/tools: add vet check for time formats with 2006-02-01
yyyy-dd-mm is a time format that isn't really used anywhere [1]. It is
much more likely that the user intended to use yyyy-mm-dd instead and
made a mistake. This happens quite often [2] because of the unusual way
to handle time formatting and parsing in Go. Since the mistake is Go
specific and happens so often a vet check will be useful.
1. https://stackoverflow.com/questions/2254014/are-there-locales-or-common-programs-that-use-yyyy-dd-mm-as-the-date-format
2. https://github.com/search?l=&p=1&q=%222006-02-01%22+language%3AGo&type=Code
Change-Id: I20960c93710766f20a7df90873bff960dea41b28
GitHub-Last-Rev: 80add7a6ea8303bf4824e7ddc5c8850edfa69f35
GitHub-Pull-Request: golang/tools#342
---
A go/analysis/passes/timeformat/testdata/src/a/a.go
A go/analysis/passes/timeformat/timeformat.go
A go/analysis/passes/timeformat/timeformat_test.go
M gopls/doc/analyzers.md
M internal/lsp/source/api_json.go
M internal/lsp/source/options.go
6 files changed, 146 insertions(+), 0 deletions(-)
To view, visit change 354010. To unsubscribe, or for help writing mail filters, visit settings.
Tim King (Gerrit)
Attention is currently required from: Robert Findley, Ian Lance Taylor, Michael Matloob.
Patch set 2:Run-TryBot +1
kokoro (Gerrit)
Attention is currently required from: Robert Findley, Ian Lance Taylor, Michael Matloob.
Kokoro presubmit build finished with status: FAILURE
Logs at: https://source.cloud.google.com/results/invocations/b17d7487-6c31-4905-8bd6-dbcee1067eb6
Patch set 2:gopls-CI -1
Gerrit Bot (Gerrit)
Attention is currently required from: Robert Findley, Ian Lance Taylor, Michael Matloob.
Gerrit Bot uploaded patch set #3 to this change.
go/tools: add vet check for time formats with 2006-02-01
yyyy-dd-mm is a time format that isn't really used anywhere [1]. It is
much more likely that the user intended to use yyyy-mm-dd instead and
made a mistake. This happens quite often [2] because of the unusual way
to handle time formatting and parsing in Go. Since the mistake is Go
specific and happens so often a vet check will be useful.
1. https://stackoverflow.com/questions/2254014/are-there-locales-or-common-programs-that-use-yyyy-dd-mm-as-the-date-format
2. https://github.com/search?l=&p=1&q=%222006-02-01%22+language%3AGo&type=Code
Change-Id: I20960c93710766f20a7df90873bff960dea41b28
GitHub-Last-Rev: bc7a674b6f47c71e320ba7af37bbc320e72fb20d
GitHub-Pull-Request: golang/tools#342
---
A go/analysis/passes/timeformat/testdata/src/a/a.go
A go/analysis/passes/timeformat/timeformat.go
A go/analysis/passes/timeformat/timeformat_test.go
M gopls/doc/analyzers.md
M internal/lsp/source/api_json.go
M internal/lsp/source/options.go
6 files changed, 146 insertions(+), 0 deletions(-)
To view, visit change 354010. To unsubscribe, or for help writing mail filters, visit settings.
Robert Findley (Gerrit)
Attention is currently required from: Ian Lance Taylor, Michael Matloob.
Patch set 3:Run-TryBot +1
Robert Findley (Gerrit)
Attention is currently required from: Ian Lance Taylor, Erik Dubbelboer, Michael Matloob.
1 comment:
Patchset:
I don't think the failures above are from my code?
I think they were, as the analyzer documentation needed updating? Can you try the suggested command?
`go run doc/generate.go` from the gopls module
To view, visit change 354010. To unsubscribe, or for help writing mail filters, visit settings.
kokoro (Gerrit)
Attention is currently required from: Ian Lance Taylor, Erik Dubbelboer, Michael Matloob.
Kokoro presubmit build finished with status: FAILURE
Logs at: https://source.cloud.google.com/results/invocations/db8d2604-7422-4706-bbf5-a018d5e8f6e7
Patch set 3:gopls-CI -1
Gerrit Bot (Gerrit)
Attention is currently required from: Ian Lance Taylor, Michael Matloob.
Gerrit Bot uploaded patch set #4 to this change.
go/tools: add vet check for time formats with 2006-02-01
yyyy-dd-mm is a time format that isn't really used anywhere [1]. It is
much more likely that the user intended to use yyyy-mm-dd instead and
made a mistake. This happens quite often [2] because of the unusual way
to handle time formatting and parsing in Go. Since the mistake is Go
specific and happens so often a vet check will be useful.
1. https://stackoverflow.com/questions/2254014/are-there-locales-or-common-programs-that-use-yyyy-dd-mm-as-the-date-format
2. https://github.com/search?l=&p=1&q=%222006-02-01%22+language%3AGo&type=Code
Change-Id: I20960c93710766f20a7df90873bff960dea41b28
GitHub-Last-Rev: 1c22f3b90e5298c590c917aa44872acdbdfb2e98
GitHub-Pull-Request: golang/tools#342
---
A go/analysis/passes/timeformat/testdata/src/a/a.go
A go/analysis/passes/timeformat/timeformat.go
A go/analysis/passes/timeformat/timeformat_test.go
M gopls/doc/analyzers.md
M internal/lsp/source/api_json.go
M internal/lsp/source/options.go
6 files changed, 147 insertions(+), 0 deletions(-)
To view, visit change 354010. To unsubscribe, or for help writing mail filters, visit settings.
Tim King (Gerrit)
Attention is currently required from: Ian Lance Taylor, Michael Matloob.
Patch set 4:Run-TryBot +1
kokoro (Gerrit)
Attention is currently required from: Ian Lance Taylor, Michael Matloob.
Kokoro presubmit build finished with status: SUCCESS
Logs at: https://source.cloud.google.com/results/invocations/cbdd5b8d-fce0-4e9f-ac94-d9959527973b
Patch set 4:gopls-CI +1
Tim King (Gerrit)
Attention is currently required from: Ian Lance Taylor, Michael Matloob.
1 comment:
Patchset:
Erik, we are no longer waiting for the release. […]
Done
To view, visit change 354010. To unsubscribe, or for help writing mail filters, visit settings.
Erik Dubbelboer (Gerrit)
Patchset:
You're right, should be fixed now then.
To view, visit change 354010. To unsubscribe, or for help writing mail filters, visit settings.
Erik Dubbelboer (Gerrit)
Attention is currently required from: Robert Findley, Ian Lance Taylor, Michael Matloob.
1 comment:
Patchset:
I don't think the failures above are from my code?
To view, visit change 354010. To unsubscribe, or for help writing mail filters, visit settings.
Tim King (Gerrit)
Attention is currently required from: Ian Lance Taylor, Michael Matloob.
5 comments:
Patchset:
https://go.dev/doc/contribute#commit_messages
Please mention the issue that is being fixed in the commit message.
File go/analysis/passes/timeformat/timeformat.go:
Patch Set #4, Line 23:
2006-02-0I think in the doc string we may need to say what 2006-02-01 means in case someone is not fully aware of the format. We should also say why it is bad.
Let's check that fn.Name() is either "Format" or "Parse" first. This will be mildly more efficient due to lack of string construction.
```
if name := fn.Name(); name != "Format" || name != "Parse" {
return
}
```
Once we have filtered on this, the checks can be slower.Patch Set #4, Line 46:
call.Args[0]Let's check len(call.Args) > 0 out an abundance of caution.
Patch Set #4, Line 54:
pass *analysis.Passnit: Pass *types.Info instead.
To view, visit change 354010. To unsubscribe, or for help writing mail filters, visit settings.
Robert Findley (Gerrit)
Attention is currently required from: Ian Lance Taylor, Michael Matloob.
3 comments:
Commit Message:
Patch Set #4, Line 7:
vet checknit: this is not a vet check (yet), it is just an analyzer.
File go/analysis/passes/timeformat/timeformat.go:
Patch Set #4, Line 7:
timeformatAdding this to go/analysis constitutes an API change to x/tools, and therefore is in-scope for the proposal process:
https://github.com/golang/proposal#scopeSorry for dropping this on the CL at this time; this was a recent clarification in the scope of the proposal process.
This analyzer seems useful to me, so I think it makes sense to start by just adding it to gopls, where we can get some experience.
Can you please:
1) Update this CL to move this analyzer to x/tools/internal/lsp/analysis, so that it is not importable.
2) (if you don't mind) open a brief proposal issue for the addition to vet. It is likely that there will be active discussion around whether or not this constitutes an analysis that meets the criteria for vet.Please reference the opened issue in the commit message ("Updates golang/go#NNNNN").
Patch Set #4, Line 47:
ReportRangefIt would be trivial to include a suggested fix here (see go/analysis.Diagnostic), which would manifest as a quick-fix in gopls.
To view, visit change 354010. To unsubscribe, or for help writing mail filters, visit settings.
Tim King (Gerrit)
Attention is currently required from: Robert Findley, Ian Lance Taylor, Michael Matloob.
1 comment:
File go/analysis/passes/timeformat/timeformat.go:
Patch Set #4, Line 7:
timeformat
Adding this to go/analysis constitutes an API change to x/tools, and therefore is in-scope for the p […]
Proposal https://github.com/golang/go/issues/48801 was accepted. Do we think this needs another proposal or goes beyond what was proposed there?
(Updates golang/go#NNNNN is definitely missing.)
To view, visit change 354010. To unsubscribe, or for help writing mail filters, visit settings.
Robert Findley (Gerrit)
Attention is currently required from: Tim King, Ian Lance Taylor, Michael Matloob.
1 comment:
File go/analysis/passes/timeformat/timeformat.go:
Patch Set #4, Line 7:
timeformat
Proposal https://github.com/golang/go/issues/48801 was accepted. […]
Oh, no I just wasn't aware of that issue (or forgot about it).
In that case, please just add "Updates golang/go#48801" to the commit message.
To view, visit change 354010. To unsubscribe, or for help writing mail filters, visit settings.
Gerrit Bot (Gerrit)
Attention is currently required from: Ian Lance Taylor, Michael Matloob, Tim King.
Gerrit Bot uploaded patch set #5 to this change.
go/tools: add vet check for time formats with 2006-02-01
yyyy-dd-mm is a time format that isn't really used anywhere [1]. It is
much more likely that the user intended to use yyyy-mm-dd instead and
made a mistake. This happens quite often [2] because of the unusual way
to handle time formatting and parsing in Go. Since the mistake is Go
specific and happens so often a vet check will be useful.
1. https://stackoverflow.com/questions/2254014/are-there-locales-or-common-programs-that-use-yyyy-dd-mm-as-the-date-format
2. https://github.com/search?l=&p=1&q=%222006-02-01%22+language%3AGo&type=Code
Change-Id: I20960c93710766f20a7df90873bff960dea41b28
GitHub-Last-Rev: cb29ef3668fe4239ed16ceaade121528d860ab7b
GitHub-Pull-Request: golang/tools#342
---
A go/analysis/passes/timeformat/testdata/src/a/a.go
A go/analysis/passes/timeformat/timeformat.go
A go/analysis/passes/timeformat/timeformat_test.go
M gopls/doc/analyzers.md
M internal/lsp/source/api_json.go
M internal/lsp/source/options.go
6 files changed, 147 insertions(+), 0 deletions(-)
To view, visit change 354010. To unsubscribe, or for help writing mail filters, visit settings.
Tim King (Gerrit)
Attention is currently required from: Ian Lance Taylor, Michael Matloob.
1 comment:
Patchset:
Thank you for taking on this bug.
I did not yet see any changes from patch set 4 to patch set 5. Did I miss something?
To view, visit change 354010. To unsubscribe, or for help writing mail filters, visit settings.
Gerrit Bot (Gerrit)
Attention is currently required from: Ian Lance Taylor, Michael Matloob.
Gerrit Bot uploaded patch set #6 to this change.
go/tools: add vet check for time formats with 2006-02-01
yyyy-dd-mm is a time format that isn't really used anywhere [1]. It is
much more likely that the user intended to use yyyy-mm-dd instead and
made a mistake. This happens quite often [2] because of the unusual way
to handle time formatting and parsing in Go. Since the mistake is Go
specific and happens so often a vet check will be useful.
1. https://stackoverflow.com/questions/2254014/are-there-locales-or-common-programs-that-use-yyyy-dd-mm-as-the-date-format
2. https://github.com/search?l=&p=1&q=%222006-02-01%22+language%3AGo&type=Code
Change-Id: I20960c93710766f20a7df90873bff960dea41b28
GitHub-Last-Rev: 635d7adc418ff35d865100341fda1dbf80c53a50
GitHub-Pull-Request: golang/tools#342
---
A go/analysis/passes/timeformat/testdata/src/a/a.go
A go/analysis/passes/timeformat/timeformat.go
A go/analysis/passes/timeformat/timeformat_test.go
M gopls/doc/analyzers.md
M internal/lsp/source/api_json.go
M internal/lsp/source/options.go
6 files changed, 150 insertions(+), 0 deletions(-)
To view, visit change 354010. To unsubscribe, or for help writing mail filters, visit settings.
Tim King (Gerrit)
Attention is currently required from: Ian Lance Taylor, Michael Matloob.
6 comments:
File go/analysis/passes/timeformat/testdata/src/a/a.go:
The checker should probably have an opinion about "obvious" flows through variables.
```
v := "2006-02-01 15:04:05",
_ = time.Parse(v, "2021-01-01 00:00:00")
```Should this be reported? I am guessing the answer is no.
The proposal's justification relies on this format being unused. There is a slim chance this is used legitimately (possibly an organization). We may want to provide an easy escape hatch in case there are FPs.
OTOH, we probably want a unit test that constants are reported.
```
const c = "2006-02-01 15:04:05",
_ = time.Parse(c, "2021-01-01 00:00:00") // want ....
```(Other reviewers may disagree on this.)
File go/analysis/passes/timeformat/timeformat.go:
Patch Set #4, Line 23:
2006-02-0
I think in the doc string we may need to say what 2006-02-01 means in case someone is not fully aware of the format.
Thank you for addressing this.
We should also say why it is bad.
I am not sure this has been addressed yet. How about:
The timeformat checker looks time formats with the bad 2006-02-01 (yyyy-dd-mm) format. Internationally "yyyy-dd-mm" is a [possibly] unused uncommon calendar date format. This is more likely to be a
Let's check that fn.Name() is either "Format" or "Parse" first. […]
Done
Patch Set #4, Line 46:
call.Args[0]Let's check len(call.Args) > 0 out an abundance of caution.
Done
Patch Set #4, Line 54:
pass *analysis.Passnit: Pass *types.Info instead.
Done
File go/analysis/passes/timeformat/timeformat.go:
I believe the code needs to confirm that the package of fn is the "time" package.
It would ideal to include a unit test that includes a call to a function named "Format" and "Parse" that belongs to a different package.
To view, visit change 354010. To unsubscribe, or for help writing mail filters, visit settings.
Tim King (Gerrit)
Attention is currently required from: Ian Lance Taylor, Michael Matloob.
1 comment:
Patchset:
Again, thank you for taking the time to shepherd this proposal through!
To view, visit change 354010. To unsubscribe, or for help writing mail filters, visit settings.
Gerrit Bot (Gerrit)
Attention is currently required from: Ian Lance Taylor, Michael Matloob.
Gerrit Bot uploaded patch set #7 to this change.
go/tools: add vet check for time formats with 2006-02-01
yyyy-dd-mm is a time format that isn't really used anywhere [1]. It is
much more likely that the user intended to use yyyy-mm-dd instead and
made a mistake. This happens quite often [2] because of the unusual way
to handle time formatting and parsing in Go. Since the mistake is Go
specific and happens so often a vet check will be useful.
1. https://stackoverflow.com/questions/2254014/are-there-locales-or-common-programs-that-use-yyyy-dd-mm-as-the-date-format
2. https://github.com/search?l=&p=1&q=%222006-02-01%22+language%3AGo&type=Code
Change-Id: I20960c93710766f20a7df90873bff960dea41b28
GitHub-Last-Rev: 8b04ba8e997a22116a833a5cd04be1688e62021c
GitHub-Pull-Request: golang/tools#342
---
A go/analysis/passes/timeformat/testdata/src/a/a.go
A go/analysis/passes/timeformat/testdata/src/b/b.go
A go/analysis/passes/timeformat/timeformat.go
A go/analysis/passes/timeformat/timeformat_test.go
M gopls/doc/analyzers.md
M internal/lsp/source/api_json.go
M internal/lsp/source/options.go
7 files changed, 198 insertions(+), 0 deletions(-)
To view, visit change 354010. To unsubscribe, or for help writing mail filters, visit settings.
Tim King (Gerrit)
Attention is currently required from: Ian Lance Taylor, Michael Matloob, Robert Findley.
7 comments:
Patchset:
Thank you again!
Please rebase as there is a merge conflict at the moment.
File go/analysis/passes/timeformat/testdata/src/a/a.go:
The checker should probably have an opinion about "obvious" flows through variables. […]
Done
File go/analysis/passes/timeformat/timeformat.go:
Patch Set #4, Line 7:
timeformatOh, no I just wasn't aware of that issue (or forgot about it). […]
(This still needs to be addressed.)
Patch Set #4, Line 23:
2006-02-0
> I think in the doc string we may need to say what 2006-02-01 means in case someone is not fully a […]
Done
File go/analysis/passes/timeformat/timeformat.go:
I believe the code needs to confirm that the package of fn is the "time" package. […]
Done
File go/analysis/passes/timeformat/timeformat.go:
Patch Set #7, Line 24:
unused uncommonnit: This sentence sounds odd to me at the moment. I think some editing around these two words may be helpful.
I recommend testing against `fn.Pkg().Path()`. Name() can match against other packages such as "example.com/path/to/my/time".
To view, visit change 354010. To unsubscribe, or for help writing mail filters, visit settings.
Erik Dubbelboer (Gerrit)
Attention is currently required from: Ian Lance Taylor, Michael Matloob, Robert Findley, Tim King.
9 comments:
Patchset:
File go/analysis/passes/timeformat/testdata/src/a/a.go:
The checker should probably have an opinion about "obvious" flows through variables. […]
I added a whole bunch of extra tests and fixed it so const isn't allowed but var is.
File go/analysis/passes/timeformat/timeformat.go:
Patch Set #4, Line 7:
timeformat
(This still needs to be addressed. […]
I have rebased on master and squashed all commits so this line is clearly visible.
Let's check that fn.Name() is either "Format" or "Parse" first. […]
Done
Patch Set #4, Line 46:
call.Args[0]Let's check len(call.Args) > 0 out an abundance of caution.
Done
Patch Set #4, Line 54:
pass *analysis.Passnit: Pass *types.Info instead.
Done
File go/analysis/passes/timeformat/timeformat.go:
I believe the code needs to confirm that the package of fn is the "time" package. […]
I fixed it and added a test case.
File go/analysis/passes/timeformat/timeformat.go:
Patch Set #7, Line 24:
unused uncommonnit: This sentence sounds odd to me at the moment. […]
I changed it to `Internationally "yyyy-dd-mm" is an unused and uncommon calendar date format.` which reads better.
I recommend testing against `fn.Pkg().Path()`. […]
Good point, fixed.
To view, visit change 354010. To unsubscribe, or for help writing mail filters, visit settings.
Gerrit Bot (Gerrit)
Attention is currently required from: Ian Lance Taylor, Michael Matloob, Robert Findley, Tim King.
Gerrit Bot uploaded patch set #8 to this change.
go/tools: add vet check for time formats with 2006-02-01
yyyy-dd-mm is a time format that isn't really used anywhere [1]. It is
much more likely that the user intended to use yyyy-mm-dd instead and
made a mistake. This happens quite often [2] because of the unusual way
to handle time formatting and parsing in Go. Since the mistake is Go
specific and happens so often a vet check will be useful.
1. https://stackoverflow.com/questions/2254014/are-there-locales-or-common-programs-that-use-yyyy-dd-mm-as-the-date-format
2. https://github.com/search?l=&p=1&q=%222006-02-01%22+language%3AGo&type=Code
Change-Id: I20960c93710766f20a7df90873bff960dea41b28
GitHub-Last-Rev: 8733a0ec7da883bac471823d739a4c0741a86b78
GitHub-Pull-Request: golang/tools#342
---
A go/analysis/passes/timeformat/testdata/src/a/a.go
A go/analysis/passes/timeformat/testdata/src/b/b.go
A go/analysis/passes/timeformat/timeformat.go
A go/analysis/passes/timeformat/timeformat_test.go
M gopls/doc/analyzers.md
M internal/lsp/source/api_json.go
M internal/lsp/source/options.go
7 files changed, 198 insertions(+), 0 deletions(-)
To view, visit change 354010. To unsubscribe, or for help writing mail filters, visit settings.
Tim King (Gerrit)
Attention is currently required from: Ian Lance Taylor, Michael Matloob, Robert Findley.
Patch set 8:Run-TryBot +1
kokoro (Gerrit)
Attention is currently required from: Ian Lance Taylor, Michael Matloob, Robert Findley.
Kokoro presubmit build finished with status: SUCCESS
Logs at: https://source.cloud.google.com/results/invocations/700eec58-caa0-47c8-8f08-389234615306
Patch set 8:gopls-CI +1
Robert Findley (Gerrit)
Attention is currently required from: Erik Dubbelboer, Ian Lance Taylor, Michael Matloob, Tim King.
3 comments:
File go/analysis/passes/timeformat/timeformat.go:
Patch Set #4, Line 7:
timeformat
I have rebased on master and squashed all commits so this line is clearly visible.
For some reason I still don't see this line in the Gerrit commit message.
File go/analysis/passes/timeformat/timeformat.go:
Nit: I think we should drop or replace the word "bad" here. Technically, we don't *know* it's bad. The next sentence establishes context, so I think just dropping it is fine.
Patch Set #8, Line 24:
unused and uncommon"unused and uncommon" still sounds incorrect to me (if something is unused then we don't need to say it is uncommon 😊).
How about just "uncommon", or maybe something like this:
Internationally, "yyyy-dd-mm" does not occur in common calendar date standards, and so it is more likely that 2006-01-02 (yyyy-mm-dd) was intended.
To view, visit change 354010. To unsubscribe, or for help writing mail filters, visit settings.
Gerrit Bot (Gerrit)
Attention is currently required from: Erik Dubbelboer, Ian Lance Taylor, Michael Matloob, Tim King.
Gerrit Bot uploaded patch set #9 to this change.
The following approvals got outdated and were removed: Run-TryBot+1 by Tim King, TryBot-Result+1 by Gopher Robot, gopls-CI+1 by kokoro
go/tools: add check for time formats with 2006-02-01
yyyy-dd-mm is a time format that isn't really used anywhere [1]. It is
much more likely that the user intended to use yyyy-mm-dd instead and
made a mistake. This happens quite often [2] because of the unusual way
to handle time formatting and parsing in Go. Since the mistake is Go
specific and happens so often a vet check will be useful.
Updates golang/go#48801
1. https://stackoverflow.com/questions/2254014/are-there-locales-or-common-programs-that-use-yyyy-dd-mm-as-the-date-format
2. https://github.com/search?l=&p=1&q=%222006-02-01%22+language%3AGo&type=Code
Change-Id: I20960c93710766f20a7df90873bff960dea41b28
GitHub-Last-Rev: 8733a0ec7da883bac471823d739a4c0741a86b78
GitHub-Pull-Request: golang/tools#342
---
A go/analysis/passes/timeformat/testdata/src/a/a.go
A go/analysis/passes/timeformat/testdata/src/b/b.go
A go/analysis/passes/timeformat/timeformat.go
A go/analysis/passes/timeformat/timeformat_test.go
M gopls/doc/analyzers.md
M internal/lsp/source/api_json.go
M internal/lsp/source/options.go
7 files changed, 200 insertions(+), 0 deletions(-)
To view, visit change 354010. To unsubscribe, or for help writing mail filters, visit settings.
Erik Dubbelboer (Gerrit)
Attention is currently required from: Ian Lance Taylor, Michael Matloob, Robert Findley, Tim King.
4 comments:
Commit Message:
Patch Set #4, Line 7:
vet checknit: this is not a vet check (yet), it is just an analyzer.
Done
File go/analysis/passes/timeformat/timeformat.go:
Patch Set #4, Line 7:
timeformat
For some reason I still don't see this line in the Gerrit commit message.
Editing the pull request message on Github seems to have done the trick.
File go/analysis/passes/timeformat/timeformat.go:
Nit: I think we should drop or replace the word "bad" here. Technically, we don't *know* it's bad. […]
Done
Patch Set #8, Line 24:
unused and uncommon
"unused and uncommon" still sounds incorrect to me (if something is unused then we don't need to say […]
Done
To view, visit change 354010. To unsubscribe, or for help writing mail filters, visit settings.
Gerrit Bot (Gerrit)
Attention is currently required from: Ian Lance Taylor, Michael Matloob, Robert Findley, Tim King.
Gerrit Bot uploaded patch set #10 to this change.
go/tools: add check for time formats with 2006-02-01
yyyy-dd-mm is a time format that isn't really used anywhere [1]. It is
much more likely that the user intended to use yyyy-mm-dd instead and
made a mistake. This happens quite often [2] because of the unusual way
to handle time formatting and parsing in Go. Since the mistake is Go
specific and happens so often a vet check will be useful.
Updates golang/go#48801
1. https://stackoverflow.com/questions/2254014/are-there-locales-or-common-programs-that-use-yyyy-dd-mm-as-the-date-format
2. https://github.com/search?l=&p=1&q=%222006-02-01%22+language%3AGo&type=Code
Change-Id: I20960c93710766f20a7df90873bff960dea41b28
GitHub-Last-Rev: 77e76003bafcc9e74d2ae7768f8e5405c32aa44e
GitHub-Pull-Request: golang/tools#342
---
A go/analysis/passes/timeformat/testdata/src/a/a.go
A go/analysis/passes/timeformat/testdata/src/b/b.go
A go/analysis/passes/timeformat/timeformat.go
A go/analysis/passes/timeformat/timeformat_test.go
M gopls/doc/analyzers.md
M internal/lsp/source/api_json.go
M internal/lsp/source/options.go
7 files changed, 200 insertions(+), 0 deletions(-)
To view, visit change 354010. To unsubscribe, or for help writing mail filters, visit settings.
Robert Findley (Gerrit)
Attention is currently required from: Ian Lance Taylor, Michael Matloob, Tim King.
2 comments:
Commit Message:
Patch Set #10, Line 15:
Updates golang/go#48801Sorry to be a pain: can you put this below (1) and (2) below (but ABOVE Change-Id)? It should be after the full body of the commit message
https://github.com/golang/go/wiki/CommitMessage
File go/analysis/passes/timeformat/timeformat.go:
Patch Set #10, Line 50:
name != "Format"I think we should be more precise here. This check is aimed at verifying that the function is time.Time.Format, but only checks that the func/method name is Format. Right now that is sufficient, but if in the future we ever add e.g. time.Duration.Format, this filter would be wrong.
```
func isTimeDotFormat(f *types.Func) bool {
if f.Name() != "Format" || f.Pkg().Path() != "time" {
return false
}
// verify that the receiver is time.Time
recv := f.Recv()
if recv == nil {
return false
}
named, ok := recv.Type().(*types.Named)
if !ok {
return false
}
return named.Obj().Name() == "Time"
}
```
To view, visit change 354010. To unsubscribe, or for help writing mail filters, visit settings.
Tim King (Gerrit)
Attention is currently required from: Erik Dubbelboer, Ian Lance Taylor, Michael Matloob, Robert Findley.
4 comments:
File go/analysis/passes/timeformat/timeformat.go:
Patch Set #4, Line 7:
timeformat
Editing the pull request message on Github seems to have done the trick.
Done
File go/analysis/passes/timeformat/timeformat.go:
Patch Set #7, Line 24:
unused uncommonI changed it to `Internationally "yyyy-dd-mm" is an unused and uncommon calendar date format. […]
Done
Good point, fixed.
Done
File go/analysis/passes/timeformat/timeformat.go:
Patch Set #10, Line 50:
name != "Format"
I think we should be more precise here. This check is aimed at verifying that the function is time. […]
Q orthogonal to approval. rfindley@ Feels like this is a repeated pattern that is long and tricky to get right. Should we have a utility function in passes/internal for this? Might be too many subtly different edge cases to standardize.
To view, visit change 354010. To unsubscribe, or for help writing mail filters, visit settings.
Robert Findley (Gerrit)
Attention is currently required from: Erik Dubbelboer, Ian Lance Taylor, Michael Matloob, Tim King.
1 comment:
File go/analysis/passes/timeformat/timeformat.go:
Patch Set #10, Line 50:
name != "Format"
Q orthogonal to approval. […]
Yeah, I was surprised that such a function didn't exist (or maybe it does, and I failed to find it). Perhaps for a separate CL?
To view, visit change 354010. To unsubscribe, or for help writing mail filters, visit settings.
Tim King (Gerrit)
Attention is currently required from: Erik Dubbelboer, Ian Lance Taylor, Michael Matloob, Robert Findley.
1 comment:
File go/analysis/passes/timeformat/timeformat.go:
Patch Set #10, Line 50:
name != "Format"
Perhaps for a separate CL?
SGTM
To view, visit change 354010. To unsubscribe, or for help writing mail filters, visit settings.
Gerrit Bot (Gerrit)
Attention is currently required from: Erik Dubbelboer, Ian Lance Taylor, Michael Matloob.
Gerrit Bot uploaded patch set #11 to this change.
go/tools: add check for time formats with 2006-02-01
yyyy-dd-mm is a time format that isn't really used anywhere [1]. It is
much more likely that the user intended to use yyyy-mm-dd instead and
made a mistake. This happens quite often [2] because of the unusual way
to handle time formatting and parsing in Go. Since the mistake is Go
specific and happens so often a vet check will be useful.
1. https://stackoverflow.com/questions/2254014/are-there-locales-or-common-programs-that-use-yyyy-dd-mm-as-the-date-format
2. https://github.com/search?l=&p=1&q=%222006-02-01%22+language%3AGo&type=Code
Updates golang/go#48801
Change-Id: I20960c93710766f20a7df90873bff960dea41b28
GitHub-Last-Rev: 77e76003bafcc9e74d2ae7768f8e5405c32aa44e
GitHub-Pull-Request: golang/tools#342
---
A go/analysis/passes/timeformat/testdata/src/a/a.go
A go/analysis/passes/timeformat/testdata/src/b/b.go
A go/analysis/passes/timeformat/timeformat.go
A go/analysis/passes/timeformat/timeformat_test.go
M gopls/doc/analyzers.md
M internal/lsp/source/api_json.go
M internal/lsp/source/options.go
7 files changed, 200 insertions(+), 0 deletions(-)
To view, visit change 354010. To unsubscribe, or for help writing mail filters, visit settings.
Gerrit Bot (Gerrit)
Attention is currently required from: Erik Dubbelboer, Ian Lance Taylor, Michael Matloob.
Gerrit Bot uploaded patch set #12 to this change.
go/tools: add check for time formats with 2006-02-01
yyyy-dd-mm is a time format that isn't really used anywhere [1]. It is
much more likely that the user intended to use yyyy-mm-dd instead and
made a mistake. This happens quite often [2] because of the unusual way
to handle time formatting and parsing in Go. Since the mistake is Go
specific and happens so often a vet check will be useful.
1. https://stackoverflow.com/questions/2254014/are-there-locales-or-common-programs-that-use-yyyy-dd-mm-as-the-date-format
2. https://github.com/search?l=&p=1&q=%222006-02-01%22+language%3AGo&type=Code
Updates golang/go#48801
Change-Id: I20960c93710766f20a7df90873bff960dea41b28
GitHub-Last-Rev: 5d16f9963c709fca31714516051af4f8d9818f4f
GitHub-Pull-Request: golang/tools#342
---
A go/analysis/passes/timeformat/testdata/src/a/a.go
A go/analysis/passes/timeformat/testdata/src/b/b.go
A go/analysis/passes/timeformat/timeformat.go
A go/analysis/passes/timeformat/timeformat_test.go
M gopls/doc/analyzers.md
M internal/lsp/source/api_json.go
M internal/lsp/source/options.go
7 files changed, 230 insertions(+), 0 deletions(-)
To view, visit change 354010. To unsubscribe, or for help writing mail filters, visit settings.
Erik Dubbelboer (Gerrit)
Attention is currently required from: Ian Lance Taylor, Michael Matloob, Robert Findley, Tim King.
2 comments:
Commit Message:
Patch Set #10, Line 15:
Updates golang/go#48801
Sorry to be a pain: can you put this below (1) and (2) below (but ABOVE Change-Id)? It should be aft […]
Ack
File go/analysis/passes/timeformat/timeformat.go:
Patch Set #10, Line 50:
name != "Format"
> Perhaps for a separate CL? […]
I have added isTimeDotFormat and isTimeDotParse functions. They are both slightly different as time.Parse doesn't have a receiver.
To view, visit change 354010. To unsubscribe, or for help writing mail filters, visit settings.
Tim King (Gerrit)
Attention is currently required from: Ian Lance Taylor, Michael Matloob, Robert Findley.
Patch set 12:Code-Review +2
1 comment:
File go/analysis/passes/timeformat/timeformat.go:
named, ok := recv.Type().(*types.Named)
if !ok {
return false
}
return named.Obj().Name() == "Time"
nit: Suggested form:
```
named, ok := recv.Type().(*types.Named)
return ok && named.Obj().Name() == "Time"
```(Just a "nit pick". You can choose to ignore this.)
To view, visit change 354010. To unsubscribe, or for help writing mail filters, visit settings.
Erik Dubbelboer (Gerrit)
Attention is currently required from: Ian Lance Taylor, Michael Matloob, Robert Findley.
1 comment:
File go/analysis/passes/timeformat/timeformat.go:
named, ok := recv.Type().(*types.Named)
if !ok {
return false
}
return named.Obj().Name() == "Time"
nit: Suggested form: […]
Ack
To view, visit change 354010. To unsubscribe, or for help writing mail filters, visit settings.
Gerrit Bot (Gerrit)
Attention is currently required from: Ian Lance Taylor, Michael Matloob, Robert Findley.
Gerrit Bot uploaded patch set #13 to this change.
go/tools: add check for time formats with 2006-02-01
yyyy-dd-mm is a time format that isn't really used anywhere [1]. It is
much more likely that the user intended to use yyyy-mm-dd instead and
made a mistake. This happens quite often [2] because of the unusual way
to handle time formatting and parsing in Go. Since the mistake is Go
specific and happens so often a vet check will be useful.
1. https://stackoverflow.com/questions/2254014/are-there-locales-or-common-programs-that-use-yyyy-dd-mm-as-the-date-format
2. https://github.com/search?l=&p=1&q=%222006-02-01%22+language%3AGo&type=Code
Updates golang/go#48801
Change-Id: I20960c93710766f20a7df90873bff960dea41b28
GitHub-Last-Rev: 139641140e84fc6fabbee55ecbb78d0fe6630d3d
GitHub-Pull-Request: golang/tools#342
---
A go/analysis/passes/timeformat/testdata/src/a/a.go
A go/analysis/passes/timeformat/testdata/src/b/b.go
A go/analysis/passes/timeformat/timeformat.go
A go/analysis/passes/timeformat/timeformat_test.go
M gopls/doc/analyzers.md
M internal/lsp/source/api_json.go
M internal/lsp/source/options.go
7 files changed, 223 insertions(+), 0 deletions(-)
To view, visit change 354010. To unsubscribe, or for help writing mail filters, visit settings.
Robert Findley (Gerrit)
Attention is currently required from: Ian Lance Taylor, Michael Matloob.
Patch set 13:Run-TryBot +1
2 comments:
Patchset:
I am guessing that TryBots will fail on this CL, due to it needing a rebase.
File go/analysis/passes/timeformat/timeformat.go:
Patch Set #4, Line 47:
ReportRangefIt would be trivial to include a suggested fix here (see go/analysis. […]
(not necessary for this CL)
To view, visit change 354010. To unsubscribe, or for help writing mail filters, visit settings.
kokoro (Gerrit)
Attention is currently required from: Ian Lance Taylor, Michael Matloob.
Kokoro presubmit build finished with status: SUCCESS
Logs at: https://source.cloud.google.com/results/invocations/7224d2dc-2eb6-434c-9183-bdcd34958c15
Patch set 13:gopls-CI +1
Erik Dubbelboer (Gerrit)
Attention is currently required from: Ian Lance Taylor, Michael Matloob, Robert Findley.
2 comments:
Patchset:
I am guessing that TryBots will fail on this CL, due to it needing a rebase.
It wasn't needed to pass but I rebased it.
File go/analysis/passes/timeformat/timeformat.go:
Patch Set #4, Line 47:
ReportRangef
(not necessary for this CL)
It was easy so I added it.
To view, visit change 354010. To unsubscribe, or for help writing mail filters, visit settings.
Gerrit Bot (Gerrit)
Attention is currently required from: Ian Lance Taylor, Michael Matloob, Robert Findley.
Gerrit Bot uploaded patch set #14 to this change.
The following approvals got outdated and were removed: Run-TryBot+1 by Robert Findley, TryBot-Result+1 by Gopher Robot, gopls-CI+1 by kokoro
go/tools: add check for time formats with 2006-02-01
yyyy-dd-mm is a time format that isn't really used anywhere [1]. It is
much more likely that the user intended to use yyyy-mm-dd instead and
made a mistake. This happens quite often [2] because of the unusual way
to handle time formatting and parsing in Go. Since the mistake is Go
specific and happens so often a vet check will be useful.
1. https://stackoverflow.com/questions/2254014/are-there-locales-or-common-programs-that-use-yyyy-dd-mm-as-the-date-format
2. https://github.com/search?l=&p=1&q=%222006-02-01%22+language%3AGo&type=Code
Updates golang/go#48801
Change-Id: I20960c93710766f20a7df90873bff960dea41b28
GitHub-Last-Rev: a717a0a3db5b7d255f6fd42ca8b7fbbf5e890722
GitHub-Pull-Request: golang/tools#342
---
A go/analysis/passes/timeformat/testdata/src/a/a.go
A go/analysis/passes/timeformat/testdata/src/b/b.go
A go/analysis/passes/timeformat/timeformat.go
A go/analysis/passes/timeformat/timeformat_test.go
M gopls/doc/analyzers.md
M internal/lsp/source/api_json.go
M internal/lsp/source/options.go
7 files changed, 235 insertions(+), 0 deletions(-)
To view, visit change 354010. To unsubscribe, or for help writing mail filters, visit settings.
Robert Findley (Gerrit)
Attention is currently required from: Erik Dubbelboer, Ian Lance Taylor, Michael Matloob.
1 comment:
File go/analysis/passes/timeformat/timeformat.go:
Patch Set #4, Line 47:
ReportRangef
It was easy so I added it.
Cool, thanks!
Since you've done this in this CL, can you please update the test data as well? You can test suggested fixes by using "analysistest.RunWithSuggestedFixes". The docstring for that function should explain how it works, and there are several existing usages in x/tools that you can refer to.
To view, visit change 354010. To unsubscribe, or for help writing mail filters, visit settings.
Erik Dubbelboer (Gerrit)
Attention is currently required from: Ian Lance Taylor, Michael Matloob, Robert Findley.
1 comment:
File go/analysis/passes/timeformat/timeformat.go:
Patch Set #4, Line 47:
ReportRangef
Cool, thanks! […]
I have added this and also had to make some changes to correct the suggested fixes.
To view, visit change 354010. To unsubscribe, or for help writing mail filters, visit settings.
Gerrit Bot (Gerrit)
Attention is currently required from: Ian Lance Taylor, Michael Matloob, Robert Findley.
Gerrit Bot uploaded patch set #15 to this change.
go/tools: add check for time formats with 2006-02-01
yyyy-dd-mm is a time format that isn't really used anywhere [1]. It is
much more likely that the user intended to use yyyy-mm-dd instead and
made a mistake. This happens quite often [2] because of the unusual way
to handle time formatting and parsing in Go. Since the mistake is Go
specific and happens so often a vet check will be useful.
1. https://stackoverflow.com/questions/2254014/are-there-locales-or-common-programs-that-use-yyyy-dd-mm-as-the-date-format
2. https://github.com/search?l=&p=1&q=%222006-02-01%22+language%3AGo&type=Code
Updates golang/go#48801
Change-Id: I20960c93710766f20a7df90873bff960dea41b28
GitHub-Last-Rev: 496b9917b5eda0525cb75d04a3487d174ccf8fea
GitHub-Pull-Request: golang/tools#342
---
A go/analysis/passes/timeformat/testdata/src/a/a.go
A go/analysis/passes/timeformat/testdata/src/a/a.go.golden
A go/analysis/passes/timeformat/testdata/src/b/b.go
A go/analysis/passes/timeformat/timeformat.go
A go/analysis/passes/timeformat/timeformat_test.go
M gopls/doc/analyzers.md
M internal/lsp/source/api_json.go
M internal/lsp/source/options.go
8 files changed, 304 insertions(+), 0 deletions(-)
To view, visit change 354010. To unsubscribe, or for help writing mail filters, visit settings.
Tim King (Gerrit)
Attention is currently required from: Ian Lance Taylor, Michael Matloob, Robert Findley.
Patch set 15:Run-TryBot +1Code-Review +2
kokoro (Gerrit)
Attention is currently required from: Ian Lance Taylor, Michael Matloob, Robert Findley.
Kokoro presubmit build finished with status: SUCCESS
Logs at: https://source.cloud.google.com/results/invocations/7d7b16e4-c82c-4a8c-a02e-2216b5978689
Patch set 15:gopls-CI +1
Robert Findley (Gerrit)
Attention is currently required from: Ian Lance Taylor, Michael Matloob.
Patch set 15:Code-Review +1
Tim King (Gerrit)
Tim King submitted this change.
go/tools: add check for time formats with 2006-02-01
yyyy-dd-mm is a time format that isn't really used anywhere [1]. It is
much more likely that the user intended to use yyyy-mm-dd instead and
made a mistake. This happens quite often [2] because of the unusual way
to handle time formatting and parsing in Go. Since the mistake is Go
specific and happens so often a vet check will be useful.
1. https://stackoverflow.com/questions/2254014/are-there-locales-or-common-programs-that-use-yyyy-dd-mm-as-the-date-format
2. https://github.com/search?l=&p=1&q=%222006-02-01%22+language%3AGo&type=Code
Updates golang/go#48801
Change-Id: I20960c93710766f20a7df90873bff960dea41b28
GitHub-Last-Rev: 496b9917b5eda0525cb75d04a3487d174ccf8fea
GitHub-Pull-Request: golang/tools#342
Reviewed-on: https://go-review.googlesource.com/c/tools/+/354010
gopls-CI: kokoro <noreply...@google.com>
TryBot-Result: Gopher Robot <go...@golang.org>
Reviewed-by: Tim King <tak...@google.com>
Run-TryBot: Tim King <tak...@google.com>
Reviewed-by: Robert Findley <rfin...@google.com>
---
A go/analysis/passes/timeformat/testdata/src/a/a.go
A go/analysis/passes/timeformat/testdata/src/a/a.go.golden
A go/analysis/passes/timeformat/testdata/src/b/b.go
A go/analysis/passes/timeformat/timeformat.go
A go/analysis/passes/timeformat/timeformat_test.go
M gopls/doc/analyzers.md
M internal/lsp/source/api_json.go
M internal/lsp/source/options.go
8 files changed, 310 insertions(+), 0 deletions(-)
diff --git a/go/analysis/passes/timeformat/testdata/src/a/a.go b/go/analysis/passes/timeformat/testdata/src/a/a.go
new file mode 100644
index 0000000..9848144
--- /dev/null
+++ b/go/analysis/passes/timeformat/testdata/src/a/a.go
@@ -0,0 +1,50 @@
+// Copyright 2022 The Go Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style
+// license that can be found in the LICENSE file.
+
+// This file contains tests for the timeformat checker.
+
+package a
+
+import (
+ "time"
+
+ "b"
+)
+
+func hasError() {
+ a, _ := time.Parse("2006-02-01 15:04:05", "2021-01-01 00:00:00") // want `2006-02-01 should be 2006-01-02`
+ a.Format(`2006-02-01`) // want `2006-02-01 should be 2006-01-02`
+ a.Format("2006-02-01 15:04:05") // want `2006-02-01 should be 2006-01-02`
+
+ const c = "2006-02-01"
+ a.Format(c) // want `2006-02-01 should be 2006-01-02`
+}
+
+func notHasError() {
+ a, _ := time.Parse("2006-01-02 15:04:05", "2021-01-01 00:00:00")
+ a.Format("2006-01-02")
+
+ const c = "2006-01-02"
+ a.Format(c)
+
+ v := "2006-02-01"
+ a.Format(v) // Allowed though variables.
+
+ m := map[string]string{
+ "y": "2006-02-01",
+ }
+ a.Format(m["y"])
+
+ s := []string{"2006-02-01"}
+ a.Format(s[0])
+
+ a.Format(badFormat())
+
+ o := b.Parse("2006-02-01 15:04:05", "2021-01-01 00:00:00")
+ o.Format("2006-02-01")
+}
+
+func badFormat() string {
+ return "2006-02-01"
+}
diff --git a/go/analysis/passes/timeformat/testdata/src/a/a.go.golden b/go/analysis/passes/timeformat/testdata/src/a/a.go.golden
new file mode 100644
index 0000000..9eccded
--- /dev/null
+++ b/go/analysis/passes/timeformat/testdata/src/a/a.go.golden
@@ -0,0 +1,50 @@
+// Copyright 2022 The Go Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style
+// license that can be found in the LICENSE file.
+
+// This file contains tests for the timeformat checker.
+
+package a
+
+import (
+ "time"
+
+ "b"
+)
+
+func hasError() {
+ a, _ := time.Parse("2006-01-02 15:04:05", "2021-01-01 00:00:00") // want `2006-02-01 should be 2006-01-02`
+ a.Format(`2006-01-02`) // want `2006-02-01 should be 2006-01-02`
+ a.Format("2006-01-02 15:04:05") // want `2006-02-01 should be 2006-01-02`
+
+ const c = "2006-02-01"
+ a.Format(c) // want `2006-02-01 should be 2006-01-02`
+}
+
+func notHasError() {
+ a, _ := time.Parse("2006-01-02 15:04:05", "2021-01-01 00:00:00")
+ a.Format("2006-01-02")
+
+ const c = "2006-01-02"
+ a.Format(c)
+
+ v := "2006-02-01"
+ a.Format(v) // Allowed though variables.
+
+ m := map[string]string{
+ "y": "2006-02-01",
+ }
+ a.Format(m["y"])
+
+ s := []string{"2006-02-01"}
+ a.Format(s[0])
+
+ a.Format(badFormat())
+
+ o := b.Parse("2006-02-01 15:04:05", "2021-01-01 00:00:00")
+ o.Format("2006-02-01")
+}
+
+func badFormat() string {
+ return "2006-02-01"
+}
diff --git a/go/analysis/passes/timeformat/testdata/src/b/b.go b/go/analysis/passes/timeformat/testdata/src/b/b.go
new file mode 100644
index 0000000..de56908
--- /dev/null
+++ b/go/analysis/passes/timeformat/testdata/src/b/b.go
@@ -0,0 +1,11 @@
+package b
+
+type B struct {
+}
+
+func Parse(string, string) B {
+ return B{}
+}
+
+func (b B) Format(string) {
+}
diff --git a/go/analysis/passes/timeformat/timeformat.go b/go/analysis/passes/timeformat/timeformat.go
new file mode 100644
index 0000000..9147826
--- /dev/null
+++ b/go/analysis/passes/timeformat/timeformat.go
@@ -0,0 +1,131 @@
+// Copyright 2022 The Go Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style
+// license that can be found in the LICENSE file.
+
+// Package timeformat defines an Analyzer that checks for the use
+// of time.Format or time.Parse calls with a bad format.
+package timeformat
+
+import (
+ "fmt"
+ "go/ast"
+ "go/constant"
+ "go/token"
+ "go/types"
+ "strings"
+
+ "golang.org/x/tools/go/analysis"
+ "golang.org/x/tools/go/analysis/passes/inspect"
+ "golang.org/x/tools/go/ast/inspector"
+ "golang.org/x/tools/go/types/typeutil"
+)
+
+const badFormat = "2006-02-01"
+const goodFormat = "2006-01-02"
+
+const Doc = `check for calls of (time.Time).Format or time.Parse with 2006-02-01
+
+The timeformat checker looks for time formats with the 2006-02-01 (yyyy-dd-mm)
+format. Internationally, "yyyy-dd-mm" does not occur in common calendar date
+standards, and so it is more likely that 2006-01-02 (yyyy-mm-dd) was intended.
+`
+
+var Analyzer = &analysis.Analyzer{
+ Name: "timeformat",
+ Doc: Doc,
+ Requires: []*analysis.Analyzer{inspect.Analyzer},
+ Run: run,
+}
+
+func run(pass *analysis.Pass) (interface{}, error) {
+ inspect := pass.ResultOf[inspect.Analyzer].(*inspector.Inspector)
+
+ nodeFilter := []ast.Node{
+ (*ast.CallExpr)(nil),
+ }
+ inspect.Preorder(nodeFilter, func(n ast.Node) {
+ call := n.(*ast.CallExpr)
+ fn, ok := typeutil.Callee(pass.TypesInfo, call).(*types.Func)
+ if !ok {
+ return
+ }
+ if !isTimeDotFormat(fn) && !isTimeDotParse(fn) {
+ return
+ }
+ if len(call.Args) > 0 {
+ arg := call.Args[0]
+ badAt := badFormatAt(pass.TypesInfo, arg)
+
+ if badAt > -1 {
+ // Check if it's a literal string, otherwise we can't suggest a fix.
+ if _, ok := arg.(*ast.BasicLit); ok {
+ fmt.Printf("%#v\n", arg)
+ pos := int(arg.Pos()) + badAt + 1 // +1 to skip the " or `
+ end := pos + len(badFormat)
+
+ pass.Report(analysis.Diagnostic{
+ Pos: token.Pos(pos),
+ End: token.Pos(end),
+ Message: badFormat + " should be " + goodFormat,
+ SuggestedFixes: []analysis.SuggestedFix{{
+ Message: "Replace " + badFormat + " with " + goodFormat,
+ TextEdits: []analysis.TextEdit{{
+ Pos: token.Pos(pos),
+ End: token.Pos(end),
+ NewText: []byte(goodFormat),
+ }},
+ }},
+ })
+ } else {
+ pass.Reportf(arg.Pos(), badFormat+" should be "+goodFormat)
+ }
+ }
+ }
+ })
+ return nil, nil
+}
+
+func isTimeDotFormat(f *types.Func) bool {
+ if f.Name() != "Format" || f.Pkg().Path() != "time" {
+ return false
+ }
+ sig, ok := f.Type().(*types.Signature)
+ if !ok {
+ return false
+ }
+ // Verify that the receiver is time.Time.
+ recv := sig.Recv()
+ if recv == nil {
+ return false
+ }
+ named, ok := recv.Type().(*types.Named)
+ return ok && named.Obj().Name() == "Time"
+}
+
+func isTimeDotParse(f *types.Func) bool {
+ if f.Name() != "Parse" || f.Pkg().Path() != "time" {
+ return false
+ }
+ // Verify that there is no receiver.
+ sig, ok := f.Type().(*types.Signature)
+ return ok && sig.Recv() == nil
+}
+
+// badFormatAt return the start of a bad format in e or -1 if no bad format is found.
+func badFormatAt(info *types.Info, e ast.Expr) int {
+ tv, ok := info.Types[e]
+ if !ok { // no type info, assume good
+ return -1
+ }
+
+ t, ok := tv.Type.(*types.Basic)
+ if !ok || t.Info()&types.IsString == 0 {
+ return -1
+ }
+
+ if tv.Value == nil {
+ return -1
+ }
+
+ return strings.Index(constant.StringVal(tv.Value), badFormat)
+}
diff --git a/go/analysis/passes/timeformat/timeformat_test.go b/go/analysis/passes/timeformat/timeformat_test.go
new file mode 100644
index 0000000..86bbe1b
--- /dev/null
+++ b/go/analysis/passes/timeformat/timeformat_test.go
@@ -0,0 +1,17 @@
+// Copyright 2022 The Go Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style
+// license that can be found in the LICENSE file.
+
+package timeformat_test
+
+import (
+ "testing"
+
+ "golang.org/x/tools/go/analysis/analysistest"
+ "golang.org/x/tools/go/analysis/passes/timeformat"
+)
+
+func Test(t *testing.T) {
+ testdata := analysistest.TestData()
+ analysistest.RunWithSuggestedFixes(t, testdata, timeformat.Analyzer, "a")
+}
diff --git a/gopls/doc/analyzers.md b/gopls/doc/analyzers.md
index 861be1c..90a5118 100644
--- a/gopls/doc/analyzers.md
+++ b/gopls/doc/analyzers.md
@@ -497,6 +497,17 @@
**Enabled by default.**
+## **timeformat**
+
+check for calls of (time.Time).Format or time.Parse with 2006-02-01
+
+The timeformat checker looks for time formats with the 2006-02-01 (yyyy-dd-mm)
+format. Internationally, "yyyy-dd-mm" does not occur in common calendar date
+standards, and so it is more likely that 2006-01-02 (yyyy-mm-dd) was intended.
+
+
+**Enabled by default.**
+
## **unmarshal**
report passing non-pointer or non-interface values to unmarshal
diff --git a/internal/lsp/source/api_json.go b/internal/lsp/source/api_json.go
index 807f496..e20b8a6 100755
--- a/internal/lsp/source/api_json.go
+++ b/internal/lsp/source/api_json.go
@@ -379,6 +379,11 @@
Default: "true",
},
{
+ Name: "\"timeformat\"",
+ Doc: "check for calls of (time.Time).Format or time.Parse with 2006-02-01\n\nThe timeformat checker looks for time formats with the 2006-02-01 (yyyy-dd-mm)\nformat. Internationally, \"yyyy-dd-mm\" does not occur in common calendar date\nstandards, and so it is more likely that 2006-01-02 (yyyy-mm-dd) was intended.\n",
+ Default: "true",
+ },
+ {
Name: "\"unmarshal\"",
Doc: "report passing non-pointer or non-interface values to unmarshal\n\nThe unmarshal analysis reports calls to functions such as json.Unmarshal\nin which the argument type is not a pointer or an interface.",
Default: "true",
@@ -967,6 +972,11 @@
Default: true,
},
{
+ Name: "timeformat",
+ Doc: "check for calls of (time.Time).Format or time.Parse with 2006-02-01\n\nThe timeformat checker looks for time formats with the 2006-02-01 (yyyy-dd-mm)\nformat. Internationally, \"yyyy-dd-mm\" does not occur in common calendar date\nstandards, and so it is more likely that 2006-01-02 (yyyy-mm-dd) was intended.\n",
+ Default: true,
+ },
+ {
Name: "unmarshal",
Doc: "report passing non-pointer or non-interface values to unmarshal\n\nThe unmarshal analysis reports calls to functions such as json.Unmarshal\nin which the argument type is not a pointer or an interface.",
Default: true,
diff --git a/internal/lsp/source/options.go b/internal/lsp/source/options.go
index 8abcc74..6881a7c 100644
--- a/internal/lsp/source/options.go
+++ b/internal/lsp/source/options.go
@@ -43,6 +43,7 @@
"golang.org/x/tools/go/analysis/passes/structtag"
"golang.org/x/tools/go/analysis/passes/testinggoroutine"
"golang.org/x/tools/go/analysis/passes/tests"
+ "golang.org/x/tools/go/analysis/passes/timeformat"
"golang.org/x/tools/go/analysis/passes/unmarshal"
"golang.org/x/tools/go/analysis/passes/unreachable"
"golang.org/x/tools/go/analysis/passes/unsafeptr"
@@ -1393,6 +1394,7 @@
useany.Analyzer.Name: {Analyzer: useany.Analyzer, Enabled: false},
infertypeargs.Analyzer.Name: {Analyzer: infertypeargs.Analyzer, Enabled: true},
embeddirective.Analyzer.Name: {Analyzer: embeddirective.Analyzer, Enabled: true},
+ timeformat.Analyzer.Name: {Analyzer: timeformat.Analyzer, Enabled: true},
// gofmt -s suite:
simplifycompositelit.Analyzer.Name: {
To view, visit change 354010. To unsubscribe, or for help writing mail filters, visit settings.
Tim King (Gerrit)
1 comment:
Patchset:
RELNOTE=yes
To view, visit change 354010. To unsubscribe, or for help writing mail filters, visit settings.