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, go/constant: improve documentation on constant.Kind and relationship to concrete types #45906

Open
dominikh opened this issue May 1, 2021 · 4 comments
Open

go/types, go/constant: improve documentation on constant.Kind and relationship to concrete types #45906

dominikh opened this issue May 1, 2021 · 4 comments
Labels
Documentation NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone
Backlog

Comments

@dominikh
Copy link
Member

dominikh commented May 1, 2021

ISTM that go/types and go/constant don't currently do a good job at explaining which constant.Kind can be encountered in which contexts. For example, before seeing #43891 I wasn't aware that a floating point constant may have Kind() == Int.

And while that issue explains that go/constant may use a more efficient representation, it doesn't mention if it may use a less efficient representation, especially in relationship with the type checker. For example, in the following example

var foo func(int)
const k = 4.0
_ = 1 << k
foo(k)

it is not clear to me whether the types.TypeAndValue for the ast.Ident k as used in the bit shift and function call are guaranteed to be a constant.Int or not. It currently seems to be, and existing code in vet doesn't defensively use ToInt before using Uint64Val

Summarizing, I'd like to see documentation that at least

  • mentions that constant kinds aren't directly related to concrete types
  • explains which simplifying guarantees go/types makes, if any
  • states how defensively code has to be written in practice

/cc @griesemer
@cagedmantis cagedmantis added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label May 4, 2021
@cagedmantis cagedmantis added this to the Backlog milestone May 4, 2021
@griesemer griesemer self-assigned this May 6, 2021
@griesemer
Copy link
Contributor

Regarding the specific example above: The k is defined as 4.0 which is of kind constant.Float (not constant.Int). It happens to be representable as an integer value without loss of precision, so the shift 1 << k is valid. And the result of that shift is another constant, of kind constant.Int (because shift results are always integers). In general, the kind associated with a constant literal must be the kind implied by its notation, which in turn explains what its default type would be, if it came into play (otherwise the default type couldn't be correctly computed).

In general, constant.Kind's of constants referring to constant literals don't change. And the constant.Kind of a constant expression depends on the expression. For instance, for the above k, 1 + k has kind constant.Float but int(1) + k has kind constant.Int.

@dominikh
Copy link
Member Author

dominikh commented May 7, 2021

In general, constant.Kind's of constants referring to constant literals don't change

Am I understanding correctly that you mean to say that in 1 << k, the constant.Kind of k won't be constant.Int? Because that's not what I am seeing. The Kind changes based on the specific use of k: https://play.golang.org/p/31JgWHr91-o

@griesemer
Copy link
Contributor

Sorry, I was not precise: The constant.Kind of an identifier (which is an expression) depends on context, so if we have a shift and the constant (k) is acceptable as shift count (in this case because it's representable as an integer), then the constant.Kind for that identifier is constant.Int. But the constant.Kind of the value associated with the k Const object will reflect the syntax of that constant and won't change (example) - in fact it can't change otherwise we'd lose the information about the literal representation. Hopefully that makes things a bit clearer.

@dominikh
Copy link
Member Author

dominikh commented May 8, 2021

It does, thank you.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Documentation NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Milestone
Backlog
Development

No branches or pull requests
3 participants
@dominikh @cagedmantis @griesemer