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: code action to "Add switch cases for [enum|type]" #65411

Closed
martskins opened this issue Jan 31, 2024 · 4 comments
Closed

x/tools/gopls: code action to "Add switch cases for [enum|type]" #65411

martskins opened this issue Jan 31, 2024 · 4 comments
Labels
FeatureRequest gopls Issues related to the Go language server, gopls. Refactoring Issues related to refactoring tools Tools This label describes issues relating to any tools in the x/tools repository.
Milestone

Comments

@martskins
Copy link

gopls version

v0.14.2

go env

Not relevant

What did you do?

Consider the following snippet:

type AnimalType int

const (
	AnimalTypeUnknown AnimalType = iota
	AnimalTypeDog
	AnimalTypeCat
	AnimalTypeHorse
)

func main() {
	var at AnimalType
	switch at {
	case AnimalTypeDog:
		// do stuff
	}
}

(I realise switching on at in that example doesn't make sense, just doing that to illustrate the use-case)

What did you see happen?

N/A

What did you expect to see?

It would be nice if gopls offered completing the missing switch arms as a code action.

I'm thinking this could be offered only when there's a finite set of cases, so it would work for defined types such as the example above, but not on basic types such as string, int, etc.

Is there an appetite to implement something like this? Or is it intentionally not supported?

Editor and settings

No response

Logs

No response

@martskins martskins added gopls Issues related to the Go language server, gopls. Tools This label describes issues relating to any tools in the x/tools repository. labels Jan 31, 2024
@gopherbot gopherbot added this to the Unreleased milestone Jan 31, 2024
@adonovan
Copy link
Member

adonovan commented Feb 1, 2024

Nice idea! It should also support type switches: such a feature would be helpful to me to as I frequently write functions that switch over ast.Node or types.Type or types.Object.

I suggest the precise criteria should be:

  • a switch expr {} statement should offer a code action at the "switch" keyword to "Add cases for AnimalType", if the type of expr is a named constant declared in a single const declaration.
  • a switch iface.(type) {} statement should offer a code action at the "switch" keyword to "Add cases for ast.Node". It should add cases for all accessible types that implement the interface and are declared in the same package as it.
  • If the switch already has some cases, it should fill in the missing ones.
  • If all are present (or there is a "default" case), then there should be no code action.
  • Only constants or types that are accessible to the switch statement (i.e. exported, or in same package) should be offered.

Do you want to take a crack at it yourself?

@adonovan adonovan added FeatureRequest Refactoring Issues related to refactoring tools labels Feb 1, 2024
@adonovan adonovan changed the title x/tools/gopls: Provide fill switch arms action x/tools/gopls: code action to "Add switch cases for [enum|type]" Feb 1, 2024
@adonovan adonovan modified the milestones: Unreleased, gopls/unplanned Feb 1, 2024
@martskins
Copy link
Author

martskins commented Feb 1, 2024

Agree with all your points! I do I think offering it when there's a default case could potentially be useful as well, but I guess it's probably better to do a first implementation without that and add it later if people think it's useful.

I'm happy to take a stab at this but I'm gonna need some hand-holding. I played around a bit trying to do this but I'm 100% sure it wasn't the right way to do it and also it fell short when the type wasn't declared on the same file that the switch was in.

Edit: So I was having another look now and managed to make it work for types in other packages, but I'm still sure this is not anywhere near the correct way of doing this. I'll take any pointers you can give me and will take some more time to try and make type switches work.

@adonovan
Copy link
Member

adonovan commented Feb 3, 2024

it fell short when the type wasn't declared on the same file that the switch was in.

Most of the computation will be done in the domain of go/types: you'll need to identify the type of the switch operand (a types.Type, found in types.Info.TypeOf(expr)), check that it is a named type (*types.Named), find its package, (Named.Obj().Pkg(), a types.Package), observe that there are various accessible named constants of the same type (types.Identical) in the same package (by iterating over Package.Scope.{Named,Lookup}), and subtract the cases that already exist (by again using TypeOf for each case).

Type switches will be similar except that you are iterating over types, not constants, and testing compatibility with the switch operand using types.AssignableTo. You should test whether each *T as assignable to the interface if plain old T is not, as the pointer type may have more methods.

Send a CL when you're ready, even if just to ask for clarification.

@martskins
Copy link
Author

Cool! That was very helpful! I think I got something more or less working, will share a CL soon!

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

Successfully merging a pull request may close this issue.

4 participants