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: inconsistency in recorded types for instantiations #60212

Closed
findleyr opened this issue May 15, 2023 · 7 comments
Closed

go/types: inconsistency in recorded types for instantiations #60212

findleyr opened this issue May 15, 2023 · 7 comments
Assignees
@griesemer
Labels
NeedsFix The path to resolution is known, but the work has not been done.
Milestone
Go1.21

Comments

@findleyr
Copy link
Contributor

findleyr commented May 15, 2023 •
edited

This issue describes what is arguably a bug in the go/types API, which we may or may not be able to fix.

Specifically, consider the following generic declaration and instantiations:

func f[P, Q any](P, Q) {}

func _() {
  f(0, "")              // 0 type arguments provided
  f[int](0, "")         // 1 type argument provided
  f[int, string](0, "") // 2 type arguments provided
}

In this example, we record in types.Info.Types the type func(int, string) for the expressions f[int] and f[int, string], but for all f identifiers we record the type func[P, Q any](P, Q).

This is inconsistent, and in hindsight clearlyprobably wrong: types.Info.Types should record the effective type of each node in the syntax tree. In this case, the Fun expression of the call expression must have type func(int, string). types.Info.Types should never contain generic types.

There is some discussion of this at #47916 (comment), where I agreed with @mdempsky that it makes most sense to record the instance for the identifier. Unfortunately, that is not what was implemented, most likely as an artifact of the implementation. Mea culpa.

Now that new forms of inference are being implemented, it is harder to preserve this inconsistency. Can we fix this bug so that we record the instance for f, or must we preserve this behavior as a historical artifact?

On the one hand, this bug has existed for over a year. On the other hand, the current behavior is inconsistent, and likely to cause bugs for tool authors.

CC @griesemer @mdempsky @adonovan @dominikh
@findleyr findleyr added this to the Go1.21 milestone May 15, 2023
@griesemer griesemer self-assigned this May 15, 2023
@griesemer griesemer added the NeedsFix The path to resolution is known, but the work has not been done. label May 15, 2023
@mdempsky
Copy link
Member

Is the question here just what Type should be recorded in Info.Types for the f identifier in f(0, "")? I.e., whether it should be the generic type of uninstantiated f, or the inferred type from its usage?

Personally, I'm fine either way. Regardless of what Types[f] holds, it's possible to explicitly get the uninstatiated or instantiated type with minimal extra work:

  • For the uninstantiated type, notice that the ast.Expr is an *ast.Ident, look it up in Info.Uses, and get the Origin Object's Type.
  • For the instantiated type, notice that the ast.Expr is an *ast.Ident, look it up in Info.Instances.

FWIW, I think golang.org/cl/492455 probably wouldn't have been necessary if Types[f] held the instantiated type.

As for the cases where f is explicitly instantiated with one or more type arguments, I'd still lean towards thinking those Types[f] entries should have the uninstantiated generic type. I can't tell whether you're suggesting they should be changed too.

@findleyr
Copy link
Contributor Author

Is the question here just what Type should be recorded in Info.Types for the f identifier in f(0, "")
I can't tell whether you're suggesting they should be changed too.

Yes, I would argue that the f identifier in f[int] and f[int, string] should also be the instance. As well as in the explicit instantiation f[int, string] (without a call expression).

@griesemer
Copy link
Contributor

At the moment (with pending CL 494116), we're close to what is being asked here, with the exception @mdempsky is pointing out:

As for the cases where f is explicitly instantiated with one or more type arguments, I'd still lean towards thinking those Types[f] entries should have the uninstantiated generic type. I can't tell whether you're suggesting they should be changed too.

@mdempsky
Copy link
Member

Yes, I would argue that the f identifier in f[int] and f[int, string] should also be the instance.

Can you elaborate why? f[int] is only valid because f has generic type. It seems inconsistent to me to record the non-generic type instead.

I understand and see the tension in the implicit instantiation case: there are two interesting types (uninstantiated and inferred instantiation), and only one of them can be recorded in Info.Types.

But for explicit instantiations, I think the identifier should have the generic type and the IndexExpr/IndexListExpr should have the instantiated type. The index expression is the operation that transformed it from generic to instantiated type. It's not like f[int][int] is valid.

@findleyr
Copy link
Contributor Author

Can you elaborate why? f[int] is only valid because f has generic type. It seems inconsistent to me to record the non-generic type instead.

Hmm, point taken about f[int][int]. My rationale was that it doesn't really make sense to record any generic types in types.Info.Types, since this map records the types of expressions, and expressions cannot be generic. However, in that case the correct course of action was probably not to record anything for the f in f[int, string], instead relying on types.Info.Instances in types.Info.TypeOf. But I think it's too late to make that change now. It's also more likely that people are relying on the current behavior when inspecting an index expression. Perhaps we should just change this in the case of f(0, "").

@griesemer
Copy link
Contributor

@findleyr Is there anything left to do here?

@findleyr
Copy link
Contributor Author

findleyr commented Jun 6, 2023

If we now record the correct type for call.Fun, I think that is good enough.

@findleyr findleyr closed this as completed Jun 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@griesemer griesemer

Labels
NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Milestone
Go1.21
Development

No branches or pull requests
3 participants
@mdempsky @griesemer @findleyr