Skip to content

go/types, types2: allow choice between reg. expression or not in ERROR test comments #51006

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.

Sign up for GitHub

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

Jump to bottom
Closed
griesemer opened this issue Feb 4, 2022 · 9 comments
Closed

go/types, types2: allow choice between reg. expression or not in ERROR test comments #51006

griesemer opened this issue Feb 4, 2022 · 9 comments
Assignees
@griesemer
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Milestone
Go1.21

Comments

@griesemer
Copy link
Contributor

go/types and types2 (and the syntax package) rely on ERROR comments in test cases. Such a comment accepts a regular expression pattern that is then matched against the expected error:

// ERROR regexp pattern

Often we just want to see if the error string appears verbatim (as a substring) in the error output, we don't need reg. expression matching. If we could choose a non-regexp string in those cases, we wouldn't need to escape various characters, which would make these ERROR comments more readable.

Example:

// ERROR \*T2 does not implement I1 \(wrong type for method foo\)\n\t\thave foo\(x int\)\n\t\twant foo\(\)

would become

// ERROR *T2 does not implement I1 (wrong type for method foo)\n\t\thave foo(x int)\n\t\twant foo()

The non-regexp pattern might be the more common case. Maybe we could have ERROR and ERRORx (the latter is for regexp. matching).

cc: @findleyr
@griesemer griesemer added the NeedsFix The path to resolution is known, but the work has not been done. label Feb 4, 2022
@griesemer griesemer added this to the Go1.19 milestone Feb 4, 2022
@griesemer griesemer self-assigned this Feb 4, 2022
@griesemer griesemer modified the milestones: Go1.19, Go1.20 May 11, 2022
@griesemer
Copy link
Contributor Author

Per discussion with @findleyr: Let's use ERROR /regexp/ and ERROR substring:
ERROR /regexp/ means that the error message must match the regular expression regexp
ERROR substring means that the error message must contain the substring substring

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/449736 mentions this issue: internal/types: remove unnecessary quotes ("") from error patterns

@griesemer griesemer modified the milestones: Go1.20, Go1.21 Nov 16, 2022
@gopherbot
Copy link
Contributor

Change https://go.dev/cl/455718 mentions this issue: internal/types: remove "'s around ERROR strings (cleanup)

@griesemer
Copy link
Contributor Author

@findleyr If the regexp ends in a *, the immediately following / will end a /* ERROR comment prematurely. Example: for regexp invalid type T .* we arrive at/* ERROR /invalid type T .*/ */. In other words, regexps ending in * cannot be written for /* ERROR-style comments.

We can live with this (it's rare), or we can chose a different mechanism to distinguish between substring and regexp error patterns (e.g. ERROR and ERRORx).

@findleyr
Copy link
Member

findleyr commented Dec 7, 2022

Hmm, that's a good point that I hadn't considered.

ERRORx seems like a reasonable alternative to me.

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/456137 mentions this issue: go/types: use commentMap to collect error comments

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/456125 mentions this issue: go/types: add commentMap and test

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/456155 mentions this issue: cmd/compile/internal/syntax: rename ErrorMap to CommentMap, make more flexible

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/455755 mentions this issue: go/types, types2: distinguish between substring and regexp error patterns

gopherbot pushed a commit that referenced this issue Jan 17, 2023
@griesemer @gopherbot
cmd/compile/internal/syntax: rename ErrorMap to CommentMap, make more…
1e00516 
… flexible

Change the ErrorMap function to collect all comments with a comment
text that matches a given regexp pattern. Also rename it to CommentMap.

Adjust uses and corresponding test.

Adjust various type-checker tests with incorrect ERROR patterns.

For #51006.

Change-Id: I749e8f31b532edbf8568f27ba1546dc849efd143
Reviewed-on: https://go-review.googlesource.com/c/go/+/456155
Reviewed-by: Robert Findley <rfindley@google.com>
Reviewed-by: Robert Griesemer <gri@google.com>
Run-TryBot: Robert Griesemer <gri@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Auto-Submit: Cherry Mui <cherryyz@google.com>
gopherbot pushed a commit that referenced this issue Jan 17, 2023
@griesemer @gopherbot
go/types: add commentMap and test
526b895 
This adds the (adjusted) syntax.CommentMap function and corresponding
test to the types_test package so that we can use it for collecting
ERROR comments in the next CL.

For #51006.

Change-Id: I63ce96e7394c28c02d5a292250586cc49c1f6e19
Reviewed-on: https://go-review.googlesource.com/c/go/+/456125
Reviewed-by: Robert Findley <rfindley@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Robert Griesemer <gri@google.com>
Auto-Submit: Robert Griesemer <gri@google.com>
Run-TryBot: Robert Griesemer <gri@google.com>
gopherbot pushed a commit that referenced this issue Jan 17, 2023
@griesemer @gopherbot
go/types: use commentMap to collect error comments
390eee7 
Adjust the testFiles function to use the new commentMap
function. This makes it possible for testFiles to match
the types2.TestFiles logic more closely.

For #51006.

Change-Id: I6c5ecbeb86d095404ec04ba4452fb90d404b8280
Reviewed-on: https://go-review.googlesource.com/c/go/+/456137
Reviewed-by: Robert Findley <rfindley@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Robert Griesemer <gri@google.com>
Run-TryBot: Robert Griesemer <gri@google.com>
Auto-Submit: Robert Griesemer <gri@google.com>
gopherbot pushed a commit that referenced this issue Jan 17, 2023
@griesemer @gopherbot
internal/types: consistently use double quotes around ERROR patterns
b003ee4 
Before matching the pattern, the double quotes are simply stripped
(no Go string unquoting) for now. This is a first step towards use
of proper Go strings as ERROR patterns.

The changes were obtained through a couple of global regexp
find/replace commands:

/\* ERROR ([^"]+) \*/   =>   /* ERROR "$1" */
// ERROR ([^"]+)$       =>   // ERROR "$1"

followed up by manual fixes where multiple "/* ERROR"-style
errors appeared on the same line (in that case, the first
regexp matches the first and last ERROR).

For #51006.

Change-Id: Ib92c2d5e339075aeec1ea74c339b5fecf953d1a0
Reviewed-on: https://go-review.googlesource.com/c/go/+/455718
Auto-Submit: Robert Griesemer <gri@google.com>
Reviewed-by: Robert Griesemer <gri@google.com>
Run-TryBot: Robert Griesemer <gri@google.com>
Reviewed-by: Robert Findley <rfindley@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
eric pushed a commit to fancybits/go that referenced this issue Sep 7, 2023
@griesemer @eric
cmd/compile/internal/syntax: rename ErrorMap to CommentMap, make more…
5ef2d7e 
… flexible

Change the ErrorMap function to collect all comments with a comment
text that matches a given regexp pattern. Also rename it to CommentMap.

Adjust uses and corresponding test.

Adjust various type-checker tests with incorrect ERROR patterns.

For golang#51006.

Change-Id: I749e8f31b532edbf8568f27ba1546dc849efd143
Reviewed-on: https://go-review.googlesource.com/c/go/+/456155
Reviewed-by: Robert Findley <rfindley@google.com>
Reviewed-by: Robert Griesemer <gri@google.com>
Run-TryBot: Robert Griesemer <gri@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Auto-Submit: Cherry Mui <cherryyz@google.com>
eric pushed a commit to fancybits/go that referenced this issue Sep 7, 2023
@griesemer @eric
go/types: use commentMap to collect error comments
cdfdf43 
Adjust the testFiles function to use the new commentMap
function. This makes it possible for testFiles to match
the types2.TestFiles logic more closely.

For golang#51006.

Change-Id: I6c5ecbeb86d095404ec04ba4452fb90d404b8280
Reviewed-on: https://go-review.googlesource.com/c/go/+/456137
Reviewed-by: Robert Findley <rfindley@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Robert Griesemer <gri@google.com>
Run-TryBot: Robert Griesemer <gri@google.com>
Auto-Submit: Robert Griesemer <gri@google.com>
eric pushed a commit to fancybits/go that referenced this issue Sep 7, 2023
@griesemer @eric
internal/types: consistently use double quotes around ERROR patterns
46bbe87 
Before matching the pattern, the double quotes are simply stripped
(no Go string unquoting) for now. This is a first step towards use
of proper Go strings as ERROR patterns.

The changes were obtained through a couple of global regexp
find/replace commands:

/\* ERROR ([^"]+) \*/   =>   /* ERROR "$1" */
// ERROR ([^"]+)$       =>   // ERROR "$1"

followed up by manual fixes where multiple "/* ERROR"-style
errors appeared on the same line (in that case, the first
regexp matches the first and last ERROR).

For golang#51006.

Change-Id: Ib92c2d5e339075aeec1ea74c339b5fecf953d1a0
Reviewed-on: https://go-review.googlesource.com/c/go/+/455718
Auto-Submit: Robert Griesemer <gri@google.com>
Reviewed-by: Robert Griesemer <gri@google.com>
Run-TryBot: Robert Griesemer <gri@google.com>
Reviewed-by: Robert Findley <rfindley@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
eric pushed a commit to fancybits/go that referenced this issue Sep 7, 2023
@griesemer @eric
cmd/compile/internal/syntax: rename ErrorMap to CommentMap, make more…
0adb0a3 
… flexible

Change the ErrorMap function to collect all comments with a comment
text that matches a given regexp pattern. Also rename it to CommentMap.

Adjust uses and corresponding test.

Adjust various type-checker tests with incorrect ERROR patterns.

For golang#51006.

Change-Id: I749e8f31b532edbf8568f27ba1546dc849efd143
Reviewed-on: https://go-review.googlesource.com/c/go/+/456155
Reviewed-by: Robert Findley <rfindley@google.com>
Reviewed-by: Robert Griesemer <gri@google.com>
Run-TryBot: Robert Griesemer <gri@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Auto-Submit: Cherry Mui <cherryyz@google.com>
eric pushed a commit to fancybits/go that referenced this issue Sep 7, 2023
@griesemer @eric
go/types: use commentMap to collect error comments
4944b31 
Adjust the testFiles function to use the new commentMap
function. This makes it possible for testFiles to match
the types2.TestFiles logic more closely.

For golang#51006.

Change-Id: I6c5ecbeb86d095404ec04ba4452fb90d404b8280
Reviewed-on: https://go-review.googlesource.com/c/go/+/456137
Reviewed-by: Robert Findley <rfindley@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Robert Griesemer <gri@google.com>
Run-TryBot: Robert Griesemer <gri@google.com>
Auto-Submit: Robert Griesemer <gri@google.com>
eric pushed a commit to fancybits/go that referenced this issue Sep 7, 2023
@griesemer @eric
internal/types: consistently use double quotes around ERROR patterns
40886a0 
Before matching the pattern, the double quotes are simply stripped
(no Go string unquoting) for now. This is a first step towards use
of proper Go strings as ERROR patterns.

The changes were obtained through a couple of global regexp
find/replace commands:

/\* ERROR ([^"]+) \*/   =>   /* ERROR "$1" */
// ERROR ([^"]+)$       =>   // ERROR "$1"

followed up by manual fixes where multiple "/* ERROR"-style
errors appeared on the same line (in that case, the first
regexp matches the first and last ERROR).

For golang#51006.

Change-Id: Ib92c2d5e339075aeec1ea74c339b5fecf953d1a0
Reviewed-on: https://go-review.googlesource.com/c/go/+/455718
Auto-Submit: Robert Griesemer <gri@google.com>
Reviewed-by: Robert Griesemer <gri@google.com>
Run-TryBot: Robert Griesemer <gri@google.com>
Reviewed-by: Robert Findley <rfindley@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
@golang golang locked and limited conversation to collaborators Jan 17, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees

@griesemer griesemer

Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Milestone
Go1.21
Development

No branches or pull requests
3 participants
@griesemer @gopherbot @findleyr