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

x/tools/gopls: completion gives unneeded generic type instantiation snippet #51783

Closed
muirdm opened this issue Mar 18, 2022 · 8 comments
Closed
Assignees
Labels
gopls/completion Issues related to auto-completion in gopls. gopls/generics Issues related to gopls' support for generics gopls Issues related to the Go language server, gopls. Tools This label describes issues relating to any tools in the x/tools repository.
Milestone

Comments

@muirdm
Copy link

muirdm commented Mar 18, 2022

func foo[T float64 | int](a T) T {
	return a
}

func main() {
	var _ int = fo<>
}

Completing to "foo" at <> inserts "int(foo[T float64|int](a T))" (ignore the extra type conversion), but most likely the type argument can be inferred so gopls should offer just "foo(a int)" instead.

@gopherbot gopherbot added Tools This label describes issues relating to any tools in the x/tools repository. gopls Issues related to the Go language server, gopls. labels Mar 18, 2022
@gopherbot gopherbot added this to the Unreleased milestone Mar 18, 2022
@muirdm
Copy link
Author

muirdm commented Mar 18, 2022

Would this work: if there is one type param and it is the func’s return type and one of the variants is assignable to the expected type (int in this case), then presume type inference will work and omit the instantiating snippet? Is there any programmatic interface to check type param inference?

I guess it is more question of "will type inference work for this function call", and if "yes" then don't give the type param snippet. If go/types can't tell us that, then I feel like some simple heuristics can cover common cases. For example, if all the type params are referenced in the param list, then type inference will work (is that correct?).

@suzmue suzmue modified the milestones: Unreleased, gopls/on-deck Mar 18, 2022
@muirdm
Copy link
Author

muirdm commented Mar 26, 2022

@findleyr Can we hook in to type param inference to determine a priori if a generic function's type params will/might be inferable after the normal params are filled in? I know sometimes it depends on the arguments, but my feeling is that type param inference works most of the time if it can work at all for a given function.

In a similar vein, the heuristic I suggested above would not work in all cases, but I think it would be an improvement (i.e. if type inference will "probably" work, don't give the type param snippet).

@findleyr findleyr added gopls/completion Issues related to auto-completion in gopls. gopls/generics Issues related to gopls' support for generics labels May 17, 2022
@rogeryk
Copy link
Contributor

rogeryk commented Nov 30, 2023

The completion of generic functions is currently very poor. In most cases, type parameters are unnecessary. If you use completion now, you must also delete the type parameters after completion.

The standard library already has the "slices" and "maps" packages, so providing a completion experience for generic functions would be very helpful. If it is difficult to determine whether type parameters should be completed, it is also acceptable to remove the type parameters in complete (it is only needed in rare cases).

@findleyr
Copy link
Contributor

@muirdm suggested here essentially the same as I suggested in #64480 (comment), apologies that this slipped through the cracks during generics work.

go/types does not provide precisely what we need, but as I commented on the other issue, I think a simple heuristic is probably good enough.

We should fix this soon.

@adonovan
Copy link
Member

Consider slices.Clone, which is representative of a large number of library functions that are cautiously written for the reasons given in https://go.dev/blog/deconstructing-type-parameters. Its type is:

func Clone[S ~[]E, E any](s S) S

The heuristic proposed above suggests that we can omit the suffix of type parameters that are free (referenced by) the parameter types, but in this case, E is not free, to ensure that all of these are valid:

https://go.dev/play/p/HM9slI86sI5

	type strings []string

	type stringy string
	type stringys []stringy
	{
		var x []string
		slices.Clone(x)
		slices.Clone[[]string](x)
		slices.Clone[[]string, string](x)
		slices.Clone[strings](x)         // non-trivial
		slices.Clone[strings, string](x) // non-trivial
	}
	{
		var x strings
		slices.Clone(x)
		slices.Clone[[]string](x)         // non-trivial
		slices.Clone[[]string, string](x) // non-trivial
		slices.Clone[strings](x)
		slices.Clone[strings, string](x)
	}
	{

		var x stringys
		slices.Clone(x)
		slices.Clone[[]stringy](x)          // non-trivial
		slices.Clone[[]stringy, stringy](x) // non-trivial
		slices.Clone[stringys, stringy](x)
		slices.Clone[stringys, stringy](x)
	}

(The instantiations that would not be inferred are labeled "non-trivial".)

So I think this heuristic will not work for such cautiously-written library functions.

@rogeryk
Copy link
Contributor

rogeryk commented Dec 23, 2023

E can infer from S, so the E also is free.

(The instantiations that would not be inferred are labeled "non-trivial".)

So I think this heuristic will not work for such cautiously-written library functions.

I don’t think we need to consider whether the type parameter can be omitted in all scenarios, the type parameter should always be specified in rare cases.

This problem doesn't seem to be solved quickly. I thought of a simple solution to alleviate this problem, we can change the tabstop range.
For example.
current complete snippet

slices.Clone[${1:S ~[]E}, ${2:E any}](${3:s S})

change to

slices.Clone${1:[${2:S ~[]E}, ${3:E any}]}(${4:s S})

Then we can simply ignore the type parameter if we don't need it

@rogeryk
Copy link
Contributor

rogeryk commented Dec 23, 2023

I gave it a try and it looks good, it shouldn't be too difficult whether you need the type parameters or not.

iShot_2023-12-24_00 30 26

@gopherbot
Copy link

Change https://go.dev/cl/554855 mentions this issue: gopls/internal/lsp/source/completion: infer parameter types

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
gopls/completion Issues related to auto-completion in gopls. gopls/generics Issues related to gopls' support for generics gopls Issues related to the Go language server, gopls. Tools This label describes issues relating to any tools in the x/tools repository.
Projects
None yet
Development

No branches or pull requests

6 participants