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

cmd/compile: error message dumps internal details #46558

Closed
ianlancetaylor opened this issue Jun 3, 2021 · 5 comments
Closed

cmd/compile: error message dumps internal details #46558

ianlancetaylor opened this issue Jun 3, 2021 · 5 comments
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Milestone
Go1.18

Comments

@ianlancetaylor
Copy link
Contributor

Using a compiler built at 6d98301, compiling this erroneous file gives a strange error message that seems to dump an internal compiler data structure.

package p

func F(s string) {
	switch s[0] {
	case 'a':
		case s[2] {
		case 'b':
		}
	}
}
> go build foo.go
# command-line-arguments
NOT NTYPE [0xc00034e0c0]
.   INDEX # foo.go:6
.   .   NAME-p.s Class:PPARAM Offset:0 OnStack # foo.go:3
.   .   NAME-ntype
.   .   .   NONAME-p.string # foo.go:3
.   .   LITERAL-2 untyped int # foo.go:6
../foo.go:7:3: syntax error: unexpected case, expecting expression
../foo.go:8:4: syntax error: unexpected newline, expecting :

This does not happen with Go 1.16.

CC @mdempsky
@ianlancetaylor ianlancetaylor added the NeedsFix The path to resolution is known, but the work has not been done. label Jun 3, 2021
@ianlancetaylor ianlancetaylor added this to the Backlog milestone Jun 3, 2021
@mdempsky
Copy link
Member

mdempsky commented Jun 3, 2021

I think there are two minor issues here:

  1. cmd/compile/internal/parser is now trying to parse s[2] { case 'b': } as a composite literal of instantiated type s[2]. This then fails during noding, because s[2] isn't a valid syntactic form for a type expression (prior to generics, which noder doesn't support). (I'm inferring this because of the change in error message/lines from 1.16 to master.)
  2. cmd/compile/internal/noder has an ir.Dump call that Russ added for debugging some ir.Node refactoring stuff. It will never print on valid input, and (I think) it also never printed before the change in parser semantics.

Maybe a parser change is appropriate here (/cc @griesemer). But I also think it's fine that we just remove the spurious ir.Dump. It's not serving any purpose now that things have stabilized.

@griesemer griesemer self-assigned this Jun 3, 2021
@griesemer
Copy link
Contributor

Yeah, the parser should probably handle this case as it gets badly into the weeds otherwise. Will fix.

@griesemer griesemer modified the milestones: Backlog, Go1.18 Jun 3, 2021
@gopherbot
Copy link

Change https://golang.org/cl/324991 mentions this issue: cmd/compile: remove spurious ir.Dump

gopherbot pushed a commit that referenced this issue Jun 4, 2021
@mdempsky
cmd/compile: remove spurious ir.Dump
b29b123 
This ir.Dump call is a debugging artifact introduced in
golang.org/cl/274103, which should never be printed for valid,
non-generic code, but evidently can now sometimes appear due to how
the parser handles invalid syntax.

The parser should probably not recognize "x[2]" as a type expression
in non-generics mode, but also probably we shouldn't try noding after
reporting syntax errors. Either way, this diagnostic has outlived its
usefulness, and noder's days are numbered anyway, so we might as well
just remove it to save end users any confusion.

Updates #46558.

Change-Id: Ib68502ef834d610b883c2f2bb11d9b385bc66e37
Reviewed-on: https://go-review.googlesource.com/c/go/+/324991
Trust: Matthew Dempsky <mdempsky@google.com>
Run-TryBot: Matthew Dempsky <mdempsky@google.com>
Reviewed-by: Robert Griesemer <gri@golang.org>
TryBot-Result: Go Bot <gobot@golang.org>
@gopherbot
Copy link

Change https://golang.org/cl/325009 mentions this issue: [dev.typeparams] cmd/compile/internal/syntax: not all index expressions can be instantiated types

gopherbot pushed a commit that referenced this issue Jun 5, 2021
@griesemer
[dev.typeparams] cmd/compile/internal/syntax: not all index expressio…
692399f 
…ns can be instantiated types

An index expression followed by an opening "{" may indicate
a composite literal but only if the index expression can be
a type. Exclude cases where the index expression cannot be
a type (e.g. s[0], a[i+j], etc.).

This leads to a better error message in code that is erroneous.

Fixes #46558.

Change-Id: Ida9291ca30683c211812dfb95abe4969f44c474f
Reviewed-on: https://go-review.googlesource.com/c/go/+/325009
Trust: Robert Griesemer <gri@golang.org>
Reviewed-by: Robert Findley <rfindley@google.com>
@griesemer
Copy link
Contributor

griesemer commented Aug 4, 2021 •
edited

This was fixed with the above CLs.

@golang golang locked and limited conversation to collaborators Jun 23, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Milestone
Go1.18
Development

No branches or pull requests
4 participants
@mdempsky @ianlancetaylor @griesemer @gopherbot