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: marker test should ensure type checked file remain type checked after code edits #70604

Closed
h9jiang opened this issue Nov 27, 2024 · 1 comment
Labels
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

@h9jiang
Copy link
Member

h9jiang commented Nov 27, 2024

gopls version

Any version.

go env

N/A

What did you do?

Run fillstruct code action in a type checked file, after the struct being filled, the file have type errors.

In the fillstruct marker test,

Originally, the file is type checked without any error:

type basicStructWithTypeParams[T any] struct {
	foo T
}

type nestedStructWithTypeParams struct {
	bar   string
	basic basicStructWithTypeParams[int]
}

var _ = nestedStructWithTypeParams{} // <- fill struct code action

After trigger the code action, the generated code:

var _ = nestedStructWithTypeParams{
	bar:   "",
	basic: basicStructWithTypeParams{},
}

The basic field is filled with ast.CompositeLit of type basicStructWithTypeParams, instead it should be basicStructWithTypeParams[int].

I'm working on a fix for this specific issue. The test here is to test the behavior of the code action but it generate the wrong code and create type errors.

I think it would be better if we can catch this early. We thought we are generating the right code but we are not. Here is what I hope can be implemented in marker test:

  • Run type check before trigger the code action or edits. PASS or NOT PASS.
  • Trigger code action or apply edits.
  • Run type check again. If it PASSed typecheck before the code action, it should PASS after the code action.

And maybe have some options for the developer to say we want to skip this typecheck check if they know what they are doing.

What did you see happen?

N/A

What did you expect to see?

The file should remain type checked after code action.

Editor and settings

No response

Logs

No response

@h9jiang h9jiang 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 Nov 27, 2024
@gopherbot gopherbot added this to the Unreleased milestone Nov 27, 2024
@findleyr
Copy link
Member

I don't think we should do this. The marker tests currently zip through the test data requesting edits for many different operations. It would really slow them down to have to mutate the editor state at each marker, wait for diagnostics, and then undo the edit, before proceeding to the next. This is also quite a complex operation, and may lead to significant complexity and/or bugs.

It should be the job of code review to find bugs in tests. And if we miss something, that's OK, we can fix it later.

It should not be necessary to verify that the expected golden output is in fact golden, especially at the cost of significantly slower and more complex tests.

@findleyr findleyr closed this as not planned Won't fix, can't repro, duplicate, stale Dec 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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

3 participants