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.

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

go/parser: goto; is incorrectly valid #70957

Closed
mateusz834 opened this issue Dec 22, 2024 · 7 comments
Closed

go/parser: goto; is incorrectly valid #70957

mateusz834 opened this issue Dec 22, 2024 · 7 comments
Assignees
@mateusz834
Labels
NeedsFix The path to resolution is known, but the work has not been done.

Comments

@mateusz834
Copy link
Member

mateusz834 commented Dec 22, 2024 •
edited
Loading

While poking around the go/types internals, i noticed that nothing really "type-checks" gotos without labels.

First pass:

go/src/go/types/stmt.go

Lines 578 to 608 in 500675a

case *ast.BranchStmt:
if s.Label != nil {
check.hasLabel = true
return // checked in 2nd pass (check.labels)
}
switch s.Tok {
case token.BREAK:
if ctxt&breakOk == 0 {
check.error(s, MisplacedBreak, "break not in for, switch, or select statement")
}
case token.CONTINUE:
if ctxt&continueOk == 0 {
check.error(s, MisplacedContinue, "continue not in for statement")
}
case token.FALLTHROUGH:
if ctxt&fallthroughOk == 0 {
var msg string
switch {
case ctxt&finalSwitchCase != 0:
msg = "cannot fallthrough final case in switch"
case ctxt&inTypeSwitch != 0:
msg = "cannot fallthrough in type switch"
default:
msg = "fallthrough statement out of place"
}
check.error(s, MisplacedFallthrough, msg)
}
default:
check.errorf(s, InvalidSyntaxTree, "branch statement: %s", s.Tok)
}

Second pass:

go/src/go/types/labels.go

Lines 173 to 176 in 500675a

case *ast.BranchStmt:
if s.Label == nil {
return // checked in 1st pass (check.stmt)
}

Also it also turns out that:

func test() {
    goto;
}

Is accepted by the go/parser. The spec disallows gotos without labels, so we should either return an appropriate error in go/types or go/parser. Currently go/types returns an InvalidSyntaxTree error.

go/src/go/types/stmt.go

Lines 605 to 607 in 500675a

default:
check.errorf(s, InvalidSyntaxTree, "branch statement: %s", s.Tok)
}

CC @griesemer @adonovan
@gabyhelp
Copy link

Related Issues

(Emoji vote if this was helpful or unhelpful; more detailed feedback welcome in this discussion.)

@mateusz834 mateusz834 added the NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. label Dec 22, 2024
@mateusz834
Copy link
Member Author

Actually, this looks like a bug in go/parser:

go/src/go/parser/parser.go

Lines 2051 to 2056 in 500675a

pos := p.expect(tok)
var label *ast.Ident
if tok != token.FALLTHROUGH && p.tok == token.IDENT {
label = p.parseIdent()
}
p.expectSemi()

Will send a CL.

@mateusz834 mateusz834 self-assigned this Dec 22, 2024
@mateusz834
Copy link
Member Author

cmd/compile/internal/syntax currently reports this as an error:

go/src/cmd/compile/internal/syntax/parser.go

Lines 2660 to 2667 in 500675a

case _Goto:
s := new(BranchStmt)
s.pos = p.pos()
s.Tok = _Goto
p.next()
s.Label = p.name()
return s

@mateusz834 mateusz834 mentioned this issue Dec 22, 2024
Closed
@gopherbot
Copy link
Contributor

Change https://go.dev/cl/638395 mentions this issue: go/parser: require label after goto

@griesemer griesemer added NeedsFix The path to resolution is known, but the work has not been done. and removed NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. labels Jan 6, 2025
@findleyr
Copy link
Member

@griesemer did you want this fix to land for 1.24? If so, shall we milestone accordingly?

This also seems like it could wait for the tree to open.

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/651155 mentions this issue: go/analysis/passes/unreachable/testdata: relax test for CL 638395

gopherbot pushed a commit to golang/tools that referenced this issue Feb 24, 2025
@adonovan @gopherbot
go/analysis/passes/unreachable/testdata: relax test for CL 638395
d2fcd36 
This test case is about to become a parse error. To allow us
to submit the change to the parser, we must relax this test.

Updates golang/go#71659
Updates golang/go#70957

Change-Id: Ic4fbfedb69d152d691dec41a94bb402149463f84
Reviewed-on: https://go-review.googlesource.com/c/tools/+/651155
Auto-Submit: Alan Donovan <[email protected]>
LUCI-TryBot-Result: Go LUCI <[email protected]>
Reviewed-by: Robert Findley <[email protected]>
@gopherbot
Copy link
Contributor

Change https://go.dev/cl/652977 mentions this issue: [gopls-release-branch.0.18] go/analysis/passes/unreachable/testdata: relax test for CL 638395

gopherbot pushed a commit to golang/tools that referenced this issue Feb 26, 2025
@adonovan @gopherbot
[gopls-release-branch.0.18] go/analysis/passes/unreachable/testdata: …
64f801f 
…relax test for CL 638395

This test case is about to become a parse error. To allow us
to submit the change to the parser, we must relax this test.

Updates golang/go#71659
Updates golang/go#70957

Change-Id: Ic4fbfedb69d152d691dec41a94bb402149463f84
Reviewed-on: https://go-review.googlesource.com/c/tools/+/651155
Auto-Submit: Alan Donovan <[email protected]>
LUCI-TryBot-Result: Go LUCI <[email protected]>
Reviewed-by: Robert Findley <[email protected]>
(cherry picked from commit d2fcd36)
Reviewed-on: https://go-review.googlesource.com/c/tools/+/652977
Auto-Submit: Robert Findley <[email protected]>
Reviewed-by: Hongxiang Jiang <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@mateusz834 mateusz834

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

Successfully merging a pull request may close this issue.

go/parser: require label after goto mateusz834/go
5 participants
@griesemer @gopherbot @mateusz834 @findleyr @gabyhelp