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

Jump to bottom

go/types: suspicious return of Typ[Int] in (*Checker).index #45667

Closed
mdempsky opened this issue Apr 21, 2021 · 4 comments
Closed

go/types: suspicious return of Typ[Int] in (*Checker).index #45667

mdempsky opened this issue Apr 21, 2021 · 4 comments
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Milestone
Go1.17

Comments

@mdempsky
Copy link
Member

While looking at go/types source, I noticed this return statement at the end of (*Checker).index:

go/src/go/types/expr.go

Lines 1047 to 1048 in 5f1df26

// 0 <= v [ && v < max ]
return Typ[Int], v

I think this should probably use x.typ instead of Typ[Int]. Untyped constants were already converted to int at line 1020.

I think the consequence of this is that for an expression like make([]int, uint(1)), the signature passed to recordBuiltinType will incorrectly report the parameter as int instead of uint. I haven't confirmed this though.

/cc @griesemer @findleyr
@mdempsky mdempsky added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Apr 21, 2021
@findleyr
Copy link
Contributor

Thanks. Your analysis is correct, and I've confirmed we're recording an incorrect signature.

We should fix this (right?), but that does seem to be the only consequence -- other uses of Checker.index only check for Typ[Invalid].

@griesemer griesemer added this to the Go1.17 milestone Apr 21, 2021
@griesemer griesemer added NeedsFix The path to resolution is known, but the work has not been done. and removed NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. labels Apr 21, 2021
@gopherbot
Copy link

Change https://golang.org/cl/312309 mentions this issue: cmd/compile/internal/types2: fix incorrect result type of Checker.index

@griesemer
Copy link
Contributor

Yes, it does look like a bug.

gopherbot pushed a commit that referenced this issue Apr 21, 2021
@griesemer
cmd/compile/internal/types2: fix incorrect result type of Checker.index
b8a359d 
While at it, add missing "invalid argument: " prefix
to a couple of local error messages, for consistency.

For #45667.

Change-Id: I814800b2f3f3750583e335c98a3f8e27030a9daa
Reviewed-on: https://go-review.googlesource.com/c/go/+/312309
Trust: Robert Griesemer <gri@golang.org>
Run-TryBot: Robert Griesemer <gri@golang.org>
Reviewed-by: Robert Findley <rfindley@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
@gopherbot
Copy link

Change https://golang.org/cl/312591 mentions this issue: go/types: cleanup and fix Checker.index

@golang golang locked and limited conversation to collaborators Apr 22, 2022
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.17
Development

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