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

cmd/go: regression compiling "const X = string(rune(31))" #34563

Closed
mark-rushakoff opened this issue Sep 26, 2019 · 8 comments
Closed

cmd/go: regression compiling "const X = string(rune(31))" #34563

mark-rushakoff opened this issue Sep 26, 2019 · 8 comments
Assignees
Milestone

Comments

@mark-rushakoff
Copy link
Contributor

@mark-rushakoff mark-rushakoff commented Sep 26, 2019

Given this file:

package stringrune

const X = string(rune(31))

It compiled successfully with go build on go 1.12, but at tip it failed with:

const initializer string(rune(31)) is not a constant

So I used git bisect to determine that 581526c (cmd/compile: rewrite untyped constant conversion logic) was the Go commit where it stopped compiling.

I skimmed through the spec and I wasn't able to quickly determine whether the current or previous behavior was conformant.

This particular line exists in the wild, in influxdb: https://github.com/influxdata/influxdb/blob/99ef5fb3e44400bcc49383035ab7c81bd7be4979/services/continuous_querier/service.go#L28. I have no context on why it was written like that instead of a string literal "\x1f".

/cc @mdempsky @griesemer

@griesemer griesemer added this to the Go1.14 milestone Sep 26, 2019
@griesemer

This comment has been minimized.

Copy link
Contributor

@griesemer griesemer commented Sep 26, 2019

Probably a simple oversight. Assigning to @mdempsky as he's recently changed this code. Release blocker because it's a regression.

@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Sep 26, 2019

Change https://golang.org/cl/197677 mentions this issue: cmd/compile: apply constant folding to ORUNESTR

@mdempsky

This comment has been minimized.

Copy link
Member

@mdempsky mdempsky commented Sep 26, 2019

The issue is evconst wasn't performing constant folding for integer->string conversions.

This used to not be a problem because convertlit1 would internally handle constant folding for explicit conversions, but 581526c changed convertlit1 to short-circuit if the operand is already typed, expecting that evconst would handle this instead.

@mdempsky

This comment has been minimized.

Copy link
Member

@mdempsky mdempsky commented Sep 26, 2019

Fixed. Thanks for the report!

@gopherbot gopherbot closed this in ac1d440 Sep 26, 2019
@av86743

This comment has been minimized.

Copy link

@av86743 av86743 commented Sep 27, 2019

@mdempsky

Notice that your test does not actually validate against original problem (constant initializer not recognized as a constant.)

@mdempsky

This comment has been minimized.

Copy link
Member

@mdempsky mdempsky commented Sep 27, 2019

@av86743 What do you mean? I added two new (blank) constant declarations.

Did you maybe miss the const keyword on line 13? Or am I missing something?

@av86743

This comment has been minimized.

Copy link

@av86743 av86743 commented Sep 27, 2019

@mdempsky My mistake. Apologies.

@mdempsky

This comment has been minimized.

Copy link
Member

@mdempsky mdempsky commented Sep 27, 2019

@av86743 No problem. Thanks for confirming. :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.