Skip to content

Quick fix 1002 suggests use of tagged switch statement inappropriately #1613

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

Closed
seh opened this issue Oct 24, 2024 · 3 comments
Closed

Quick fix 1002 suggests use of tagged switch statement inappropriately #1613

seh opened this issue Oct 24, 2024 · 3 comments
Labels

Comments

@seh
Copy link

seh commented Oct 24, 2024

Consider the following code, which I admit could use strings.Cut instead:

// Variable "c" is of type string.
// We wish to split it on an embedded space character, if any.
switch i := strings.IndexByte(c, ' '); {
case i == -1:
	return c, "", nil
case i == 0:
	return "", "", errors.New("value starts with a space character")
default:
	return c[:i], c[i+1:], nil
}

My LSP implementation—gopls—reports the following:

could use tagged switch on i [default]

It then offers a quick fix: "Replace with tagged switch". If I accept that suggestion, though, the resulting code is invalid, since I lose access to the "i" variable used in the return statement in the default case.

// Note the extra "i" here.
switch i i := strings.IndexByte(c, ' '); {
case -1:
	return c, "", nil
case 0:
	return "", "", errors.New("value starts with a space character")
default:
	// We can no longer use "i" here.
	return c[:i], c[i+1:], nil
}

Note also the strange insertion of an extra "i" in the initial simple statement.

It seems that there are three things wrong here:

  • The recommendation is unjustified, as we can't eliminate the "i" variable, given its use in the return statement.
  • The applied fix changes the first two case clauses, but leaves the default case clause with dangling references to a nonexistent variable.
  • The applied fix mangles the initial simple statement.

I don't know whether any of these are to blame on Emacs's lsp-mode that's making use of gopls.

@seh seh added the needs-triage Newly filed issue that needs triage label Oct 24, 2024
@dominikh
Copy link
Owner

I reckon the intended rewrite is switch i := strings.IndexByte(c, ' '); i { which AFAICT would be correct. I'll have to check tomorrow if we emit a broken quickfix, or if this is an instance of golang/go#63930.

@dominikh dominikh added bug and removed needs-triage Newly filed issue that needs triage labels Oct 24, 2024
@dominikh
Copy link
Owner

2739fd0 fixes this. It will take a release of Staticcheck and a release of gopls for this to reach users, however.

@seh
Copy link
Author

seh commented Oct 24, 2024

Thank you for the fast diagnosis and correction!

I'm going to rewrite the original function to use strings.Cut, but I'm glad that its current state allowed us to find this burr to sand off.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants