`PointerTo()` produces different results for repeat invocations

[thockin]

For every type in a Universe, there is a single Type object instance. This allows comparisons, e.g. to types.String. The way PointerTo() is implement, PointerTo(String) != PointerTo(String).

We should probably change it to do be safer.

[thockin]

Also: if we want other common types like error or utils like SliceOf() or ArrayOf()...

[ziyue-101]

/assign @ziyue-101 for fixit week

[yongruilin]

/cc @yongruilin

[ziyue-101]

One thought is when loading non-pointer types, load corresponding pointer types in the types map in gengo.Package. Each time PointerTo() is triggered, return the right pointer. However, this requires adding a pointer receiver the func such as (*Package) PointerTo().

The caller usage for PointerTo() is expecting [a single gengo.Type object](https://cs.k8s.io/?q=types%5C.PointerTo%5C(&i=nope&literal=nope&files=&excludeFiles=&repos=). I am wondering if it is viable/reasonable to change the way of usage today?

/cc @yongruilin

[thockin]

You are right, for the few places that call PointerTo(), we don't have a reference to the Universe, so we can't "just" change it to u.PointerTo(t).

We could maybe do something like:

• store *Universe in each Type
• call t.Pointer() which would:
  • lookup the pointer type in the Universe
  • add it if needed
  • return it

This is super racy, since all the universe setup is assumed to be single-threaded (no mutexes or anything, just a map). So I think we can't do that. I hate the Unverse API for this reason but changing it is a lot of work and risk.

We could instead keep a cache of synthetic types (struct with a mutex and a Package):

• store *Universe in each Type
• store *syntheticCache in each Type
• call t.Pointer() which would:
  • lookup the pointer type in the Universe (careful to not create it), if found return it
  • otherwise add it to the synthetic cache (under the mutex)
  • return it

That adds 2 pointers to every Type, which is ugly but not intolerable, I think? Every call should get the same result.

Additionally, we could add a builtin value (like types.String) for Error.

Or we could just document the "bad" behavior, I guess

[yongruilin]

This SGTM. Since Universe is a map, should it be ok if we just put a non-pointer Universe in Type? So that we can just modify the WalkType function with something like

// walkType adds the type, and any necessary child types.
func (p *Parser) walkType(u types.Universe, useName *types.Name, in gotypes.Type) (out *types.Type) {
	defer func() {
		out.Universe = u
	}()
...

[ziyue-101]

FYI, I have a PR that implements the Tim's idea. Let me know if it works, thanks!

[thockin]

should it be ok if we just put a non-pointer Universe in Type?

Maps are by reference so, no. Also we want to share, since types are built up incrementally. There is only one universe, and we wanted to stay that way. It's unfortunately not very well abstracted.

[k8s-triage-robot]

The Kubernetes project currently lacks enough contributors to adequately respond to all issues.

This bot triages un-triaged issues according to the following rules:

• After 90d of inactivity, lifecycle/stale is applied
• After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
• After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

• Mark this issue as fresh with /remove-lifecycle stale
• Close this issue with /close
• Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale