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

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

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

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
Contributor

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
… 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
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
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
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
… 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
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
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
… 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
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
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.
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Development

No branches or pull requests

3 participants