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: 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
Assignees
Labels
Milestone

Comments

@mdempsky
Copy link
Member

@mdempsky mdempsky commented Apr 21, 2021

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

@findleyr
Copy link
Contributor

@findleyr findleyr commented Apr 21, 2021

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].

Loading

@gopherbot
Copy link

@gopherbot gopherbot commented Apr 21, 2021

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

Loading

@griesemer
Copy link
Contributor

@griesemer griesemer commented Apr 21, 2021

Yes, it does look like a bug.

Loading

gopherbot pushed a commit that referenced this issue Apr 21, 2021
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

@gopherbot gopherbot commented Apr 22, 2021

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

Loading

@gopherbot gopherbot closed this in f7afdfd Apr 22, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
4 participants